printk: ringbuffer: Fix truncating buffer size min_t cast
Subsystems (2)
PATCH 1/1 printk: ringbuffer: Fix truncating buffer size min_t cast
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.
--- kernel/printk/printk_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/printk/printk_ringbuffer.c +1 -1
@@ -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;
Signature · 1 line
2.34.1
Quoted 6 lines · Kees Cook
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
Quoted 9 lines
Cc Petr Mladek <[email protected]>Cc Sergey Senozhatsky <[email protected]>Cc Steven Rostedt <[email protected]>Cc John Ogness <[email protected]>Reported-by Vijay Balakrishna <[email protected]>Fixes b6cf8b3f3312 ("printk: add lockless ringbuffer")Signed-off-by Kees Cook <[email protected]>
Excellent.
I have verified on v5.10.
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:
Quoted 6 lines · Kees Cook
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
Quoted 9 lines
Cc Petr Mladek <[email protected]>Cc Sergey Senozhatsky <[email protected]>Cc Steven Rostedt <[email protected]>Cc John Ogness <[email protected]>Reported-by Vijay Balakrishna <[email protected]>Fixes b6cf8b3f3312 ("printk: add lockless ringbuffer")Signed-off-by Kees Cook <[email protected]>
Nice find!
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
Quoted 20 lines
--- kernel/printk/printk_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/printk/printk_ringbuffer.c +1
-1@@ -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;Signature · 1 line
2.34.1
Quoted 5 lines · Kees Cook
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
Quoted 6 lines · Kees Cook
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
Quoted 9 lines
Cc Petr Mladek <[email protected]>Cc Sergey Senozhatsky <[email protected]>Cc Steven Rostedt <[email protected]>Cc John Ogness <[email protected]>Reported-by Vijay Balakrishna <[email protected]>Fixes b6cf8b3f3312 ("printk: add lockless ringbuffer")Signed-off-by Kees Cook <[email protected]>
Thanks a lot!
From: Kees Cook
Quoted 8 lines
Sent: 11 August 2023 06:46
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
...
kernel/printk/printk_ringbuffer.c static bool copy_data(struct prb_data_ring *data_ring, · line 1738
Quoted 10 lines
kernel/printk/printk_ringbuffer.c +1
-1@@ -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
Quoted 5 lines
kernel/printk/printk_ringbuffer.c +0
-1memcpy (&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)
Petr Mladek
maintainerQuoted 18 lines · David Laight
From: Kees Cook
Quoted 8 lines
Sent: 11 August 2023 06:46
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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....
Quoted 8 lines
kernel/printk/printk_ringbuffer.c +0
-0@@ -1735,7 +1735,7 @@ static bool copy_data(struct prb_data_ring *data_ring,if (!buf || !buf_size)return true;
Quoted 8 lines
kernel/printk/printk_ringbuffer.c static bool copy_data(struct prb_data_ring *data_ring, · line 1738
Quoted 2 lines
kernel/printk/printk_ringbuffer.c +1
-1- 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/[email protected]/
but it is more about "signed" vs. "unsigned" problem. Maybe it is more complicated that I expected.
Quoted 2 lines
Or put an ack on my patch 3/5 to
minmax.hand thenmin(buf_size, len)will be fine (because both arguments are unsigned).
Do you mean
https://lore.kernel.org/lkml/[email protected]/ ? 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
Quoted 27 lines
Sent: 14 August 2023 13:56
Quoted 24 lines · David Laight
From: Kees Cook
Quoted 8 lines
Sent: 11 August 2023 06:46
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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....
kernel/printk/printk_ringbuffer.c static bool copy_data(struct prb_data_ring *data_ring, · line 1738
Quoted 10 lines
kernel/printk/printk_ringbuffer.c +1
-1@@ -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.
Quoted 4 lines
Quoted 2 lines
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'.
Quoted 5 lines
unsigned int data_size; unsigned int buf_size; u16 len
Quoted 8 lines
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/[email protected]/)
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)).
...
Quoted 3 lines
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)
Quoted 6 lines · Kees Cook
If an output buffer size exceeded
U16_MAX, themin_t(u16, ...)cast incopy_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.
Quoted 6 lines
Cc Petr Mladek <[email protected]>Cc Sergey Senozhatsky <[email protected]>Cc Steven Rostedt <[email protected]>Cc John Ogness <[email protected]>Reported-by Vijay Balakrishna <[email protected]>
checkpatch.pl suggested that "Link:" should be used instead of "Closes:".
Quoted 3 lines
Fixes b6cf8b3f3312 ("printk: add lockless ringbuffer")Signed-off-by Kees Cook <[email protected]>
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