* [PATCH] bpf: btf: Fix vsnprintf return value check @ 2022-07-11 21:13 Fedor Tokarev 2022-07-11 21:42 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Fedor Tokarev @ 2022-07-11 21:13 UTC (permalink / raw) To: Martin KaFai Lau; +Cc: bpf, linux-kernel, ftokarev 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. Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: btf: Fix vsnprintf return value check 2022-07-11 21:13 [PATCH] bpf: btf: Fix vsnprintf return value check Fedor Tokarev @ 2022-07-11 21:42 ` Jiri Olsa 2022-07-13 18:40 ` Andrii Nakryiko 0 siblings, 1 reply; 6+ messages in thread From: Jiri Olsa @ 2022-07-11 21:42 UTC (permalink / raw) To: Fedor Tokarev; +Cc: Martin KaFai Lau, bpf, linux-kernel 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 ? jirka > > Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: btf: Fix vsnprintf return value check 2022-07-11 21:42 ` Jiri Olsa @ 2022-07-13 18:40 ` Andrii Nakryiko 2022-07-14 10:06 ` Alan Maguire 0 siblings, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2022-07-13 18:40 UTC (permalink / raw) To: Jiri Olsa, Alan Maguire; +Cc: Fedor Tokarev, Martin KaFai Lau, bpf, open list On Mon, Jul 11, 2022 at 2:45 PM Jiri Olsa <olsajiri@gmail.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. > > jirka > > > > > Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: btf: Fix vsnprintf return value check 2022-07-13 18:40 ` Andrii Nakryiko @ 2022-07-14 10:06 ` Alan Maguire 2022-07-15 7:07 ` Fedor Tokarev 0 siblings, 1 reply; 6+ messages in thread From: Alan Maguire @ 2022-07-14 10:06 UTC (permalink / raw) To: Andrii Nakryiko, Jiri Olsa Cc: Fedor Tokarev, Martin KaFai Lau, bpf, open list On 13/07/2022 19:40, Andrii Nakryiko wrote: > On Mon, Jul 11, 2022 at 2:45 PM Jiri Olsa <olsajiri@gmail.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@oracle.com> >> >> jirka >> >>> >>> Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: btf: Fix vsnprintf return value check 2022-07-14 10:06 ` Alan Maguire @ 2022-07-15 7:07 ` Fedor Tokarev 2022-07-25 7:41 ` Alan Maguire 0 siblings, 1 reply; 6+ messages in thread From: Fedor Tokarev @ 2022-07-15 7:07 UTC (permalink / raw) To: Alan Maguire Cc: Andrii Nakryiko, Jiri Olsa, Fedor Tokarev, Martin KaFai Lau, bpf, open list 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@gmail.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@oracle.com> > > >> > >> jirka > >> > >>> > >>> Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 > >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: btf: Fix vsnprintf return value check 2022-07-15 7:07 ` Fedor Tokarev @ 2022-07-25 7:41 ` Alan Maguire 0 siblings, 0 replies; 6+ messages in thread From: Alan Maguire @ 2022-07-25 7:41 UTC (permalink / raw) To: Fedor Tokarev Cc: Andrii Nakryiko, Jiri Olsa, Martin KaFai Lau, bpf, open list 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@gmail.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@oracle.com> >> >>>> >>>> jirka >>>> >>>>> >>>>> Signed-off-by: Fedor Tokarev <ftokarev@gmail.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 >>>>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-25 7:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-11 21:13 [PATCH] bpf: btf: Fix vsnprintf return value check Fedor Tokarev 2022-07-11 21:42 ` Jiri Olsa 2022-07-13 18:40 ` Andrii Nakryiko 2022-07-14 10:06 ` Alan Maguire 2022-07-15 7:07 ` Fedor Tokarev 2022-07-25 7:41 ` Alan Maguire
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox