Re: [PATCH bpf v4 1/2] bpf: Reject negative const offsets for buffer pointers

[Shung-Hsi Yu <shung-hsi.yu@suse.com>]

On Wed, Jul 08, 2026 at 02:01:50AM -0700, Sun Jian wrote:
[...]
> Fixes: 022ac0750883 ("bpf: use reg->var_off instead of reg->off for pointers")

Agree that above is the commit that introduced the issue.

More comments below.

[...]
> @@ -5326,14 +5326,18 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
>  static int __check_buffer_access(struct bpf_verifier_env *env,
>  				 const char *buf_info,
>  				 const struct bpf_reg_state *reg,
> -				 argno_t argno, int off, int size)
> +				 argno_t argno, int off, int size,
> +				 u32 *access_end)
>  {
> +	s64 start, var_off;
> +
>  	if (off < 0) {
>  		verbose(env,
>  			"%s invalid %s buffer access: off=%d, size=%d\n",
>  			reg_arg_name(env, argno), buf_info, off, size);
>  		return -EACCES;
>  	}
> +
>  	if (!tnum_is_const(reg->var_off)) {
>  		char tn_buf[48];
>  
> @@ -5344,6 +5348,29 @@ static int __check_buffer_access(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> +	var_off = (s64)reg->var_off.value;
> +	if (var_off >= BPF_MAX_VAR_OFF || var_off <= -BPF_MAX_VAR_OFF) {
> +		verbose(env, "%s %s buffer offset %lld is not allowed\n",
> +			reg_arg_name(env, argno), buf_info, var_off);
> +		return -EACCES;
> +	}
> +
> +	start = var_off + off;
> +	if (start < 0) {
> +		verbose(env,
> +			"%s invalid negative %s buffer offset: off=%d, var_off=%lld\n",
> +			reg_arg_name(env, argno), buf_info, off, var_off);
> +		return -EACCES;
> +	}

I was thinking of suggest to just do a single unsigned check

  var_off = reg->var_off.value;
  if (var_off >= BPF_MAX_VAR_OFF) {
    ...

But looking at the code before 022ac0750883, what you have is closer
aligned to the previous behavior, let's stick to this.

> +
> +	if (size < 0) {
> +		verbose(env, "%s invalid %s buffer access: off=%lld, size=%d\n",
> +			reg_arg_name(env, argno), buf_info, start, size);
> +		return -EACCES;
> +	}

`size` comes from bpf_size_to_bytes(bpf_size) in check_mem_access(), and
is already checked to be not negative; plus other check_* helpers does
not check for this, so I think it can be dropped.

Otherwise, LGTM:

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>

> +
> +	*access_end = (u32)start + (u32)size;

If you want, this could use a quick one-line comments regarding the fact
that it won't overflow, or even verifier_bug_if().

> +
>  	return 0;
>  }
>  
[...]