Re: [PATCH printk] printk_ringbuffer: Fix get_data() size sanity check

[Petr Mladek <pmladek@suse.com>]

On 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