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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jun 2022 09:15:53 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Petr Mladek <pmladek@...e.com>, Jia He <justin.he@....com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kernel@...r.kernel.org,
        Kent Overstreet <kent.overstreet@...il.com>
Subject: Re: [PATCH RFCv4 3/4] lib/test_printf.c: split write-beyond-buffer
 check in two

On 17/06/2021 16.17, Petr Mladek wrote:
> On Tue 2021-06-15 23:49:51, Jia He wrote:
>> From: Rasmus Villemoes <linux@...musvillemoes.dk>
>>
>> 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@...musvillemoes.dk>
>> Tested-by: Jia He <justin.he@....com>
>> Signed-off-by: Jia He <justin.he@....com>
> 
> Reviewed-by: Petr Mladek <pmladek@...e.com>

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@arm.com/ ,
while my original submission is at
https://lore.kernel.org/lkml/20210615085044.1923788-1-linux@rasmusvillemoes.dk/
.

Rasmus