> > +/* Check that the stack access at 'regno + off' falls within the maximum stack
> > + * bounds.
> > + *
> > + * 'off' includes `regno->offset`, but not its dynamic part (if any).
> > + */
> > +static int check_stack_access_within_bounds(
> > + struct bpf_verifier_env *env,
> > + int regno, int off, int access_size,
> > + enum stack_access_src src, enum bpf_access_type type)
> > +{
> > + struct bpf_reg_state *regs = cur_regs(env);
> > + struct bpf_reg_state *reg = regs + regno;
> > + struct bpf_func_state *state = func(env, reg);
> > + int min_off, max_off;
> > + int err;
> > + char *err_extra;
> > +
> > + if (src == ACCESS_HELPER)
>
> the ACCESS_HELPER|DIRECT enum should probably be moved right before this function.
> It's not used earlier, I think, and it made the reviewing a bit harder than could have been.
It is, unfortunately. ACCESS_DIRECT is used close to where it is
defined, in check_stack_read_var_off().
>
> > + /* We don't know if helpers are reading or writing (or both). */
> > + err_extra = " indirect access to";
> > + else if (type == BPF_READ)
> > + err_extra = " read from";
> > + else
> > + err_extra = " write to";
>
> Thanks for improving verifier errors.
>
> > +
> > + if (tnum_is_const(reg->var_off)) {
> > + min_off = reg->var_off.value + off;
> > + if (access_size > 0)
> > + max_off = min_off + access_size - 1;
> > + else
> > + max_off = min_off;
> > + } else {
> > + if (reg->smax_value >= BPF_MAX_VAR_OFF ||
> > + reg->smax_value <= -BPF_MAX_VAR_OFF) {
>
> hmm. are you sure about smax in both conditions? looks like typo?
This is how it used to be before this patch btw, but I think you're
right. It looks like the second one should have been smin_value.
Fixing here.
Existing code: https://github.com/torvalds/linux/blob/1e0d27fce010b0a4a9e595506b6ede75934c31be/kernel/bpf/verifier.c#L3721
>
> > + verbose(env, "invalid unbounded variable-offset%s stack R%d\n",
> > + err_extra, regno);
> > + return -EACCES;
> > + }
> > + min_off = reg->smin_value + off;
> > + if (access_size > 0)
> > + max_off = reg->smax_value + off + access_size - 1;
> > + else
> > + max_off = min_off;
> > + }
>
> The rest looks good.
