Message-ID: <ba445c37-1057-a71c-0ba4-cf8c348ccebb@oracle.com>
Date:   Mon, 25 Jul 2022 08:41:01 +0100
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Fedor Tokarev <ftokarev@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Jiri Olsa <olsajiri@...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 15/07/2022 08:07, Fedor Tokarev wrote:
> On Thu, Jul 14, 2022 at 11:06:22AM +0100, Alan Maguire wrote:
>> 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).
> 
> I'll gladly give it a try.

Thanks! I've sent 

https://lore.kernel.org/bpf/1658734261-4951-1-git-send-email-alan.maguire@oracle.com/

If you could give it a try that would be great; tested at my end
with your fix and all works well. I'd suggest pulling it into a
2-patch series comprised of your fix + the selftest, but since the
fix targets bpf and the tests are new (so would be more like a bpf-next
candidate), not sure if that's the right way to handle this..

If not I can follow up with the test once the fix lands.

Anyway thanks again for finding and fixing this!

Alan 

> 
>> 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?
>>
> Agreed.
> 
>> "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
>>>>>

Powered by blists - more mailing lists