On Tue, Jul 11, 2023 at 12:59 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On 07/09, Yafang Shao 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;
> > +};
>
> Does it say that sk member of sk_buff is always dereferencable?
> Why is it universally safe?
> In general, I don't really understand why it's safe to statically
> mark the members this way. Shouldn't it depend on the context?
Right. It should depend on the context. Will change it.
--
Regards
Yafang
