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 <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: John Ogness <[email protected]>
Reported-by: Vijay Balakrishna <[email protected]>
Closes: https://lore.kernel.org/lkml/[email protected]/
Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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 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 <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: John Ogness <[email protected]>
> Reported-by: Vijay Balakrishna <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Excellent.
I have verified on v5.10.
Tested-by: Vijay Balakrishna <[email protected]>
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 <[email protected]> # 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 <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: John Ogness <[email protected]>
> Reported-by: Vijay Balakrishna <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Nice find!
Reviewed-by: Tyler Hicks (Microsoft) <[email protected]>
Tested-by: Tyler Hicks (Microsoft) <[email protected]>
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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: John Ogness <[email protected]>
> Reported-by: Vijay Balakrishna <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Thanks a lot!
Reviewed-by: Sergey Senozhatsky <[email protected]>
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/[email protected]/
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/[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.
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 <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: John Ogness <[email protected]>
> Reported-by: Vijay Balakrishna <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
checkpatch.pl suggested that "Link:" should be used instead of "Closes:".
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Petr Mladek <[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
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/[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)).
...
> 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)