Re: [PATCH bpf-next v1 16/29] bpf: Describe locks at throw sites

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: s16 overflow in ref_locations() when computing lock offset
  adjustment
--

commit b280bb740c33eccf7a76373a4566fec46d049b84
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Describe locks at throw sites

This commit records held locks in throw site descriptors either by their fixed
address for single-element array maps or relative to a stack slot or register
holding a pointer to the containing object, avoiding hidden stack slots when
every visit to the site agrees on the lock.

> diff --git a/kernel/bpf/frame_desc.c b/kernel/bpf/frame_desc.c
> index 5cb837688f90c..72dc6597321a9 100644
> --- a/kernel/bpf/frame_desc.c
> +++ b/kernel/bpf/frame_desc.c

[ ... ]

> @@ -805,42 +844,50 @@ static void ref_locations(struct bpf_verifier_env *env, const struct bpf_verifie
>  	const struct bpf_res_acq *acq = &env->res_acqs[ref->acq - 1];
>  	const struct bpf_func_state *frame = st->frame[f];
>  	s16 adj, lim = env->subprog_info[frame->subprogno].fastcall_stack_end;
> +	bool lock = ref->type & REF_TYPE_LOCK_MASK, ptr;
>  	const struct bpf_stack_state *slot;
>  	int k;
>  
> +	memset(loc->at, 0, sizeof(loc->at));
>  	loc->cnt = 0;
>  	loc->obj = false;
> -	if (ref->type & REF_TYPE_LOCK_MASK)
> +	loc->fixed = lock && env->resources[acq->res].addr;
> +	if (loc->fixed) {
> +		loc->cnt = 1;
>  		return;
> +	}
> +	/* a pointer the frame keeps: to the object, or to what a lock with an id is in */
> +	ptr = lock ? ref->id && !(acq->flags & BPF_RES_F_NO_PTR) :
> +		     ref->type == REF_TYPE_PTR && !(acq->flags & BPF_RES_F_ARG);
>  	for (k = 0; k < bpf_stack_nr_slots(frame) && loc->cnt < BPF_RES_LOC_MAX; k++) {
>  		slot = bpf_stack_slot(frame, k);
>  		adj = 0;
>  		if (spi_off(k) < lim)
>  			break;
> -		if (stack_slot_is_object(slot, ref->id)) {
> +		if (!lock && stack_slot_is_object(slot, ref->id)) {
>  			/* the handle of a dynptr is its first word, as when it was created */
>  			if (slot->slot_type[BPF_REG_SIZE - 1] != STACK_DYNPTR) {
>  				loc->obj = true;
>  				loc->cnt = 0;
>  			}
> -		} else if (ref->type != REF_TYPE_PTR || (acq->flags & BPF_RES_F_ARG) ||
> -			   !bpf_is_spilled_reg(slot) || !reg_ref_off(&slot->spilled_ptr, ref->id, &adj)) {
> +		} else if (!ptr || !bpf_is_spilled_reg(slot) ||
> +			   !reg_ref_off(&slot->spilled_ptr, ref->id, &adj)) {
>  			continue;
>  		}
>  		loc->at[loc->cnt].off = spi_off(k);
> -		loc->at[loc->cnt].reg = false;
> -		loc->at[loc->cnt++].adj = adj;
> +		/* the acquisition of a lock has how far the lock is into what it is in */
> +		loc->at[loc->cnt++].adj = lock ? adj - acq->adj : adj;
>  		if (loc->obj)
>  			return;
>  	}
> -	if (!env->describe_regs || ref->type != REF_TYPE_PTR || (acq->flags & BPF_RES_F_ARG))
> +	if (!env->describe_regs || !ptr)
>  		return;
>  	for (k = 0; k < (throws ? ARRAY_SIZE(site_regs) : 4) && loc->cnt < BPF_RES_LOC_MAX; k++) {
>  		if (!reg_ref_off(&frame->regs[site_regs[k]], ref->id, &adj))
>  			continue;
>  		loc->at[loc->cnt].off = site_regs[k];
>  		loc->at[loc->cnt].reg = true;
> -		loc->at[loc->cnt++].adj = adj;
> +		loc->at[loc->cnt++].adj = lock ? adj - acq->adj : adj;
>  	}
>  }

[Severity: High]
Can adj - acq->adj underflow the s16 loc->at[].adj field in both loops?

resource_of_lock() checks that acq->adj fits in s16 (0..32767) and
reg_ref_off() checks that the pointer's constant var_off fits in s16
(-32768..32767), but when the spilled or register pointer has been adjusted to
a negative offset and acq->adj > 0 such that adj - acq->adj < S16_MIN, storing
the difference into loc->at[].adj wraps by +65536.  On unwind,
bpf_unwind_release_desc() computes handle = (void *)(long)(val - ent->adj) and
passes an out-of-bounds address 65536 bytes below the lock to
bpf_spin_unlock() or bpf_res_spin_unlock(), corrupting kernel memory and
leaving the lock held.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794399387.git.memxor@gmail.com?part=16