On Tue, Jul 11, 2023 at 10:56 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> >
> > When we are verifying a field in a union, we may unexpectedly verify
> > another field which has the same offset in this union. So in such case,
> > we should annotate that field as PTR_UNTRUSTED. However, in some cases
> > we are sure some fields in a union is safe and then we can add them into
> > BTF_TYPE_SAFE_TRUSTED_UNION allow list.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > ---
> > kernel/bpf/btf.c | 20 +++++++++-----------
> > kernel/bpf/verifier.c | 21 +++++++++++++++++++++
> > 2 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 3dd47451f097..fae6fc24a845 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:
> > if (btf_type_is_modifier(t))
> > t = btf_type_skip_modifiers(btf, t->type, NULL);
> > @@ -6144,6 +6143,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 && !(*flag & PTR_UNTRUSTED))
> > + /*
> > + * 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.
> > @@ -6304,15 +6311,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;
> >
> > @@ -6478,7 +6476,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? */
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 11e54dd8b6dd..1fb0a64f5bce 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5847,6 +5847,7 @@ static int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val)
> > #define BTF_TYPE_SAFE_RCU(__type) __PASTE(__type, __safe_rcu)
> > #define BTF_TYPE_SAFE_RCU_OR_NULL(__type) __PASTE(__type, __safe_rcu_or_null)
> > #define BTF_TYPE_SAFE_TRUSTED(__type) __PASTE(__type, __safe_trusted)
> > +#define BTF_TYPE_SAFE_TRUSTED_UNION(__type) __PASTE(__type, __safe_trusted_union)
> >
> > /*
> > * Allow list few fields as RCU trusted or full trusted.
> > @@ -5914,6 +5915,11 @@ BTF_TYPE_SAFE_TRUSTED(struct socket) {
> > struct sock *sk;
> > };
> >
> > +/* union trusted: these fields are trusted even in a uion */
> > +BTF_TYPE_SAFE_TRUSTED_UNION(struct sk_buff) {
> > + struct sock *sk;
> > +};
>
> Why is this needed?
Will discard it.
> We already have:
> BTF_TYPE_SAFE_RCU_OR_NULL(struct sk_buff) {
> struct sock *sk;
> };
>
> > + /* Clear the PTR_UNTRUSTED for the fields which are in the allow list */
> > + if (type_is_trusted_union(env, reg, field_name, btf_id))
> > + flag &= ~PTR_UNTRUSTED;
>
> we cannot do this unconditionally.
> The type_is_rcu_or_null() check applies only after
> in_rcu_cs(env) && !type_may_be_null(reg->type)).
Thanks for the explanation. Will change it.
--
Regards
Yafang
