Re: [PATCH bpf-next v2 02/43] bpf: describe helper stack accesses with arg_access_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: must_write fails to discard liveness due to may_read mask
--

commit cbeee936583603e6c17220772e7f9ac86e513c50
Author: Eduard Zingerman <eddyz87@gmail.com>

bpf: describe helper stack accesses with arg_access_info

Adjust bpf_helper_stack_access_bytes() to return results as a struct
arg_access_info. The commit intends for MEM_WRITE to produce must_write
when allow_uninit_stack is true, allowing the verifier to discard
previous stack knowledge over the affected range.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -14455,18 +14455,21 @@ s64 bpf_helper_stack_access_bytes(struct bpf_verifier_env *env, struct bpf_insn
>  		return (struct arg_access_info) {};
>  	default:
> -		return S64_MIN;
> +		return info;
>  	}
>  out:
> -	/*
> -	 * Other accesses keep the previous state live, including untouched bytes
> -	 * of an unprivileged generic output.
> -	 */
> -	if (full_write)
> -		return -size;
> -	return size;
> +	info.size = min_t(u64, size, U32_MAX);
> +	info.may_read = !!(access_type & BPF_READ);
> +	info.may_write = !!(access_type & BPF_WRITE);
> +	info.must_write = info.may_write && exact_size && info.size != U32_MAX;

[Severity: Medium]
When a helper uses MEM_WRITE without MEM_UNINIT, access_type evaluates to
BPF_READ | BPF_WRITE. This logic then sets both may_read and must_write to
true for the argument.

Does this accidentally defeat the intended liveness pruning optimization?

Looking at the liveness propagation equation in kernel/bpf/liveness.c
inside update_insn():

unsigned long new_before = (new_after[w] & ~must_write[w]) | may_read[w];

If may_read is true, the ~must_write clearing operation is overridden. As a
result, the bit is unconditionally set in new_before, and the verifier will
continue to track the state of these stack slots instead of discarding them.

> +	/* Generic unprivileged outputs retain prior initialization state. */
> +	if (!env->allow_uninit_stack && arg_type_is_raw_mem(at)) {
> +		info.may_read = true;
> +		info.must_write = false;
> +	}
> +	return info;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-scev-minimal-rebase-v2-0-f6254e29fdf2@gmail.com?part=2