* [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-09 2:59 [PATCH bpf-next 0/3] bpf: Introduce union trusted Yafang Shao
@ 2023-07-09 2:59 ` Yafang Shao
2023-07-10 16:59 ` Stanislav Fomichev
2023-07-11 2:55 ` Alexei Starovoitov
2023-07-09 2:59 ` [PATCH bpf-next 2/3] selftests/bpf: Add selftests for BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
2023-07-09 2:59 ` [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union Yafang Shao
2 siblings, 2 replies; 10+ messages in thread
From: Yafang Shao @ 2023-07-09 2:59 UTC (permalink / raw)
To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, Yafang Shao
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;
+};
+
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);
--
2.30.1 (Apple Git-130)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-09 2:59 ` [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
@ 2023-07-10 16:59 ` Stanislav Fomichev
2023-07-11 14:20 ` Yafang Shao
2023-07-11 2:55 ` Alexei Starovoitov
1 sibling, 1 reply; 10+ messages in thread
From: Stanislav Fomichev @ 2023-07-10 16:59 UTC (permalink / raw)
To: Yafang Shao
Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, haoluo, jolsa, bpf
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-10 16:59 ` Stanislav Fomichev
@ 2023-07-11 14:20 ` Yafang Shao
0 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2023-07-11 14:20 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, haoluo, jolsa, bpf
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.
--
Regards
Yafang
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-09 2:59 ` [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
2023-07-10 16:59 ` Stanislav Fomichev
@ 2023-07-11 2:55 ` Alexei Starovoitov
2023-07-11 14:21 ` Yafang Shao
1 sibling, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2023-07-11 2:55 UTC (permalink / raw)
To: Yafang Shao
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
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)).
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-11 2:55 ` Alexei Starovoitov
@ 2023-07-11 14:21 ` Yafang Shao
0 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2023-07-11 14:21 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
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.
--
Regards
Yafang
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next 2/3] selftests/bpf: Add selftests for BTF_TYPE_SAFE_TRUSTED_UNION
2023-07-09 2:59 [PATCH bpf-next 0/3] bpf: Introduce union trusted Yafang Shao
2023-07-09 2:59 ` [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
@ 2023-07-09 2:59 ` Yafang Shao
2023-07-09 2:59 ` [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union Yafang Shao
2 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2023-07-09 2:59 UTC (permalink / raw)
To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, Yafang Shao
Add selftests for BTF_TYPE_SAFE_TRUSTED_UNION, the result as follows:
#141/1 nested_trust/test_read_cpumask:OK
#141/2 nested_trust/test_skb_field:OK <<<<
#141/3 nested_trust/test_invalid_nested_user_cpus:OK
#141/4 nested_trust/test_invalid_nested_offset:OK
#141/5 nested_trust/test_invalid_skb_field:OK <<<<
#141 nested_trust:OK
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
.../selftests/bpf/progs/nested_trust_failure.c | 16 ++++++++++++++++
.../selftests/bpf/progs/nested_trust_success.c | 15 +++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/nested_trust_failure.c b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
index 0d1aa6bbace4..ea39497f11ed 100644
--- a/tools/testing/selftests/bpf/progs/nested_trust_failure.c
+++ b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
@@ -10,6 +10,13 @@
char _license[] SEC("license") = "GPL";
+struct {
+ __uint(type, BPF_MAP_TYPE_SK_STORAGE);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, int);
+ __type(value, u64);
+} sk_storage_map SEC(".maps");
+
/* Prototype for all of the program trace events below:
*
* TRACE_EVENT(task_newtask,
@@ -31,3 +38,12 @@ int BPF_PROG(test_invalid_nested_offset, struct task_struct *task, u64 clone_fla
bpf_cpumask_first_zero(&task->cpus_mask);
return 0;
}
+
+/* Although R2 is of type sk_buff but sock_common is expected, we will hit untrusted ptr first. */
+SEC("tp_btf/tcp_probe")
+__failure __msg("R2 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_")
+int BPF_PROG(test_invalid_skb_field, struct sock *sk, struct sk_buff *skb)
+{
+ bpf_sk_storage_get(&sk_storage_map, skb->next, 0, 0);
+ return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/nested_trust_success.c b/tools/testing/selftests/bpf/progs/nested_trust_success.c
index 886ade4aa99d..833840bffd3b 100644
--- a/tools/testing/selftests/bpf/progs/nested_trust_success.c
+++ b/tools/testing/selftests/bpf/progs/nested_trust_success.c
@@ -10,6 +10,13 @@
char _license[] SEC("license") = "GPL";
+struct {
+ __uint(type, BPF_MAP_TYPE_SK_STORAGE);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, int);
+ __type(value, u64);
+} sk_storage_map SEC(".maps");
+
SEC("tp_btf/task_newtask")
__success
int BPF_PROG(test_read_cpumask, struct task_struct *task, u64 clone_flags)
@@ -17,3 +24,11 @@ int BPF_PROG(test_read_cpumask, struct task_struct *task, u64 clone_flags)
bpf_cpumask_test_cpu(0, task->cpus_ptr);
return 0;
}
+
+SEC("tp_btf/tcp_probe")
+__success
+int BPF_PROG(test_skb_field, struct sock *sk, struct sk_buff *skb)
+{
+ bpf_sk_storage_get(&sk_storage_map, skb->sk, 0, 0);
+ return 0;
+}
--
2.30.1 (Apple Git-130)
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union
2023-07-09 2:59 [PATCH bpf-next 0/3] bpf: Introduce union trusted Yafang Shao
2023-07-09 2:59 ` [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
2023-07-09 2:59 ` [PATCH bpf-next 2/3] selftests/bpf: Add selftests for BTF_TYPE_SAFE_TRUSTED_UNION Yafang Shao
@ 2023-07-09 2:59 ` Yafang Shao
2023-07-11 2:56 ` Alexei Starovoitov
2 siblings, 1 reply; 10+ messages in thread
From: Yafang Shao @ 2023-07-09 2:59 UTC (permalink / raw)
To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, Yafang Shao
We are utilizing BPF LSM to monitor BPF operations within our container
environment. When we add support for raw_tracepoint, it hits below
error.
; (const void *)attr->raw_tracepoint.name);
27: (79) r3 = *(u64 *)(r2 +0)
access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
It can be reproduced with below BPF prog.
SEC("lsm/bpf")
int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
{
switch (cmd) {
case BPF_RAW_TRACEPOINT_OPEN:
bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
break;
default:
break;
}
return 0;
}
The reason is that when accessing a field in a union, such as bpf_attr,
if the field is located within a nested struct that is not the first
member of the union, it can result in incorrect field verification.
union bpf_attr {
struct {
__u32 map_type; <<<< Actually it will find that field.
__u32 key_size;
__u32 value_size;
...
};
...
struct {
__u64 name; <<<< We want to verify this field.
__u32 prog_fd;
} raw_tracepoint;
};
Considering the potential deep nesting levels, finding a perfect
solution to address this issue has proven challenging. Therefore, I
propose a solution where we simply skip the verification process if the
field in question is located within a union.
Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
kernel/bpf/btf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index fae6fc24a845..a542760c807a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6368,7 +6368,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
* that also allows using an array of int as a scratch
* space. e.g. skb->cb[].
*/
- if (off + size > mtrue_end) {
+ if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {
bpf_log(log,
"access beyond the end of member %s (mend:%u) in struct %s with off %u size %u\n",
mname, mtrue_end, tname, off, size);
--
2.30.1 (Apple Git-130)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union
2023-07-09 2:59 ` [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union Yafang Shao
@ 2023-07-11 2:56 ` Alexei Starovoitov
2023-07-11 14:22 ` Yafang Shao
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2023-07-11 2:56 UTC (permalink / raw)
To: Yafang Shao
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> We are utilizing BPF LSM to monitor BPF operations within our container
> environment. When we add support for raw_tracepoint, it hits below
> error.
>
> ; (const void *)attr->raw_tracepoint.name);
> 27: (79) r3 = *(u64 *)(r2 +0)
> access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
>
> It can be reproduced with below BPF prog.
>
> SEC("lsm/bpf")
> int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
> {
> switch (cmd) {
> case BPF_RAW_TRACEPOINT_OPEN:
> bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
> break;
> default:
> break;
> }
> return 0;
> }
>
> The reason is that when accessing a field in a union, such as bpf_attr,
> if the field is located within a nested struct that is not the first
> member of the union, it can result in incorrect field verification.
>
> union bpf_attr {
> struct {
> __u32 map_type; <<<< Actually it will find that field.
> __u32 key_size;
> __u32 value_size;
> ...
> };
> ...
> struct {
> __u64 name; <<<< We want to verify this field.
> __u32 prog_fd;
> } raw_tracepoint;
> };
>
> Considering the potential deep nesting levels, finding a perfect
> solution to address this issue has proven challenging. Therefore, I
> propose a solution where we simply skip the verification process if the
> field in question is located within a union.
>
> Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> kernel/bpf/btf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index fae6fc24a845..a542760c807a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6368,7 +6368,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> * that also allows using an array of int as a scratch
> * space. e.g. skb->cb[].
> */
> - if (off + size > mtrue_end) {
> + if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {
The selftest for this condition is missing.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next 3/3] bpf: Fix an error in verifying a field in a union
2023-07-11 2:56 ` Alexei Starovoitov
@ 2023-07-11 14:22 ` Yafang Shao
0 siblings, 0 replies; 10+ messages in thread
From: Yafang Shao @ 2023-07-11 14:22 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
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:
> >
> > We are utilizing BPF LSM to monitor BPF operations within our container
> > environment. When we add support for raw_tracepoint, it hits below
> > error.
> >
> > ; (const void *)attr->raw_tracepoint.name);
> > 27: (79) r3 = *(u64 *)(r2 +0)
> > access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
> >
> > It can be reproduced with below BPF prog.
> >
> > SEC("lsm/bpf")
> > int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
> > {
> > switch (cmd) {
> > case BPF_RAW_TRACEPOINT_OPEN:
> > bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
> > break;
> > default:
> > break;
> > }
> > return 0;
> > }
> >
> > The reason is that when accessing a field in a union, such as bpf_attr,
> > if the field is located within a nested struct that is not the first
> > member of the union, it can result in incorrect field verification.
> >
> > union bpf_attr {
> > struct {
> > __u32 map_type; <<<< Actually it will find that field.
> > __u32 key_size;
> > __u32 value_size;
> > ...
> > };
> > ...
> > struct {
> > __u64 name; <<<< We want to verify this field.
> > __u32 prog_fd;
> > } raw_tracepoint;
> > };
> >
> > Considering the potential deep nesting levels, finding a perfect
> > solution to address this issue has proven challenging. Therefore, I
> > propose a solution where we simply skip the verification process if the
> > field in question is located within a union.
> >
> > Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> > kernel/bpf/btf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index fae6fc24a845..a542760c807a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6368,7 +6368,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> > * that also allows using an array of int as a scratch
> > * space. e.g. skb->cb[].
> > */
> > - if (off + size > mtrue_end) {
> > + if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {
>
> The selftest for this condition is missing.
Will add it.
--
Regards
Yafang
^ permalink raw reply [flat|nested] 10+ messages in thread