kernel/printk/printk_ringbuffer.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Commit cc3bad11de6e ("printk_ringbuffer: Fix check of valid data
size when blk_lpos overflows") added sanity checking to get_data()
to avoid returning data of illegal sizes (too large or too small).
It uses the helper function data_check_size() for the check.
However, data_check_size() expects the size of the data, not the
size of the data block. get_data() is providing the size of the
data block. This means that if the data size (text_buf_size) is
the maximum legal size:
sizeof(prb_data_block) + text_buf_size == DATA_SIZE(data_ring) / 2
data_check_size() will report failure because it adds
sizeof(prb_data_block) to the provided size. The sanity check in
get_data() is counting the data block header twice. The result is
that the reader fails to read the legal record.
Since get_data() subtracts the data block header size before returning,
move the sanity check to after the subtraction.
Luckily printk() is not vulnerable to this problem because
truncate_msg() limits printk-messages to 1/4 of the ringbuffer.
Indeed, by adjusting the printk_ringbuffer KUnit test, which does not
use printk() and its truncate_msg() check, it is easy to see that the
reader fails and the WARN_ON is triggered.
Fixes: cc3bad11de6e ("printk_ringbuffer: Fix check of valid data size when blk_lpos overflows")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/printk_ringbuffer.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 56c8e3d031f49..613a6524cdd81 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -1302,10 +1302,6 @@ static const char *get_data(struct prb_data_ring *data_ring,
return NULL;
}
- /* Sanity check. Data-less blocks were handled earlier. */
- if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
- return NULL;
-
/* A valid data block will always be aligned to the ID size. */
if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
@@ -1319,6 +1315,10 @@ static const char *get_data(struct prb_data_ring *data_ring,
/* Subtract block ID space from size to reflect data size. */
*data_size -= sizeof(db->id);
+ /* Sanity check. Data-less blocks were handled earlier. */
+ if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
+ return NULL;
+
return &db->data[0];
}
base-commit: 9095f233c0258e9a05e958c7d822eb38681e7a5a
--
2.47.3On Thu 2026-03-19 14:55:39, John Ogness wrote:
> Commit cc3bad11de6e ("printk_ringbuffer: Fix check of valid data
> size when blk_lpos overflows") added sanity checking to get_data()
> to avoid returning data of illegal sizes (too large or too small).
> It uses the helper function data_check_size() for the check.
> However, data_check_size() expects the size of the data, not the
> size of the data block. get_data() is providing the size of the
> data block. This means that if the data size (text_buf_size) is
> the maximum legal size:
>
> sizeof(prb_data_block) + text_buf_size == DATA_SIZE(data_ring) / 2
>
> data_check_size() will report failure because it adds
> sizeof(prb_data_block) to the provided size. The sanity check in
> get_data() is counting the data block header twice. The result is
> that the reader fails to read the legal record.
Great catch!
> Since get_data() subtracts the data block header size before returning,
> move the sanity check to after the subtraction.
>
> Luckily printk() is not vulnerable to this problem because
> truncate_msg() limits printk-messages to 1/4 of the ringbuffer.
> Indeed, by adjusting the printk_ringbuffer KUnit test, which does not
> use printk() and its truncate_msg() check, it is easy to see that the
> reader fails and the WARN_ON is triggered.
Uff ;-)
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1302,10 +1302,6 @@ static const char *get_data(struct prb_data_ring *data_ring,
> return NULL;
> }
>
> - /* Sanity check. Data-less blocks were handled earlier. */
> - if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> - return NULL;
> -
> /* A valid data block will always be aligned to the ID size. */
> if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
> WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
> @@ -1319,6 +1315,10 @@ static const char *get_data(struct prb_data_ring *data_ring,
> /* Subtract block ID space from size to reflect data size. */
> *data_size -= sizeof(db->id);
>
> + /* Sanity check. Data-less blocks were handled earlier. */
> + if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
The check of "!*data_size" is wrong after sizeof(db->id) subtractions.
The question is whether we still need it. As the comment says, the
data-less block were handled earlier. And it seems that data_alloc()
explicitly creates data-less blocks when the given size is 0.
And prb_reserve_in_last() only allows to increase the size.
IMHO, we could remove it. Wrong values will be handled by the above check:
/* A valid data block will always have at least an ID. */
if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
return NULL;
A zero size datablock which is not handled using the explicit
data-less block is not expected after all.
Best Regards,
Petr
> + return NULL;
> +
> return &db->data[0];
> }
>
>
> base-commit: 9095f233c0258e9a05e958c7d822eb38681e7a5a
> --
> 2.47.3
On 2026-03-25, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -1302,10 +1302,6 @@ static const char *get_data(struct prb_data_ring *data_ring,
>> return NULL;
>> }
>>
>> - /* Sanity check. Data-less blocks were handled earlier. */
>> - if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
>> - return NULL;
>> -
>> /* A valid data block will always be aligned to the ID size. */
>> if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
>> WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
>> @@ -1319,6 +1315,10 @@ static const char *get_data(struct prb_data_ring *data_ring,
>> /* Subtract block ID space from size to reflect data size. */
>> *data_size -= sizeof(db->id);
>>
>> + /* Sanity check. Data-less blocks were handled earlier. */
>> + if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
>
> The check of "!*data_size" is wrong after sizeof(db->id) subtractions.
It is not wrong. It is checking something else.
> The question is whether we still need it. As the comment says, the
> data-less block were handled earlier. And it seems that data_alloc()
> explicitly creates data-less blocks when the given size is 0.
> And prb_reserve_in_last() only allows to increase the size.
>
> IMHO, we could remove it. Wrong values will be handled by the above check:
>
> /* A valid data block will always have at least an ID. */
> if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> return NULL;
>
> A zero size datablock which is not handled using the explicit
> data-less block is not expected after all.
Well, the point of the WARN is to catch when things are broken. If for
some reason a data block of textlen 0 exists, it is a bug. I think we
should still catch it. I do not know if there are places in the code
that assume if it is a data block, it must have a textlen > 0. Probably
not, but still, it should not exist.
John
On Wed 2026-03-25 15:22:29, John Ogness wrote:
> On 2026-03-25, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/printk/printk_ringbuffer.c
> >> +++ b/kernel/printk/printk_ringbuffer.c
> >> @@ -1302,10 +1302,6 @@ static const char *get_data(struct prb_data_ring *data_ring,
> >> return NULL;
> >> }
> >>
> >> - /* Sanity check. Data-less blocks were handled earlier. */
> >> - if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> >> - return NULL;
> >> -
> >> /* A valid data block will always be aligned to the ID size. */
> >> if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
> >> WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
> >> @@ -1319,6 +1315,10 @@ static const char *get_data(struct prb_data_ring *data_ring,
> >> /* Subtract block ID space from size to reflect data size. */
> >> *data_size -= sizeof(db->id);
> >>
> >> + /* Sanity check. Data-less blocks were handled earlier. */
> >> + if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> >
> > The check of "!*data_size" is wrong after sizeof(db->id) subtractions.
>
> It is not wrong. It is checking something else.
Good point!
> > The question is whether we still need it. As the comment says, the
> > data-less block were handled earlier. And it seems that data_alloc()
> > explicitly creates data-less blocks when the given size is 0.
> > And prb_reserve_in_last() only allows to increase the size.
> >
> > IMHO, we could remove it. Wrong values will be handled by the above check:
> >
> > /* A valid data block will always have at least an ID. */
> > if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> > return NULL;
Ah, I have missed that this earlier check won't catch it because
it is '<' and not '<='.
> > A zero size datablock which is not handled using the explicit
> > data-less block is not expected after all.
>
> Well, the point of the WARN is to catch when things are broken. If for
> some reason a data block of textlen 0 exists, it is a bug. I think we
> should still catch it. I do not know if there are places in the code
> that assume if it is a data block, it must have a textlen > 0. Probably
> not, but still, it should not exist.
IMHO, there is no function which would expect that a data_block
must have a textlen > 0. It just should not exit. Because zero size
data blocks are handled special way.
So, just to be sure that the new code works as expected.
Note that the moved check:
/* Sanity check. Data-less blocks were handled earlier. */
if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
return NULL;
warns only when data_check_size() fails. It just quietly returns NULL
when *data_size is zero.
If we really want to warn. Then it would make more sense to change "<"
to "<=" in the previous check before the subtraction.
I do not have a strong opinion. But I tend to add the warning because
the zero size is not expected at this stage. Well, we could do
it in a followup patch and keep this on as is.
Best Regards,
Petr
On 2026-03-26, Petr Mladek <pmladek@suse.com> wrote:
> So, just to be sure that the new code works as expected.
> Note that the moved check:
>
> /* Sanity check. Data-less blocks were handled earlier. */
> if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> return NULL;
>
> warns only when data_check_size() fails. It just quietly returns NULL
> when *data_size is zero.
??? The above code warns for !*data_size as well.
> If we really want to warn. Then it would make more sense to change "<"
> to "<=" in the previous check before the subtraction.
Yes, actually I would prefer that. Let me send a v2 where I relocate and
reduce the data_check_size()-WARN and extend the data_size-WARN. So it
is something like this:
diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 56c8e3d031f49..aa4b39e94cfa2 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -1302,23 +1302,26 @@ static const char *get_data(struct prb_data_ring *data_ring,
return NULL;
}
- /* Sanity check. Data-less blocks were handled earlier. */
- if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
- return NULL;
-
/* A valid data block will always be aligned to the ID size. */
if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
return NULL;
}
- /* A valid data block will always have at least an ID. */
- if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
+ /*
+ * A regular data block will always have an ID and at least
+ * 1 byte of data. Data-less blocks were handled earlier.
+ */
+ if (WARN_ON_ONCE(*data_size <= sizeof(db->id)))
return NULL;
/* Subtract block ID space from size to reflect data size. */
*data_size -= sizeof(db->id);
+ /* Sanity check the max size of the regular data block. */
+ if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size)))
+ return NULL;
+
return &db->data[0];
}
John
On Thu 2026-03-26 11:46:48, John Ogness wrote:
> On 2026-03-26, Petr Mladek <pmladek@suse.com> wrote:
> > So, just to be sure that the new code works as expected.
> > Note that the moved check:
> >
> > /* Sanity check. Data-less blocks were handled earlier. */
> > if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> > return NULL;
> >
> > warns only when data_check_size() fails. It just quietly returns NULL
> > when *data_size is zero.
>
> ??? The above code warns for !*data_size as well.
Grr, I wrongly interpreted the brackets.
> > If we really want to warn. Then it would make more sense to change "<"
> > to "<=" in the previous check before the subtraction.
>
> Yes, actually I would prefer that. Let me send a v2 where I relocate and
> reduce the data_check_size()-WARN and extend the data_size-WARN. So it
> is something like this:
>
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 56c8e3d031f49..aa4b39e94cfa2 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1302,23 +1302,26 @@ static const char *get_data(struct prb_data_ring *data_ring,
> return NULL;
> }
>
> - /* Sanity check. Data-less blocks were handled earlier. */
> - if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
> - return NULL;
> -
> /* A valid data block will always be aligned to the ID size. */
> if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
> WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
> return NULL;
> }
>
> - /* A valid data block will always have at least an ID. */
> - if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> + /*
> + * A regular data block will always have an ID and at least
> + * 1 byte of data. Data-less blocks were handled earlier.
> + */
> + if (WARN_ON_ONCE(*data_size <= sizeof(db->id)))
> return NULL;
>
> /* Subtract block ID space from size to reflect data size. */
> *data_size -= sizeof(db->id);
>
> + /* Sanity check the max size of the regular data block. */
> + if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size)))
> + return NULL;
> +
> return &db->data[0];
> }
I like this. Thanks for patience with me.
Best Regards,
Petr
© 2016 - 2026 Red Hat, Inc.