linux-kernel - Re: [PATCH] bpf: btf: Fix vsnprintf return value check

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ef44abe0-81fc-9c97-a4e4-2b3ba19cd84f@oracle.com>
Date:   Thu, 14 Jul 2022 11:06:22 +0100
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Jiri Olsa <olsajiri@...il.com>
Cc:     Fedor Tokarev <ftokarev@...il.com>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        bpf <bpf@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bpf: btf: Fix vsnprintf return value check

On 13/07/2022 19:40, Andrii Nakryiko wrote:
> On Mon, Jul 11, 2022 at 2:45 PM Jiri Olsa <olsajiri@...il.com> wrote:
>>
>> On Mon, Jul 11, 2022 at 11:13:17PM +0200, Fedor Tokarev wrote:
>>> vsnprintf returns the number of characters which would have been written if
>>> enough space had been available, excluding the terminating null byte. Thus,
>>> the return value of 'len_left' means that the last character has been
>>> dropped.
>>
>> should we have test for this in progs/test_snprintf.c ?
> 
> It might be too annoying to set up such test, and given the fix is
> pretty trivial IMO it's ok without extra test. But cc Alan for ack.
> Alan, please take a look as well.
> 

I can follow up with a test, it should be okay I think (we can use
the "don't show types" flag and tryp to print "10" with a 2-byte len or
similar).

In terms of the fix, it looks good, but given that the code is tricky, 
it might be good to expand a bit on the explanation. Something like the below?

"When using btf_type_snprintf_show(), the user passes in a "len" value, and
we use it to initialize ssnprintf.len_left, indicating how much space
remains for the string representation, including the null byte, so "len - 1" 
bytes are actually available for the actual string data, leaving one for 
the terminating null byte.

In btf_snprintf_show() - which is passed the ssnprintf data as an argument -
vsnprintf() returns the len that would have been written, and this _excludes_ 
the null terminator. But we want to handle cases where the length of the string
to be written (excluding the null terminator) exactly matches the original len 
value we passed in (len == len_left) in the same way was we do other
overflow cases (len > len_left)."

Acked-by: Alan Maguire <alan.maguire@...cle.com>

>>
>> jirka
>>
>>>
>>> Signed-off-by: Fedor Tokarev <ftokarev@...il.com>
>>> ---
>>>  kernel/bpf/btf.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index eb12d4f705cc..a9c1c98017d4 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -6519,7 +6519,7 @@ static void btf_snprintf_show(struct btf_show *show, const char *fmt,
>>>       if (len < 0) {
>>>               ssnprintf->len_left = 0;
>>>               ssnprintf->len = len;
>>> -     } else if (len > ssnprintf->len_left) {
>>> +     } else if (len >= ssnprintf->len_left) {
>>>               /* no space, drive on to get length we would have written */
>>>               ssnprintf->len_left = 0;
>>>               ssnprintf->len += len;
>>> --
>>> 2.25.1
>>>