| Message ID | 20230811054528.never.165-kees@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 53e9e33ede37a247d926db5e4a9e56b55204e66c |
| Headers | show |
| Series | printk: ringbuffer: Fix truncating buffer size min_t cast | expand |
On 8/10/23 22:45, Kees Cook wrote: > 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. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: John Ogness <john.ogness@linutronix.de> > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com> > Closes: https://lore.kernel.org/lkml/d8bb1ec7-a4c5-43a2-9de0-9643a70b899f@linux.microsoft.com/ > Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Excellent. I have verified on v5.10. Tested-by: Vijay Balakrishna <vijayb@linux.microsoft.com> Thanks, Vijay
Great finding! Thanks a lot for the report and the fix - oneliners are
usually the most challenging to debug.
Tested it in the Steam Deck, and it works perfectly - I saw eventually
one line or two filled with NULLs, now they're gone.
Feel free to add:
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com> # Steam Deck
On 2023-08-10 22:45:32, Kees Cook wrote: > 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. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: John Ogness <john.ogness@linutronix.de> > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com> > Closes: https://lore.kernel.org/lkml/d8bb1ec7-a4c5-43a2-9de0-9643a70b899f@linux.microsoft.com/ > Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Nice find! Reviewed-by: Tyler Hicks (Microsoft) <code@tyhicks.com> Tested-by: Tyler Hicks (Microsoft) <code@tyhicks.com> Verified the fix by applying it to an instrumented v6.5-rc5 kernel that allows userspace to execute kmsg_dump(), detects NULL bytes in data copied from the ring buffer, and warns about invalid truncation due to the min_t(u16, ...) casting bug. Everything looks good! Tyler > --- > kernel/printk/printk_ringbuffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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); > > memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */ > return true; > -- > 2.34.1 >
On 2023-08-10, Kees Cook <keescook@chromium.org> wrote: > 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. Thanks for tracking this down. Reviewed-by: John Ogness <john.ogness@linutronix.de>
On (23/08/10 22:45), Kees Cook wrote: > 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. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: John Ogness <john.ogness@linutronix.de> > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com> > Closes: https://lore.kernel.org/lkml/d8bb1ec7-a4c5-43a2-9de0-9643a70b899f@linux.microsoft.com/ > Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Thanks a lot! Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
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); Or put an ack on my patch 3/5 to minmax.h and then min(buf_size, len) will be fine (because both arguments are unsigned). David > > memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */ > return true; > -- > 2.34.1 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu 2023-08-10 22:45:32, Kees Cook wrote: > 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. > > Cc: Petr Mladek <pmladek@suse.com> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: John Ogness <john.ogness@linutronix.de> > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com> > Closes: https://lore.kernel.org/lkml/d8bb1ec7-a4c5-43a2-9de0-9643a70b899f@linux.microsoft.com/ checkpatch.pl suggested that "Link:" should be used instead of "Closes:". > Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Petr Mladek <pmladek@suse.com> Thanks a lot for tracking this down. The patch has been comitted into printk/linux.git, branch for-6.6. I though about pushing it for 5.5-rc7. But it is pretty old issue. It does not break the system. I wanted to give it some spin in linux-next. And I leave for vacation on Thursday. I will not have internet connection until Aug 28. Best Regards, Petr
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: 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"? I tried to read the discussion at https://lore.kernel.org/lkml/b6a49ed73aba427ca8bb433763fa94e9@AcuMS.aculab.com/ but it is more about "signed" vs. "unsigned" problem. Maybe it is more complicated that I expected. > Or put an ack on my patch 3/5 to minmax.h and then min(buf_size, len) > will be fine (because both arguments are unsigned). Do you mean https://lore.kernel.org/lkml/6dc20ac7cb6f4570a0160f076e8362e3@AcuMS.aculab.com/ ? It seems to be just indentation cleanup. Best Regards, Petr 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.
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.com/) 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)
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); memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */ return true;
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. Cc: Petr Mladek <pmladek@suse.com> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: John Ogness <john.ogness@linutronix.de> Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com> Closes: https://lore.kernel.org/lkml/d8bb1ec7-a4c5-43a2-9de0-9643a70b899f@linux.microsoft.com/ Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- kernel/printk/printk_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)