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@xxxxxxxxxxxxxxxx/) 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)
