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.micr... 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(-)
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;
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.micr... 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.micr... 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.micr... 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 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.c... 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.c... ? 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.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)
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.micr...
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
linux-stable-mirror@lists.linaro.org
-
David Laight -
Guilherme G. Piccoli -
John Ogness -
Kees Cook -
Petr Mladek -
Sergey Senozhatsky -
Tyler Hicks -
Vijay Balakrishna