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?
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)).
