| Message ID | 20200904112401.667645-5-lmb@cloudflare.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | RFC: Make check_func_arg table driven | expand |
On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > Move the check for a NULL or zero register to check_helper_mem_access. This > makes check_stack_boundary easier to understand. > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > --- Looks good as is, but I'm curious about the question below. Acked-by: Andrii Nakryiko <andriin@fb.com> > kernel/bpf/verifier.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 509754c3aa7d..649bcfb4535e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > struct bpf_func_state *state = func(env, reg); > int err, min_off, max_off, i, j, slot, spi; > > - if (reg->type != PTR_TO_STACK) { > - /* Allow zero-byte read from NULL, regardless of pointer type */ > - if (zero_size_allowed && access_size == 0 && > - register_is_null(reg)) > - return 0; > - > - verbose(env, "R%d type=%s expected=%s\n", regno, > - reg_type_str[reg->type], > - reg_type_str[PTR_TO_STACK]); > - return -EACCES; > - } > - > if (tnum_is_const(reg->var_off)) { > min_off = max_off = reg->var_off.value + reg->off; > err = __check_stack_boundary(env, regno, min_off, access_size, > @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > access_size, zero_size_allowed, > "rdwr", > &env->prog->aux->max_rdwr_access); > - default: /* scalar_value|ptr_to_stack or invalid ptr */ > + case PTR_TO_STACK: > return check_stack_boundary(env, regno, access_size, > zero_size_allowed, meta); > + default: /* scalar_value or invalid ptr */ > + /* Allow zero-byte read from NULL, regardless of pointer type */ > + if (zero_size_allowed && access_size == 0 && > + register_is_null(reg)) > + return 0; Given comment explicitly states "regardless of pointer type", shouldn't this be checked before we do pointer type-specific checks? > + > + verbose(env, "R%d type=%s expected=%s\n", regno, > + reg_type_str[reg->type], > + reg_type_str[PTR_TO_STACK]); > + return -EACCES; > } > } > > -- > 2.25.1 >
On Wed, 9 Sep 2020 at 05:22, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > > > Move the check for a NULL or zero register to check_helper_mem_access. This > > makes check_stack_boundary easier to understand. > > > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > > --- > > Looks good as is, but I'm curious about the question below. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > kernel/bpf/verifier.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 509754c3aa7d..649bcfb4535e 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > > struct bpf_func_state *state = func(env, reg); > > int err, min_off, max_off, i, j, slot, spi; > > > > - if (reg->type != PTR_TO_STACK) { > > - /* Allow zero-byte read from NULL, regardless of pointer type */ > > - if (zero_size_allowed && access_size == 0 && > > - register_is_null(reg)) > > - return 0; > > - > > - verbose(env, "R%d type=%s expected=%s\n", regno, > > - reg_type_str[reg->type], > > - reg_type_str[PTR_TO_STACK]); > > - return -EACCES; > > - } > > - > > if (tnum_is_const(reg->var_off)) { > > min_off = max_off = reg->var_off.value + reg->off; > > err = __check_stack_boundary(env, regno, min_off, access_size, > > @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > > access_size, zero_size_allowed, > > "rdwr", > > &env->prog->aux->max_rdwr_access); > > - default: /* scalar_value|ptr_to_stack or invalid ptr */ > > + case PTR_TO_STACK: > > return check_stack_boundary(env, regno, access_size, > > zero_size_allowed, meta); > > + default: /* scalar_value or invalid ptr */ > > + /* Allow zero-byte read from NULL, regardless of pointer type */ > > + if (zero_size_allowed && access_size == 0 && > > + register_is_null(reg)) > > + return 0; > > Given comment explicitly states "regardless of pointer type", > shouldn't this be checked before we do pointer type-specific checks? That's a good question. As I understand it, this the check that the various comments in check_func_arg refer to: /* final test in check_stack_boundary() */ I think "regardless of pointer type" here means: we don't care what enum arg_type we're dealing with, since all NULL pointers are represented as SCALAR_VALUE with value 0. > > > + > > + verbose(env, "R%d type=%s expected=%s\n", regno, > > + reg_type_str[reg->type], > > + reg_type_str[PTR_TO_STACK]); > > + return -EACCES; > > } > > } > > > > -- > > 2.25.1 > >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 509754c3aa7d..649bcfb4535e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3594,18 +3594,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, struct bpf_func_state *state = func(env, reg); int err, min_off, max_off, i, j, slot, spi; - if (reg->type != PTR_TO_STACK) { - /* Allow zero-byte read from NULL, regardless of pointer type */ - if (zero_size_allowed && access_size == 0 && - register_is_null(reg)) - return 0; - - verbose(env, "R%d type=%s expected=%s\n", regno, - reg_type_str[reg->type], - reg_type_str[PTR_TO_STACK]); - return -EACCES; - } - if (tnum_is_const(reg->var_off)) { min_off = max_off = reg->var_off.value + reg->off; err = __check_stack_boundary(env, regno, min_off, access_size, @@ -3750,9 +3738,19 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, access_size, zero_size_allowed, "rdwr", &env->prog->aux->max_rdwr_access); - default: /* scalar_value|ptr_to_stack or invalid ptr */ + case PTR_TO_STACK: return check_stack_boundary(env, regno, access_size, zero_size_allowed, meta); + default: /* scalar_value or invalid ptr */ + /* Allow zero-byte read from NULL, regardless of pointer type */ + if (zero_size_allowed && access_size == 0 && + register_is_null(reg)) + return 0; + + verbose(env, "R%d type=%s expected=%s\n", regno, + reg_type_str[reg->type], + reg_type_str[PTR_TO_STACK]); + return -EACCES; } }
Move the check for a NULL or zero register to check_helper_mem_access. This makes check_stack_boundary easier to understand. Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- kernel/bpf/verifier.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)