[PATCH v2 bpf-next] bpf: Fix a potential use-after-free

[PATCH v2 bpf-next] bpf: Fix a potential use-after-free

[Anton Protopopov]

Refcounting in the check_pseudo_btf_id() function is incorrect:
the __check_pseudo_btf_id() function might get called with a zero
refcounted btf. Fix this, and patch related code accordingly.

Reported-by: syzbot+5a0f1995634f7c1dadbf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=5a0f1995634f7c1dadbf
Fixes: 76145f725532 ("bpf: Refactor check_pseudo_btf_id")
Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
---
 kernel/bpf/verifier.c | 51 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index edf5342b982f..24de90357450 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21333,29 +21333,28 @@ static int find_btf_percpu_datasec(struct btf *btf)
 }
 
 /*
- * Add btf to the used_btfs array and return the index. (If the btf was
- * already added, then just return the index.) Upon successful insertion
- * increase btf refcnt, and, if present, also refcount the corresponding
- * kernel module.
+ * Add btf to the used_btfs array and return the index. If needed, refcount the
+ * corresponding kernel module. To simplify caller's error checking in case of
+ * error or if btf was added before, decrease the btf refcount.
  */
 static int __add_used_btf(struct bpf_verifier_env *env, struct btf *btf)
 {
 	struct btf_mod_pair *btf_mod;
+	int ret = 0;
 	int i;
 
 	/* check whether we recorded this BTF (and maybe module) already */
 	for (i = 0; i < env->used_btf_cnt; i++)
 		if (env->used_btfs[i].btf == btf)
-			return i;
+			goto ret_put;
 
 	if (env->used_btf_cnt >= MAX_USED_BTFS) {
 		verbose(env, "The total number of btfs per program has reached the limit of %u\n",
 			MAX_USED_BTFS);
-		return -E2BIG;
+		ret = -E2BIG;
+		goto ret_put;
 	}
 
-	btf_get(btf);
-
 	btf_mod = &env->used_btfs[env->used_btf_cnt];
 	btf_mod->btf = btf;
 	btf_mod->module = NULL;
@@ -21364,12 +21363,18 @@ static int __add_used_btf(struct bpf_verifier_env *env, struct btf *btf)
 	if (btf_is_module(btf)) {
 		btf_mod->module = btf_try_get_module(btf);
 		if (!btf_mod->module) {
-			btf_put(btf);
-			return -ENXIO;
+			ret = -ENXIO;
+			goto ret_put;
 		}
 	}
 
-	return env->used_btf_cnt++;
+	env->used_btf_cnt++;
+	return 0;
+
+ret_put:
+	/* Either error or this BTF was already added */
+	btf_put(btf);
+	return ret;
 }
 
 /* replace pseudo btf_id with kernel symbol address */
@@ -21466,9 +21471,7 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 
 	btf_fd = insn[1].imm;
 	if (btf_fd) {
-		CLASS(fd, f)(btf_fd);
-
-		btf = __btf_get_by_fd(f);
+		btf = btf_get_by_fd(btf_fd);
 		if (IS_ERR(btf)) {
 			verbose(env, "invalid module BTF object FD specified.\n");
 			return -EINVAL;
@@ -21478,17 +21481,17 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
 			verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
 			return -EINVAL;
 		}
+		btf_get(btf_vmlinux);
 		btf = btf_vmlinux;
 	}
 
 	err = __check_pseudo_btf_id(env, insn, aux, btf);
-	if (err)
+	if (err) {
+		btf_put(btf);
 		return err;
+	}
 
-	err = __add_used_btf(env, btf);
-	if (err < 0)
-		return err;
-	return 0;
+	return __add_used_btf(env, btf);
 }
 
 static bool is_tracing_prog_type(enum bpf_prog_type type)
@@ -25368,13 +25371,9 @@ static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
 		return 0;
 	}
 
-	btf = __btf_get_by_fd(f);
-	if (!IS_ERR(btf)) {
-		err = __add_used_btf(env, btf);
-		if (err < 0)
-			return err;
-		return 0;
-	}
+	btf = btf_get_by_fd(fd);
+	if (!IS_ERR(btf))
+		return __add_used_btf(env, btf);
 
 	verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
 	return PTR_ERR(map);
-- 
2.34.1

Re: [PATCH v2 bpf-next] bpf: Fix a potential use-after-free

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index edf5342b982f..24de90357450 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -21333,29 +21333,28 @@ static int find_btf_percpu_datasec(struct btf *btf)
>  }
>
>  /*
> - * Add btf to the used_btfs array and return the index. (If the btf was
> - * already added, then just return the index.) Upon successful insertion
> - * increase btf refcnt, and, if present, also refcount the corresponding
> - * kernel module.
> + * Add btf to the used_btfs array and return the index. If needed, refcount the
> + * corresponding kernel module. To simplify caller's error checking in case of
> + * error or if btf was added before, decrease the btf refcount.
>   */

The updated comment says "return the index" but the function now returns 0
on success rather than the actual index. The old code returned
env->used_btf_cnt++ (the index), but the new code returns 0:

    env->used_btf_cnt++;
    return 0;

Should the comment say "return 0 on success" instead of "return the index"?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/21825693233

AI-authorship-score: low
AI-authorship-explanation: The commit has standard kernel formatting, addresses a real syzbot-reported bug with proper attribution, and shows iterative development through review feedback, all consistent with human authorship.
issues-found: 1
issue-severity-score: low
issue-severity-explanation: Minor documentation issue where a comment says 'return the index' but the function returns 0 on success; no runtime impact as no callers use the return value as an index.

Re: [PATCH v2 bpf-next] bpf: Fix a potential use-after-free

[Anton Protopopov]

On 26/02/09 01:06PM, bot+bpf-ci@kernel.org wrote:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index edf5342b982f..24de90357450 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -21333,29 +21333,28 @@ static int find_btf_percpu_datasec(struct btf *btf)
> >  }
> >
> >  /*
> > - * Add btf to the used_btfs array and return the index. (If the btf was
> > - * already added, then just return the index.) Upon successful insertion
> > - * increase btf refcnt, and, if present, also refcount the corresponding
> > - * kernel module.
> > + * Add btf to the used_btfs array and return the index. If needed, refcount the
> > + * corresponding kernel module. To simplify caller's error checking in case of
> > + * error or if btf was added before, decrease the btf refcount.
> >   */
> 
> The updated comment says "return the index" but the function now returns 0
> on success rather than the actual index. The old code returned
> env->used_btf_cnt++ (the index), but the new code returns 0:
> 
>     env->used_btf_cnt++;
>     return 0;
> 
> Should the comment say "return 0 on success" instead of "return the index"?

Yeah, thanks

> [ ... ]
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/21825693233
> 
> AI-authorship-score: low
> AI-authorship-explanation: The commit has standard kernel formatting, addresses a real syzbot-reported bug with proper attribution, and shows iterative development through review feedback, all consistent with human authorship.
> issues-found: 1
> issue-severity-score: low
> issue-severity-explanation: Minor documentation issue where a comment says 'return the index' but the function returns 0 on success; no runtime impact as no callers use the return value as an index.