On Thu, Jun 29, 2023 at 11:35 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Jun 28, 2023 at 8:12 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >
> > On Wed, Jun 28, 2023 at 7:52 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> > >
> > > Per discussion with Alexei, the PTR_UNTRUSTED flag should not been
> > > cleared when we start to walk a new struct, because the struct in
> > > question may be a struct nested in a union. We should also check and set
> > > this flag before we walk its each member, in case itself is a union.
> > >
> > > Fixes: 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> > > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > > ---
> > > kernel/bpf/btf.c | 20 +++++++++-----------
> > > 1 file changed, 9 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 29fe21099298..e0a493230727 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6133,7 +6133,6 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> > > const char *tname, *mname, *tag_value;
> > > u32 vlen, elem_id, mid;
> > >
> > > - *flag = 0;
> > > again:
> > > tname = __btf_name_by_offset(btf, t->name_off);
> > > if (!btf_type_is_struct(t)) {
> > > @@ -6142,6 +6141,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> > > }
> > >
> > > vlen = btf_type_vlen(t);
> > > + if (BTF_INFO_KIND(t->info) == BTF_KIND_UNION && vlen != 1)
> > > + /*
> > > + * walking unions yields untrusted pointers
> > > + * with exception of __bpf_md_ptr and other
> > > + * unions with a single member
> > > + */
> > > + *flag |= PTR_UNTRUSTED;
> > > +
> > > if (off + size > t->size) {
> > > /* If the last element is a variable size array, we may
> > > * need to relax the rule.
> > > @@ -6302,15 +6309,6 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> > > * of this field or inside of this struct
> > > */
> > > if (btf_type_is_struct(mtype)) {
> > > - if (BTF_INFO_KIND(mtype->info) == BTF_KIND_UNION &&
> > > - btf_type_vlen(mtype) != 1)
> > > - /*
> > > - * walking unions yields untrusted pointers
> > > - * with exception of __bpf_md_ptr and other
> > > - * unions with a single member
> > > - */
> > > - *flag |= PTR_UNTRUSTED;
> > > -
> > > /* our field must be inside that union or struct */
> > > t = mtype;
> > >
> > > @@ -6476,7 +6474,7 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > > bool strict)
> > > {
> > > const struct btf_type *type;
> > > - enum bpf_type_flag flag;
> > > + enum bpf_type_flag flag = 0;
> > > int err;
> > >
> > > /* Are we already done? */
> > > --
> > > 2.39.3
> > >
> >
> > Just noticed that it breaks test_sk_storage_tracing, because skb->sk
> > is in a union:
> > struct sk_buff {
> > ...
> > union {
> > struct sock *sk;
> > int ip_defrag_offset;
> > };
> > ...
> > };
> >
> > I will think about it.
>
> It can be whitelisted similar to BTF_TYPE_SAFE_*.
Got it.
> Please add a selftest for the new feature.
Sure, will add it.
--
Regards
Yafang
