| Message ID | 20230628115205.248395-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix errors in verifying a union | expand |
On Wed, Jun 28, 2023 at 7:52 PM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > --- > 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.
On Wed, Jun 28, 2023 at 8:12 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Wed, Jun 28, 2023 at 7:52 PM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > > --- > > 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_*. Please add a selftest for the new feature.
On Thu, Jun 29, 2023 at 11:35 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Jun 28, 2023 at 8:12 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > On Wed, Jun 28, 2023 at 7:52 PM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > > > --- > > > 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.
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? */
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@gmail.com> --- kernel/bpf/btf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)