From: Petr Mladek
Sent: 14 August 2023 13:56
On Mon 2023-08-14 10:42:26, David Laight wrote:
From: Kees Cook
Sent: 11 August 2023 06:46
If an output buffer size exceeded U16_MAX, the min_t(u16, ...) cast in copy_data() was causing writes to truncate. This manifested as output bytes being skipped, seen as %NUL bytes in pstore dumps when the available record size was larger than 65536. Fix the cast to no longer truncate the calculation.
...
diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c index 2dc4d5a1f1ff..fde338606ce8 100644 --- a/kernel/printk/printk_ringbuffer.c +++ b/kernel/printk/printk_ringbuffer.c @@ -1735,7 +1735,7 @@ static bool copy_data(struct prb_data_ring *data_ring, if (!buf || !buf_size) return true;
- data_size = min_t(u16, buf_size, len);
- data_size = min_t(unsigned int, buf_size, len);
I'd noticed that during one of my test compiles while looking at making min() less fussy.
A better fix would be: data_size = min(buf_size + 0u, len);
This looks like a magic to me. The types are:
Not quite the right magic though, needs to be 'len + 0u'.
unsigned int data_size; unsigned int buf_size; u16 len
I would naively expect that
data_size = min(buf_size, len);
would do the right job and expand @len to "unsigned int".
I do not remember why "min_t" was used. Was it an optimization? Did we miss the problem with casting "u32" down to "u16"?
The underlying problem is that (presumably) in order to stop min(signed_a, unsigned_b) converting a negative value to a large unsigned one (very nasty) min() contains (effectively) sizeof(&a == &b) so barfs if the types differ at all.
I'm sure the intent was that the types would be fixed - in this case chasing 'len' back all the way back and using 'unsigned int'. (That probably generates better code as well.)
However everyone just uses min_t(type,a,b) if type is 32bit unsigned they are mostly ok because the kernel only really deals in 'small' unsigned values. But, as in the case here, it is easy to pick a type that is too small. Pretty much all the min_t() with u8/u16 are likely to be dubious. I found an 'unsigned long' case in a filesystem where one value was u64 - could be problematic for a large file on 32bit. (The u64 definitely contained a 'file size' value.)
The patch set I proposed (see https://lore.kernel.org/lkml/01e3e09005e9434b8f558a893a47c053@AcuMS.aculab.c...) changes the basic test to (is_signed(a) == is_signed(b)) which will never generate the 'nasty' conversion of -1 to 0xffffffffull.
Of course, it is never quite that simple :-) Linus seems willing to accept min(unsigned_var, 20) but not min(signed_var, 20u) - typically as min(signed_var, sizeof(type)).
...
PS: I have already pushed the patch because it looked reasonable and got testing. I have to admit that I am probably in a pre-vacation hurry mode.
Don't worry it is now not any worse than the other 4500 min_t(). Much the same as the number of min().
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)