Re: [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing inheritance of `parent_id` in commuted pointer arithmetic (scalar += pointer) allows derived pointers like dynptr slices to escape reference invalidation, leading to a Use-After-Free.
- [High] An early return for untrusted pointers during commuted arithmetic leaves the verifier state desynchronized from the runtime state, allowing arbitrary kernel memory read/write.
--

--- Patch [1]: [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic ---
commit e1439595e89ba75141d297320a4a5530c2e2c8ce
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>

bpf: Preserve stack frame number for commuted arithmetic

When scalar += pointer is handled in adjust_ptr_min_max_vals(), the
destination register inherits the pointer type and id from the source
pointer. For PTR_TO_STACK, the inherited pointer state also has to carry
the stack frame number.

Without the frame number copy, a stack pointer derived inside a callee
through scalar += fp can be recorded as pointing to frame 0. Stack reads
and writes through that register can then update or consult the caller
frame while the actual instruction uses the callee frame.

Copy the frame number when PTR_TO_STACK state is inherited by the
commuted arithmetic form.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 52be0a118cce0..58017141d52b7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13796,11 +13796,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> -	/* In case of 'scalar += pointer', dst_reg inherits pointer type and id.
> -	 * The id may be overwritten later if we create a new variable offset.
> +	/* In case of 'scalar += pointer', dst_reg inherits pointer type, id,
> +	 * and for stack pointers also the frame number. The id may be overwritten
> +	 * later if we create a new variable offset.
>  	 */
>  	dst_reg->type = ptr_reg->type;
>  	dst_reg->id = ptr_reg->id;
> +	if (base_type(ptr_reg->type) == PTR_TO_STACK)
> +		dst_reg->frameno = ptr_reg->frameno;

[Severity: High]
This isn't a bug introduced by this patch, but should we also copy parent_id
here?

During commuted arithmetic (scalar += pointer), the verifier transforms
the scalar destination register into a pointer but fails to copy the
parent_id from the source pointer, leaving it as 0.

If a BPF program obtains a dynptr slice (PTR_TO_MEM with parent_id tied
to the dynptr), performs commuted arithmetic (scalar += slice_ptr), and
then releases the dynptr (e.g., bpf_ringbuf_discard_dynptr), will
release_reference() skip invalidating the commuted pointer?

release_reference() looks for matching parent_id:

kernel/bpf/verifier.c:release_reference() {
    ...
		bpf_for_each_reg_in_vstate_mask(vstate, state, reg, stack, mask, ({
			if (reg->id != id && reg->parent_id != id)
				continue;
    ...
}

Because the commuted pointer has a parent_id of 0, could it escape
invalidation and be dereferenced later to read or write freed memory?

[Severity: High]
This is a pre-existing issue, but does the early return for untrusted
pointers in adjust_ptr_min_max_vals() handle scalar += untrusted_ptr
correctly?

Earlier in adjust_ptr_min_max_vals(), we have:

kernel/bpf/verifier.c:adjust_ptr_min_max_vals() {
    ...
	/*
	 * Accesses to untrusted PTR_TO_MEM are done through probe
	 * instructions, hence no need to track offsets.
	 */
	if (base_type(ptr_reg->type) == PTR_TO_MEM && (ptr_reg->type & PTR_UNTRUSTED))
		return 0;
    ...
}

If a BPF program executes scalar += untrusted_ptr, check_alu_op() invokes
adjust_ptr_min_max_vals() with the scalar as dst_reg and the untrusted
pointer as src_reg.

If the early return succeeds, it returns 0 without modifying the
destination register's type. Does this leave the verifier tracking dst_reg
as a scalar, while at runtime it becomes a pointer?

Could a program then use this register as a forged index into an array or
map, bypassing bounds checks?

>  
>  	if (!check_reg_sane_offset_scalar(env, off_reg, ptr_reg->type) ||
>  	    !check_reg_sane_offset_ptr(env, ptr_reg, ptr_reg->type))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784563939.git.chenyy23@mails.tsinghua.edu.cn?part=1