On Wed, 9 Sep 2020 at 05:22, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Sep 4, 2020 at 4:29 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
>
> Looks good as is, but I'm curious about the question below.
>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
>
> > 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
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
