Re: [PATCH bpf-next v4 3/4] bpf: Reject untrusted allocated-object pointers

> The immediately following invalidate_non_owning_refs() only acts on
> registers where type_is_non_owning_ref() returns true. But after this
> patch, type_is_non_owning_ref() returns false for registers with
> PTR_UNTRUSTED.

Makes sense, I think the thing is, callers do not have a consistent
contract of `type_is_non_owning_ref()` (same for
`type_is_ptr_alloc_obj()`). Some of them only care about whether a
register has the NON_OWN_REF flag or not, but some callers assume it
also should not contain the PTR_UNTRUSTED flag. So if I make some
callers happy, then other callers probably will complain because I
break their assumption.

Then I think the previous approach, which preserves the old semantics:
`type_is_ptr_alloc_obj()` only checks the flags it declared in its
name and doesn't care about whether other flags are here or not will
work better. If some caller explicitly need to ensure the register
does not have PTR_UNTRUSTED, they should check it by themselves.

> The commit message states untrusted pointers "are still allowed for
> BPF_PROBE_MEM reads," but does the BPF_PROBE_MEM conversion actually
> work for the NON_OWN_REF variant?
>
> Looking at kernel/bpf/fixups.c, the probe conversion logic has:
>
>     case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
>             /* probe mem access */
>             ...
>     default:
>             continue;
>
> It has a case for the plain MEM_ALLOC|PTR_UNTRUSTED type, but no case
> for PTR_TO_BTF_ID|MEM_ALLOC|NON_OWN_REF|PTR_UNTRUSTED. So a load
> through the surviving register is emitted as a plain load with no
> exception-table entry, which would fault in the kernel instead of
> returning zero.
I didn't consider the NON_OWN_REF combination case, but I think that
should be a separate patch for fixups.c?

> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index bc021c8c8fbf7..c3a11c98cd451 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5820,7 +5820,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> >                       return -EACCES;
> >               }
> >
> > -             if (type_is_alloc(reg->type) && !type_is_non_owning_ref(reg->type) &&
> > +             /*
> > +              * Skip this referenced-ID sanity check for untrusted allocated objects;
> > +              * the access check above already rejects writes through them.
> > +              */
> > +             if (type_is_alloc(reg->type) &&
> > +                 !type_is_untrusted_ptr_alloc_obj(reg->type) &&
> > +                 !type_is_non_owning_ref(reg->type) &&
> >                   !(reg->type & MEM_RCU) && !reg_is_referenced(env, reg)) {
> >                       verifier_bug(env, "allocated object must have a referenced id");
> >                       return -EFAULT;
>
> This change allows the demoted PTR_UNTRUSTED|NON_OWN_REF registers to
> reach mark_btf_ld_reg() instead of hitting the sanity assertion, which
> opens the path for the read-through described above.

I think for the exemption, that is intended, ths bad thing is about
the fixups.c does not handle
PTR_TO_BTF_ID|MEM_ALLOC|NON_OWN_REF|PTR_UNTRUSTED.

Overall, there are 2 separate problems:
1. type_is_non_owning_ref should be fixed to preserve the original semantics
2. For the exemption, new cases should be added to fixups.c.

For problem one, I think there are 2 approaches:
1. Preserve the old type_is_ptr_alloc_obj() code, but for any caller
who wants to verify that the register is also trusted, they should
check themselves explicitly.
2. Change the type_is_non_owning_ref:
static inline bool type_is_non_owning_ref(u32 type)
{
return (type_is_ptr_alloc_obj(type) ||
type_is_untrusted_ptr_alloc_obj(type)) &&
      type_flag(type) & NON_OWN_REF;
}