Re: [PATCH bpf-next 1/3] bpf: Introduce BTF_TYPE_SAFE_TRUSTED_UNION

[Stanislav Fomichev <sdf@google.com>]

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?