Re: [PATCH bpf 1/2] bpf, verifier: fold reg->var_off into PTR_TO_FLOW_KEYS bounds check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Negative variable offsets on PTR_TO_BUF and PTR_TO_TP_BUFFER completely bypass memory bounds tracking, permitting out-of-bounds reads/writes at runtime.
--

commit 2118c968df54a167a0c832db86595b84ed0a8c71
Author: Nuiqi Gui <gnq25@mails.tsinghua.edu.cn>

bpf, verifier: fold reg->var_off into PTR_TO_FLOW_KEYS bounds check

This commit fixes a bounds checking bypass for PTR_TO_FLOW_KEYS by folding
a constant reg->var_off into the offset before validation. This prevents
out-of-bounds reads and writes to kernel stack memory.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4997,9 +4997,22 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  	return err;
>  }
>  
> -static int check_flow_keys_access(struct bpf_verifier_env *env, int off,
> -				  int size)
> +static int check_flow_keys_access(struct bpf_verifier_env *env, u32 regno,
> +				  int off, int size)
>  {
> +	struct bpf_reg_state *reg = reg_state(env, regno);
> +
> +	/* Only a constant offset is allowed here; fold it into off. */
> +	if (!tnum_is_const(reg->var_off)) {
> +		char tn_buf[48];
> +
> +		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> +		verbose(env, "R%d invalid variable offset to flow keys: off=%d, var_off=%s\n",
> +			regno, off, tn_buf);
> +		return -EACCES;
> +	}
> +	off += reg->var_off.value;
> +
>  	if (size < 0 || off < 0 ||
>  	    (u64)off + size > sizeof(struct bpf_flow_keys)) {
>  		verbose(env, "invalid access to flow keys off=%d size=%d\n",

[Severity: Critical]
This isn't a bug introduced by this patch, but while looking at how we
validate reg->var_off.value here, do check_buffer_access() and
check_tp_buffer_access() have a vulnerability with negative variable offsets?

In __check_buffer_access(), we reject non-constant variable offsets, but we
don't seem to verify if reg->var_off.value or reg->smin_value is negative:

kernel/bpf/verifier.c:__check_buffer_access() {
    ...
    if (!tnum_is_const(reg->var_off)) {
        ...
    }
    ...
}

Later in check_buffer_access(), this gets added to a u32 *max_access:

kernel/bpf/verifier.c:check_buffer_access() {
    ...
    *max_access = max(reg->var_off.value + off + size, *max_access);
    ...
}

If reg->var_off.value is a large negative 64-bit constant whose lower 32
bits evaluate to a small positive number (e.g., -4294967296, which is
0xFFFFFFFF00000000ULL), does reg->var_off.value + off + size truncate when
assigned to the u32 *max_access?

If it truncates to a small value, it seems like it could bypass the bounds
check, making a massively out-of-bounds access appear perfectly valid to
the verifier.

It appears check_tp_buffer_access() might apply the identical bounds
truncation logic:

kernel/bpf/verifier.c:check_tp_buffer_access() {
    ...
    env->prog->aux->max_tp_access = max(reg->var_off.value + off + size,
                                        env->prog->aux->max_tp_access);
    ...
}

Could this lead to out-of-bounds access at runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604150755.2487555-1-gnq25@mails.tsinghua.edu.cn?part=1