| Message ID | 20230709025912.3837-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Introduce union trusted | expand |
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@gmail.com> > --- > 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?
On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > --- > 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? 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)).
On Tue, Jul 11, 2023 at 12:59 AM Stanislav Fomichev <sdf@google.com> 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@gmail.com> > > --- > > 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.
On Tue, Jul 11, 2023 at 10:56 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <laoar.shao@gmail.com> 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@gmail.com> > > --- > > 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.
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; +}; + static bool type_is_rcu(struct bpf_verifier_env *env, struct bpf_reg_state *reg, const char *field_name, u32 btf_id) @@ -5950,6 +5956,17 @@ static bool type_is_trusted(struct bpf_verifier_env *env, return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_trusted"); } + +static bool type_is_trusted_union(struct bpf_verifier_env *env, + struct bpf_reg_state *reg, + const char *field_name, u32 btf_id) +{ + BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED_UNION(struct sk_buff)); + + return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, + "__safe_trusted_union"); +} + static int check_ptr_to_btf_access(struct bpf_verifier_env *env, struct bpf_reg_state *regs, int regno, int off, int size, @@ -6087,6 +6104,10 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); } + /* 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; + if (atype == BPF_READ && value_regno >= 0) mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
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@gmail.com> --- kernel/bpf/btf.c | 20 +++++++++----------- kernel/bpf/verifier.c | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 11 deletions(-)