Messages in this thread Patch in this message | | | Date | Tue, 31 Jul 2018 17:28:35 -0700 | From | Roman Gushchin <> | Subject | Re: [PATCH v4 bpf-next 08/14] bpf: introduce the bpf_get_local_storage() helper function |
| |
On Wed, Aug 01, 2018 at 12:50:16AM +0200, Daniel Borkmann wrote: > On 07/27/2018 11:52 PM, Roman Gushchin wrote: > [...] > > @@ -2533,6 +2541,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn > > } > > > > regs = cur_regs(env); > > + > > + /* check that flags argument in get_local_storage(map, flags) is 0, > > + * this is required because get_local_storage() can't return an error. > > + */ > > + if (func_id == BPF_FUNC_get_local_storage && > > + !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) { > > + verbose(env, "get_local_storage() doesn't support non-zero flags\n"); > > + return -EINVAL; > > + } > > Hmm, this check is actually not correct. You will still be able to pass non-zero > values in there. arg2_type from the helper is ARG_ANYTHING, so the register type > could for example be one of the pointer types and it will still pass the verifier. > The correct way to check would be to use register_is_null(). > > > + > > /* reset caller saved regs */ > > for (i = 0; i < CALLER_SAVED_REGS; i++) { > > mark_reg_not_init(env, regs, caller_saved[i]);
Oh, perfect catch! The diff is below. Please, let me know if you prefer me to resend the whole patch/patchset.
Thank you!
Roman
--
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0f69f827ff9f..4547e07ab3db 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2546,7 +2546,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn * this is required because get_local_storage() can't return an error. */ if (func_id == BPF_FUNC_get_local_storage && - !tnum_equals_const(regs[BPF_REG_2].var_off, 0)) { + !register_is_null(®s[BPF_REG_2])) { verbose(env, "get_local_storage() doesn't support non-zero flags\n"); return -EINVAL; }
| |