On Tue, Aug 30, 2022 at 01:41:28PM +0300, Eduard Zingerman wrote:
> 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);
> }
The verifier is using the wrapped is_pointer_value() to guard against
pointer leak.
static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno,
int off, int bpf_size, enum bpf_access_type t,
int value_regno, bool strict_alignment_once)
{
...
if (reg->type == PTR_TO_MAP_KEY) {
...
} else if (reg->type == PTR_TO_MAP_VALUE) {
struct bpf_map_value_off_desc *kptr_off_desc = NULL;
if (t == BPF_WRITE && value_regno >= 0 &&
is_pointer_value(env, value_regno)) {
verbose(env, "R%d leaks addr into map\n", value_regno);
return -EACCES;
...
}
...
}
In the check_mem_access() case the semantic of is_pointer_value() is check
whether or not the value *might* be a pointer, and since NON_INIT can be
potentially anything, it should not be excluded.
Since the use case seems different, perhaps we could split them up, e.g. a
maybe_pointer_value() and a is_pointer_value(), or something along that
line.
The former is equivalent to type != SCALAR_VALUE, and the latter equivalent
to type != NOT_INIT && type != SCALAR_VALUE. The latter can be used here for
implementing nullness propogation.
> 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
