Re: [PATCH bpf-next 1/2] bpf: propagate nullness information for reg to reg comparisons

[Eduard Zingerman <eddyz87@gmail.com>]

Hi Daniel,

Thank you for commenting.

> On Mon, 2022-08-29 at 16:23 +0200, Daniel Borkmann wrote:
> [...]
> >   kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0194a36d0b36..7585288e035b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -472,6 +472,11 @@ static bool type_may_be_null(u32 type)
> >   	return type & PTR_MAYBE_NULL;
> >   }
> >   
> > +static bool type_is_pointer(enum bpf_reg_type type)
> > +{
> > +	return type != NOT_INIT && type != SCALAR_VALUE;
> > +}
> 
> We also have is_pointer_value(), semantics there are a bit different (and mainly to
> prevent leakage under unpriv), but I wonder if this can be refactored to accommodate
> both. My worry is that if in future we extend one but not the other bugs might slip
> in.

John was concerned about this as well, guess I won't not dodging it :)
Suppose I do the following modification:

    static bool type_is_pointer(enum bpf_reg_type type)
    {
    	return type != NOT_INIT && type != SCALAR_VALUE;
    }
    
    static bool __is_pointer_value(bool allow_ptr_leaks,
    			       const struct bpf_reg_state *reg)
    {
    	if (allow_ptr_leaks)
    		return false;

-    	return reg->type != SCALAR_VALUE;
+    	return type_is_pointer(reg->type);
    }
    
And check if there are test cases that have to be added because of the
change in the __is_pointer_value behavior (it does not check for
`NOT_INIT` right now). Does this sound like a plan?

[...]
> Could we consolidate the logic above with the one below which deals with R == 0 checks?
> There are some similarities, e.g. !is_jmp32, both test for jeq/jne and while one is based
> on K, the other one on X, though we could also add check X == 0 for below. Anyway, just
> a though that it may be nice to consolidate the handling.

Ok, I will try to consolidate those.

Thanks,
Eduard