Re: [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor

[bot+bpf-ci]

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0df8db27cd8c..b8df2bc9a9a0 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> +/*
> + * bpf_ma_set_dtor() duplicates the map's btf_record. For kptr fields whose
> + * btf is the program BTF (MEM_ALLOC kptrs, e.g. objects allocated with
> + * bpf_obj_new()/bpf_percpu_obj_new()) btf_record_dup() only borrows the
> + * reference, like btf_parse_fields() did for the map's own record. The
> + * duplicated record is released later from the deferred bpf_mem_alloc
> + * destructor workqueue, by which time the program BTF may already have been
> + * freed (the map dropped its own reference in bpf_map_free()), so reading
> + * field->kptr.btf there would be a use-after-free.
> + *
> + * Hold a reference on non-kernel (program) BTF for the lifetime of the
> + * duplicated record and release it before the record is freed. After the
> + * last btf_put() the object is only destroyed after an RCU grace period, so
> + * btf_record_free() can still safely read the field descriptors.
> + */
> +static void htab_record_prog_btf_ref(struct btf_record *rec, bool get)
> +{
> +	int i;
> +
> +	if (IS_ERR_OR_NULL(rec))
> +		return;
> +
> +	for (i = 0; i < rec->cnt; i++) {
> +		const struct btf_field *field = &rec->fields[i];
> +
> +		switch (field->type) {
> +		case BPF_KPTR_UNREF:
> +		case BPF_KPTR_REF:
> +		case BPF_KPTR_PERCPU:
> +		case BPF_UPTR:
> +			if (field->kptr.btf && !btf_is_kernel(field->kptr.btf)) {
> +				if (get)
> +					btf_get(field->kptr.btf);
> +				else
> +					btf_put(field->kptr.btf);
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  static void htab_dtor_ctx_free(void *ctx)
>  {
>  	struct htab_btf_record *hrec = ctx;
>
> +	htab_record_prog_btf_ref(hrec->record, false);
>  	btf_record_free(hrec->record);
>  	kfree(ctx);
>  }

Does this ordering still have a use-after-free?

The comment states that "After the last btf_put() the object is only
destroyed after an RCU grace period, so btf_record_free() can still safely
read the field descriptors." But in htab_dtor_ctx_free(), the program-BTF
reference is dropped one statement before the record is freed:

    htab_record_prog_btf_ref(hrec->record, false);   /* btf_put() */
    btf_record_free(hrec->record);                   /* reads field->kptr.btf */

htab_record_prog_btf_ref(..., false) ends in btf_put(field->kptr.btf).
Looking at btf_put() in kernel/bpf/btf.c, it only queues the object for RCU
destruction if this was the last reference:

    void btf_put(struct btf *btf)
    {
            if (btf && refcount_dec_and_test(&btf->refcnt)) {
                    btf_free_id(btf);
                    call_rcu(&btf->rcu, btf_free_rcu);
            }
    }

and btf_free_rcu() -> btf_free() -> kfree(btf). The very next statement,
btf_record_free() in kernel/bpf/syscall.c, does for every kptr field:

            if (btf_is_kernel(rec->fields[i].kptr.btf))
                    btf_put(rec->fields[i].kptr.btf);

and btf_is_kernel() is 'return btf->kernel_btf;' - a dereference of the
object whose kfree() was just queued.

In the deferred path this is the last reference, so the drop queues an RCU
free and the following read can land on freed memory. The reachable path is
exactly the scenario described in the commit message:

  bpf_map_free_deferred()
    -> bpf_map_free()
       -> map->ops->map_free (rhtab_map_free / htab_map_free)
          -> bpf_mem_alloc_destroy()
             -> destroy_mem_alloc(), rcu_in_progress != 0
                -> queue_work(system_dfl_wq, &copy->work)
       -> btf_record_free(rec); btf_put(btf);   /* map drops its own BTF ref */
    ... later, on the workqueue ...
  free_mem_alloc_deferred()
    -> free_mem_alloc()
       -> free_mem_alloc_no_barrier()
          -> ma->dtor_ctx_free(ma->dtor_ctx) == htab_dtor_ctx_free

By then the map's reference is gone, so the btf_put() inside
htab_record_prog_btf_ref() drops the last reference and queues
btf_free_rcu. free_mem_alloc_deferred() is a plain, fully preemptible
workqueue worker; there is no rcu_read_lock() anywhere between the work
callback and htab_dtor_ctx_free() or btf_record_free(). Being preempted
outside an RCU read-side critical section is itself a quiescent state, so
on CONFIG_PREEMPT/PREEMPT_RT the grace period can complete and
btf_free_rcu() can run before the worker resumes in btf_record_free().

Could the reference be held until after btf_record_free() has read the
descriptors, or could the pair be wrapped in rcu_read_lock()/
rcu_read_unlock()?

[ ... ]


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