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: <20220715070742.GA165641@laptop>
Date:   Fri, 15 Jul 2022 09:07:42 +0200
From:   Fedor Tokarev <ftokarev@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Jiri Olsa <olsajiri@...il.com>,
        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 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.

> 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
> >>>