[PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Kees Cook]

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(-)

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

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Vijay Balakrishna]

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 

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Guilherme G. Piccoli]

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

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Tyler Hicks]

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
> 

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[John Ogness]

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>

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Sergey Senozhatsky]

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>

RE: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[David Laight]

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)

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Petr Mladek]

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

Re: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[Petr Mladek]

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.

RE: [PATCH] printk: ringbuffer: Fix truncating buffer size min_t cast

[David Laight]

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)