* [PATCH bpf-next] bpf/helpers: bpf_strnstr: Exact match length
@ 2025-08-27 4:27 Rong Tao
2025-08-27 22:35 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Rong Tao @ 2025-08-27 4:27 UTC (permalink / raw)
To: ast, daniel
Cc: rongtao, rtoax, Andrii Nakryiko, Martin KaFai Lau,
Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
open list:BPF [GENERAL] (Safe Dynamic Programs and Tools),
open list
From: Rong Tao <rongtao@cestc.cn>
strnstr should not treat the ending '\0' of s2 as a matching character,
otherwise the parameter 'len' will be meaningless, for example:
1. bpf_strnstr("openat", "open", 4) = -ENOENT
2. bpf_strnstr("openat", "open", 5) = 0
This patch makes (1) return 0, indicating a successful match.
Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
kernel/bpf/helpers.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 401b4932cc49..65bd0050c560 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3681,6 +3681,8 @@ __bpf_kfunc int bpf_strnstr(const char *s1__ign, const char *s2__ign, size_t len
return -ENOENT;
if (c1 != c2)
break;
+ if (j == len - 1)
+ return i;
}
if (j == XATTR_SIZE_MAX)
return -E2BIG;
--
2.51.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH bpf-next] bpf/helpers: bpf_strnstr: Exact match length 2025-08-27 4:27 [PATCH bpf-next] bpf/helpers: bpf_strnstr: Exact match length Rong Tao @ 2025-08-27 22:35 ` Andrii Nakryiko 2025-08-28 2:56 ` Rong Tao 0 siblings, 1 reply; 3+ messages in thread From: Andrii Nakryiko @ 2025-08-27 22:35 UTC (permalink / raw) To: Rong Tao, Viktor Malik Cc: ast, daniel, rongtao, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, open list:BPF [GENERAL] (Safe Dynamic Programs and Tools), open list cc'ing Viktor as well On Tue, Aug 26, 2025 at 9:29 PM Rong Tao <rtoax@foxmail.com> wrote: > > From: Rong Tao <rongtao@cestc.cn> > > strnstr should not treat the ending '\0' of s2 as a matching character, > otherwise the parameter 'len' will be meaningless, for example: > > 1. bpf_strnstr("openat", "open", 4) = -ENOENT > 2. bpf_strnstr("openat", "open", 5) = 0 please add these cases to the tests > > This patch makes (1) return 0, indicating a successful match. > > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > kernel/bpf/helpers.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 401b4932cc49..65bd0050c560 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -3681,6 +3681,8 @@ __bpf_kfunc int bpf_strnstr(const char *s1__ign, const char *s2__ign, size_t len > return -ENOENT; > if (c1 != c2) > break; > + if (j == len - 1) > + return i; But this seems like a wrong fix. The API assumes that s2 is well-formed zero-terminated string, and so we shouldn't just randomly truncate it. Along the examples above, what will happen to bpf_strnstr("openat", "open", 3)? With your fix it will return success, right? But it shouldn't, IMO, because "open" wasn't really found in the first 3 characters of, effectively, "ope". We should also test bpf_strnstr("", "", 0)... ;) So maybe something like this (but I haven't really tested it): diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 401b4932cc49..ced7132980fe 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -3672,10 +3672,12 @@ __bpf_kfunc int bpf_strnstr(const char *s1__ign, const char *s2__ign, size_t len guard(pagefault)(); for (i = 0; i < XATTR_SIZE_MAX; i++) { - for (j = 0; i + j < len && j < XATTR_SIZE_MAX; j++) { + for (j = 0; i + j <= len && j < XATTR_SIZE_MAX; j++) { __get_kernel_nofault(&c2, s2__ign + j, char, err_out); if (c2 == '\0') return i; + if (i + j == len) + break; __get_kernel_nofault(&c1, s1__ign + j, char, err_out); if (c1 == '\0') return -ENOENT; pw-bot: cr > } > if (j == XATTR_SIZE_MAX) > return -E2BIG; > -- > 2.51.0 > > ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] bpf/helpers: bpf_strnstr: Exact match length 2025-08-27 22:35 ` Andrii Nakryiko @ 2025-08-28 2:56 ` Rong Tao 0 siblings, 0 replies; 3+ messages in thread From: Rong Tao @ 2025-08-28 2:56 UTC (permalink / raw) To: Andrii Nakryiko, Viktor Malik Cc: ast, daniel, rongtao, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, open list:BPF [GENERAL] (Safe Dynamic Programs and Tools), open list On 8/28/25 06:35, Andrii Nakryiko wrote: > cc'ing Viktor as well > > On Tue, Aug 26, 2025 at 9:29 PM Rong Tao <rtoax@foxmail.com> wrote: >> From: Rong Tao <rongtao@cestc.cn> >> >> strnstr should not treat the ending '\0' of s2 as a matching character, >> otherwise the parameter 'len' will be meaningless, for example: >> >> 1. bpf_strnstr("openat", "open", 4) = -ENOENT >> 2. bpf_strnstr("openat", "open", 5) = 0 > please add these cases to the tests > >> This patch makes (1) return 0, indicating a successful match. >> >> Signed-off-by: Rong Tao <rongtao@cestc.cn> >> --- >> kernel/bpf/helpers.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >> index 401b4932cc49..65bd0050c560 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -3681,6 +3681,8 @@ __bpf_kfunc int bpf_strnstr(const char *s1__ign, const char *s2__ign, size_t len >> return -ENOENT; >> if (c1 != c2) >> break; >> + if (j == len - 1) >> + return i; > But this seems like a wrong fix. The API assumes that s2 is Thanks a lot Andrii Nakryiko, I just submit V2, please review. > well-formed zero-terminated string, and so we shouldn't just randomly > truncate it. Along the examples above, what will happen to > bpf_strnstr("openat", "open", 3)? With your fix it will return > success, right? But it shouldn't, IMO, because "open" wasn't really > found in the first 3 characters of, effectively, "ope". > > We should also test bpf_strnstr("", "", 0)... ;) > > > So maybe something like this (but I haven't really tested it): > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 401b4932cc49..ced7132980fe 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -3672,10 +3672,12 @@ __bpf_kfunc int bpf_strnstr(const char > *s1__ign, const char *s2__ign, size_t len > > guard(pagefault)(); > for (i = 0; i < XATTR_SIZE_MAX; i++) { > - for (j = 0; i + j < len && j < XATTR_SIZE_MAX; j++) { > + for (j = 0; i + j <= len && j < XATTR_SIZE_MAX; j++) { > __get_kernel_nofault(&c2, s2__ign + j, char, err_out); > if (c2 == '\0') > return i; > + if (i + j == len) > + break; It's works fine, thanks. > __get_kernel_nofault(&c1, s1__ign + j, char, err_out); > if (c1 == '\0') > return -ENOENT; > > > pw-bot: cr > > >> } >> if (j == XATTR_SIZE_MAX) >> return -E2BIG; >> -- >> 2.51.0 >> >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-28 3:01 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-27 4:27 [PATCH bpf-next] bpf/helpers: bpf_strnstr: Exact match length Rong Tao 2025-08-27 22:35 ` Andrii Nakryiko 2025-08-28 2:56 ` Rong Tao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).