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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Race condition in `htab_dtor_ctx_free` leading to a use-after-free of the program BTF.

Pre-existing issues:
- [Critical] The patch fails to fix the use-after-free for BPF_LIST_HEAD and BPF_RB_ROOT fields, which also rely on the program BTF.
--

--- Patch [2]: [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor ---
commit 67cf446d3408eefa2548db164b549cbc9a9e17a2
Author: Yuan Chen <[email protected]>
Subject: bpf: Fix use-after-free of program BTF in mem-alloc destructor

This commit addresses a use-after-free issue of the program BTF in the
bpf_mem_alloc destructor. It holds a reference to the program BTF for the
lifetime of the duplicated record and drops it right before the record is
freed.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0df8db27cd8cc..b8df2bc9a9a00 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -493,10 +493,54 @@ static void htab_pcpu_mem_dtor(void *obj, void *ctx)
>  		bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu));
>  }
>  
> +/*
> + * 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:

[Severity: Critical]
This is a pre-existing issue, but does this switch statement also need to
handle BPF_LIST_HEAD and BPF_RB_ROOT fields?

These field types also rely on the program BTF. When a map containing them
is destroyed, the deferred mem-alloc destructor is scheduled. Because we
don't retain a reference to the program BTF for these types, the program
BTF can be freed when the map drops its reference.

Later, when the deferred destructor calls bpf_obj_free_fields(),
bpf_list_head_free() could dereference the dangling
field->graph_root.value_rec pointer (which points into the freed program
BTF's struct_meta_tab):

kernel/bpf/helpers.c:bpf_list_head_free() {
    ...
    __bpf_obj_drop_impl((char *)pos - field->graph_root.node_offset,
                        field->graph_root.value_rec, false);
    ...
}

> +			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);

[Severity: High]
Can this sequence introduce a race condition leading to a use-after-free of
the program BTF?

This code executes in a preemptible workqueue context
(free_mem_alloc_deferred) without RCU read protection. When
htab_record_prog_btf_ref() is called with get=false, it drops the program
BTF reference. If this is the last reference, the BTF object is scheduled
for RCU destruction.

If the thread is preempted immediately after this call and an RCU grace
period elapses, the BTF memory could be freed. Then the next line calls
btf_record_free(), which iterates over the fields and dereferences the
potentially freed kptr.btf inside btf_is_kernel():

kernel/bpf/syscall.c:btf_record_free() {
    ...
    if (btf_is_kernel(rec->fields[i].kptr.btf))
        btf_put(rec->fields[i].kptr.btf);
    ...
}

>  	kfree(ctx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2