Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer check in two

From: Kent Overstreet
Date: Fri Jun 17 2022 - 12:41:22 EST


On 6/17/22 03:15, Rasmus Villemoes wrote:
On 17/06/2021 16.17, Petr Mladek wrote:
On Tue 2021-06-15 23:49:51, Jia He wrote:
From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>

Before each invocation of vsnprintf(), do_test() memsets the entire
allocated buffer to a sentinel value. That buffer includes leading and
trailing padding which is never included in the buffer area handed to
vsnprintf (spaces merely for clarity):

pad test_buffer pad
**** **************** ****

Then vsnprintf() is invoked with a bufsize argument <=
BUF_SIZE. Suppose bufsize=10, then we'd have e.g.

|pad | test_buffer |pad |
**** pizza0 **** ****** ****
A B C D E

where vsnprintf() was given the area from B to D.

It is obviously a bug for vsnprintf to touch anything between A and B
or between D and E. The former is checked for as one would expect. But
for the latter, we are actually a little stricter in that we check the
area between C and E.

Split that check in two, providing a clearer error message in case it
was a genuine buffer overrun and not merely a write within the
provided buffer, but after the end of the generated string.

So far, no part of the vsnprintf() implementation has had any use for
using the whole buffer as scratch space, but it's not unreasonable to
allow that, as long as the result is properly nul-terminated and the
return value is the right one. However, it is somewhat unusual, and
most %<something> won't need this, so keep the [C,D] check, but make
it easy for a later patch to make that part opt-out for certain tests.

Excellent commit message.

Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
Tested-by: Jia He <justin.he@xxxxxxx>
Signed-off-by: Jia He <justin.he@xxxxxxx>

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Hi Petr

It seems Justin's series got stalled, but I still think this patch makes
sense on its own (especially since another series in flight mucks about
in this area), so can you please pick it up directly?

The lore link for the above is
https://lore.kernel.org/lkml/20210615154952.2744-4-justin.he@xxxxxxx/ ,
while my original submission is at
https://lore.kernel.org/lkml/20210615085044.1923788-1-linux@xxxxxxxxxxxxxxxxxx/

That patch definitely clarifies things, feel free to add my reviewed-by tag