* [PATCH bpf-next] bpf: Silence Coverity warning for find_kfunc_desc_btf
@ 2021-10-08 17:07 Kumar Kartikeya Dwivedi
2021-10-08 20:39 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-08 17:07 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
The helper function returns a pointer that in the failure case encodes
an error in the struct btf pointer. The current code lead to Coverity
warning about the use of the invalid pointer:
*** CID 1507963: Memory - illegal accesses (USE_AFTER_FREE)
/kernel/bpf/verifier.c: 1788 in find_kfunc_desc_btf()
1782 return ERR_PTR(-EINVAL);
1783 }
1784
1785 kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp);
1786 if (IS_ERR_OR_NULL(kfunc_btf)) {
1787 verbose(env, "cannot find module BTF for func_id %u\n", func_id);
>>> CID 1507963: Memory - illegal accesses (USE_AFTER_FREE)
>>> Using freed pointer "kfunc_btf".
1788 return kfunc_btf ?: ERR_PTR(-ENOENT);
1789 }
1790 return kfunc_btf;
1791 }
1792 return btf_vmlinux ?: ERR_PTR(-ENOENT);
1793 }
Daniel suggested the use of ERR_CAST so that the intended use is clear
to Coverity, but on closer look it seems that we never return NULL from
the helper, hence it can just be switched to checking for IS_ERR and
returning the pointer, similar to the cases elsewhere in the kernel.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/verifier.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 20900a1bac12..2551b6be8d42 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1783,10 +1783,8 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env,
}
kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp);
- if (IS_ERR_OR_NULL(kfunc_btf)) {
+ if (IS_ERR(kfunc_btf))
verbose(env, "cannot find module BTF for func_id %u\n", func_id);
- return kfunc_btf ?: ERR_PTR(-ENOENT);
- }
return kfunc_btf;
}
return btf_vmlinux ?: ERR_PTR(-ENOENT);
--
2.33.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next] bpf: Silence Coverity warning for find_kfunc_desc_btf 2021-10-08 17:07 [PATCH bpf-next] bpf: Silence Coverity warning for find_kfunc_desc_btf Kumar Kartikeya Dwivedi @ 2021-10-08 20:39 ` Andrii Nakryiko 2021-10-08 22:27 ` [PATCH bpf-next v2] " Kumar Kartikeya Dwivedi 2021-10-09 4:09 ` [PATCH bpf-next v3] " Kumar Kartikeya Dwivedi 0 siblings, 2 replies; 5+ messages in thread From: Andrii Nakryiko @ 2021-10-08 20:39 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko On Fri, Oct 8, 2021 at 10:07 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > The helper function returns a pointer that in the failure case encodes > an error in the struct btf pointer. The current code lead to Coverity > warning about the use of the invalid pointer: > > *** CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) > /kernel/bpf/verifier.c: 1788 in find_kfunc_desc_btf() > 1782 return ERR_PTR(-EINVAL); > 1783 } > 1784 > 1785 kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); > 1786 if (IS_ERR_OR_NULL(kfunc_btf)) { > 1787 verbose(env, "cannot find module BTF for func_id %u\n", func_id); > >>> CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) > >>> Using freed pointer "kfunc_btf". > 1788 return kfunc_btf ?: ERR_PTR(-ENOENT); > 1789 } > 1790 return kfunc_btf; > 1791 } > 1792 return btf_vmlinux ?: ERR_PTR(-ENOENT); > 1793 } > > Daniel suggested the use of ERR_CAST so that the intended use is clear > to Coverity, but on closer look it seems that we never return NULL from > the helper, hence it can just be switched to checking for IS_ERR and > returning the pointer, similar to the cases elsewhere in the kernel. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 20900a1bac12..2551b6be8d42 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -1783,10 +1783,8 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, > } > > kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); Seems like __find_kfunc_desc_btf() already logs most of possible reasons for the failure, with possibly btf_get_by_fd failure being the biggest user-visible omission. If you complete __find_kfunc_desc_btf logging and just pass through its result here without extra check, wouldn't it work? > - if (IS_ERR_OR_NULL(kfunc_btf)) { > + if (IS_ERR(kfunc_btf)) > verbose(env, "cannot find module BTF for func_id %u\n", func_id); > - return kfunc_btf ?: ERR_PTR(-ENOENT); > - } > return kfunc_btf; > } > return btf_vmlinux ?: ERR_PTR(-ENOENT); > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf-next v2] bpf: Silence Coverity warning for find_kfunc_desc_btf 2021-10-08 20:39 ` Andrii Nakryiko @ 2021-10-08 22:27 ` Kumar Kartikeya Dwivedi 2021-10-09 4:09 ` [PATCH bpf-next v3] " Kumar Kartikeya Dwivedi 1 sibling, 0 replies; 5+ messages in thread From: Kumar Kartikeya Dwivedi @ 2021-10-08 22:27 UTC (permalink / raw) To: bpf; +Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann The helper function returns a pointer that in the failure case encodes an error in the struct btf pointer. The current code lead to Coverity warning about the use of the invalid pointer: *** CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) /kernel/bpf/verifier.c: 1788 in find_kfunc_desc_btf() 1782 return ERR_PTR(-EINVAL); 1783 } 1784 1785 kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); 1786 if (IS_ERR_OR_NULL(kfunc_btf)) { 1787 verbose(env, "cannot find module BTF for func_id %u\n", func_id); >>> CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) >>> Using freed pointer "kfunc_btf". 1788 return kfunc_btf ?: ERR_PTR(-ENOENT); 1789 } 1790 return kfunc_btf; 1791 } 1792 return btf_vmlinux ?: ERR_PTR(-ENOENT); 1793 } Daniel suggested the use of ERR_CAST so that the intended use is clear to Coverity, but on closer look it seems that we never return NULL from the helper. Andrii noted that since __find_kfunc_desc_btf already logs errors for all cases except btf_get_by_fd, it is much easier to add logging for that and remove the IS_ERR check altogether, returning directly from it. Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- v1->v2 * Remove error check, log btf_get_by_fd failure (Andrii) --- kernel/bpf/verifier.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 20900a1bac12..7180f0bc347e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1727,8 +1727,10 @@ static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env, return ERR_PTR(-EFAULT); btf = btf_get_by_fd(btf_fd); - if (IS_ERR(btf)) + if (IS_ERR(btf)) { + verbose(env, "invalid module BTF fd specified\n"); return btf; + } if (!btf_is_module(btf)) { verbose(env, "BTF fd for kfunc is not a module BTF\n"); @@ -1782,12 +1784,7 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, return ERR_PTR(-EINVAL); } - kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); - if (IS_ERR_OR_NULL(kfunc_btf)) { - verbose(env, "cannot find module BTF for func_id %u\n", func_id); - return kfunc_btf ?: ERR_PTR(-ENOENT); - } - return kfunc_btf; + return __find_kfunc_desc_btf(env, offset, btf_modp); } return btf_vmlinux ?: ERR_PTR(-ENOENT); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf-next v3] bpf: Silence Coverity warning for find_kfunc_desc_btf 2021-10-08 20:39 ` Andrii Nakryiko 2021-10-08 22:27 ` [PATCH bpf-next v2] " Kumar Kartikeya Dwivedi @ 2021-10-09 4:09 ` Kumar Kartikeya Dwivedi 2021-10-20 16:57 ` Andrii Nakryiko 1 sibling, 1 reply; 5+ messages in thread From: Kumar Kartikeya Dwivedi @ 2021-10-09 4:09 UTC (permalink / raw) To: bpf; +Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann The helper function returns a pointer that in the failure case encodes an error in the struct btf pointer. The current code lead to Coverity warning about the use of the invalid pointer: *** CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) /kernel/bpf/verifier.c: 1788 in find_kfunc_desc_btf() 1782 return ERR_PTR(-EINVAL); 1783 } 1784 1785 kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); 1786 if (IS_ERR_OR_NULL(kfunc_btf)) { 1787 verbose(env, "cannot find module BTF for func_id %u\n", func_id); >>> CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) >>> Using freed pointer "kfunc_btf". 1788 return kfunc_btf ?: ERR_PTR(-ENOENT); 1789 } 1790 return kfunc_btf; 1791 } 1792 return btf_vmlinux ?: ERR_PTR(-ENOENT); 1793 } Daniel suggested the use of ERR_CAST so that the intended use is clear to Coverity, but on closer look it seems that we never return NULL from the helper. Andrii noted that since __find_kfunc_desc_btf already logs errors for all cases except btf_get_by_fd, it is much easier to add logging for that and remove the IS_ERR check altogether, returning directly from it. Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- v2->v3 * Remove unused variable (Kernel Test Robot) v1->v2 * Remove error check, log btf_get_by_fd failure (Andrii) --- kernel/bpf/verifier.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 20900a1bac12..21cdff35a2f9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1727,8 +1727,10 @@ static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env, return ERR_PTR(-EFAULT); btf = btf_get_by_fd(btf_fd); - if (IS_ERR(btf)) + if (IS_ERR(btf)) { + verbose(env, "invalid module BTF fd specified\n"); return btf; + } if (!btf_is_module(btf)) { verbose(env, "BTF fd for kfunc is not a module BTF\n"); @@ -1771,8 +1773,6 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, u32 func_id, s16 offset, struct module **btf_modp) { - struct btf *kfunc_btf; - if (offset) { if (offset < 0) { /* In the future, this can be allowed to increase limit @@ -1782,12 +1782,7 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, return ERR_PTR(-EINVAL); } - kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); - if (IS_ERR_OR_NULL(kfunc_btf)) { - verbose(env, "cannot find module BTF for func_id %u\n", func_id); - return kfunc_btf ?: ERR_PTR(-ENOENT); - } - return kfunc_btf; + return __find_kfunc_desc_btf(env, offset, btf_modp); } return btf_vmlinux ?: ERR_PTR(-ENOENT); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v3] bpf: Silence Coverity warning for find_kfunc_desc_btf 2021-10-09 4:09 ` [PATCH bpf-next v3] " Kumar Kartikeya Dwivedi @ 2021-10-20 16:57 ` Andrii Nakryiko 0 siblings, 0 replies; 5+ messages in thread From: Andrii Nakryiko @ 2021-10-20 16:57 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi Cc: bpf, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann On Fri, Oct 8, 2021 at 9:09 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > The helper function returns a pointer that in the failure case encodes > an error in the struct btf pointer. The current code lead to Coverity > warning about the use of the invalid pointer: > > *** CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) > /kernel/bpf/verifier.c: 1788 in find_kfunc_desc_btf() > 1782 return ERR_PTR(-EINVAL); > 1783 } > 1784 > 1785 kfunc_btf = __find_kfunc_desc_btf(env, offset, btf_modp); > 1786 if (IS_ERR_OR_NULL(kfunc_btf)) { > 1787 verbose(env, "cannot find module BTF for func_id %u\n", func_id); > >>> CID 1507963: Memory - illegal accesses (USE_AFTER_FREE) > >>> Using freed pointer "kfunc_btf". > 1788 return kfunc_btf ?: ERR_PTR(-ENOENT); > 1789 } > 1790 return kfunc_btf; > 1791 } > 1792 return btf_vmlinux ?: ERR_PTR(-ENOENT); > 1793 } > > Daniel suggested the use of ERR_CAST so that the intended use is clear > to Coverity, but on closer look it seems that we never return NULL from > the helper. Andrii noted that since __find_kfunc_desc_btf already logs > errors for all cases except btf_get_by_fd, it is much easier to add > logging for that and remove the IS_ERR check altogether, returning > directly from it. > > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > v2->v3 > * Remove unused variable (Kernel Test Robot) > v1->v2 > * Remove error check, log btf_get_by_fd failure (Andrii) > --- Patch bot missed this one. Applied yesterday to bpf-next. Thanks. [...] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-20 16:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-08 17:07 [PATCH bpf-next] bpf: Silence Coverity warning for find_kfunc_desc_btf Kumar Kartikeya Dwivedi 2021-10-08 20:39 ` Andrii Nakryiko 2021-10-08 22:27 ` [PATCH bpf-next v2] " Kumar Kartikeya Dwivedi 2021-10-09 4:09 ` [PATCH bpf-next v3] " Kumar Kartikeya Dwivedi 2021-10-20 16:57 ` Andrii Nakryiko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.