Re: [PATCH bpf-next v2 1/5] bpf: allow variable-offset stack access
From: Andrei Matei <hidden>
Date: 2021-02-07 01:12:30
quoted
+/* 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().
quoted
+ /* 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.quoted
+ + 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
quoted
+ 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.