netdev - Re: [PATCH bpf-next 3/7] bpf: btf: Check array->index

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a1016774-1604-f230-5eea-95656e01f692@fb.com>
Date:   Mon, 21 May 2018 21:41:11 -0700
From:   Yonghong Song <yhs@...com>
To:     Martin KaFai Lau <kafai@...com>, <netdev@...r.kernel.org>
CC:     Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>
Subject: Re: [PATCH bpf-next 3/7] bpf: btf: Check array->index_type



On 5/18/18 5:16 PM, Martin KaFai Lau wrote:
> Instead of ingoring the array->index_type field.  Enforce that
> it must be an unsigned BTF_KIND_INT.
> 
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
>   kernel/bpf/btf.c | 83 ++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 59 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 536e5981ad8c..b4e48dae2240 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -444,6 +444,28 @@ static const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id)
>   	return btf->types[type_id];
>   }
>   
> +/*
> + * Regular int is not a bit field and it must be either
> + * u8/u16/u32/u64.
> + */
> +static bool btf_type_int_is_regular(const struct btf_type *t)
> +{
> +	u16 nr_bits, nr_bytes;
> +	u32 int_data;
> +
> +	int_data = btf_type_int(t);
> +	nr_bits = BTF_INT_BITS(int_data);
> +	nr_bytes = BITS_ROUNDUP_BYTES(nr_bits);
> +	if (BITS_PER_BYTE_MASKED(nr_bits) ||
> +	    BTF_INT_OFFSET(int_data) ||
> +	    (nr_bytes != sizeof(u8) && nr_bytes != sizeof(u16) &&
> +	     nr_bytes != sizeof(u32) && nr_bytes != sizeof(u64))) {
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>   __printf(2, 3) static void __btf_verifier_log(struct bpf_verifier_log *log,
>   					      const char *fmt, ...)
>   {
> @@ -1309,14 +1331,16 @@ static s32 btf_array_check_meta(struct btf_verifier_env *env,
>   		return -EINVAL;
>   	}
>   
> -	/* We are a little forgiving on array->index_type since
> -	 * the kernel is not using it.
> -	 */
> -	/* Array elem cannot be in type void,
> -	 * so !array->type is not allowed.
> +	/* Array elem type and index type cannot be in type void,
> +	 * so !array->type and !array->index_type are not allowed.
>   	 */
>   	if (!array->type || BTF_TYPE_PARENT(array->type)) {
> -		btf_verifier_log_type(env, t, "Invalid type_id");
> +		btf_verifier_log_type(env, t, "Invalid elem");
> +		return -EINVAL;
> +	}
> +
> +	if (!array->index_type || BTF_TYPE_PARENT(array->index_type)) {
> +		btf_verifier_log_type(env, t, "Invalid index");
>   		return -EINVAL;
>   	}
>   
> @@ -1329,11 +1353,35 @@ static int btf_array_resolve(struct btf_verifier_env *env,
>   			     const struct resolve_vertex *v)
>   {
>   	const struct btf_array *array = btf_type_array(v->t);
> -	const struct btf_type *elem_type;
> -	u32 elem_type_id = array->type;
> +	const struct btf_type *elem_type, *index_type;
> +	u32 elem_type_id, index_type_id;
>   	struct btf *btf = env->btf;
>   	u32 elem_size;
>   
> +	/* Check array->index_type */
> +	index_type_id = array->index_type;
> +	index_type = btf_type_by_id(btf, index_type_id);
> +	if (btf_type_is_void_or_null(index_type)) {
> +		btf_verifier_log_type(env, v->t, "Invalid index");
> +		return -EINVAL;
> +	}
> +
> +	if (!env_type_is_resolve_sink(env, index_type) &&
> +	    !env_type_is_resolved(env, index_type_id))
> +		return env_stack_push(env, index_type, index_type_id);
> +
> +	index_type = btf_type_id_size(btf, &index_type_id, NULL);
> +	if (!index_type || !btf_type_is_int(index_type) ||
> +	    /* bit field int is not allowed */
> +	    !btf_type_int_is_regular(index_type) ||
> +	    /* unsigned only */
> +	    BTF_INT_ENCODING(btf_type_int(index_type))) {

Could you explain why you only support array index type to be
unsigned? A lot of test cases  in Patch #7 are amended with unsigned 
types. In C, signed integers can surely be index, e.g., a[-1].

> +		btf_verifier_log_type(env, v->t, "Invalid index");
> +		return -EINVAL;
> +	}
> +
> +	/* Check array->type */
> +	elem_type_id = array->type;
>   	elem_type = btf_type_by_id(btf, elem_type_id);
>   	if (btf_type_is_void_or_null(elem_type)) {
>   		btf_verifier_log_type(env, v->t,
> @@ -1351,22 +1399,9 @@ static int btf_array_resolve(struct btf_verifier_env *env,
>   		return -EINVAL;
>   	}
>   
> -	if (btf_type_is_int(elem_type)) {
> -		int int_type_data = btf_type_int(elem_type);
> -		u16 nr_bits = BTF_INT_BITS(int_type_data);
> -		u16 nr_bytes = BITS_ROUNDUP_BYTES(nr_bits);
> -
> -		/* Put more restriction on array of int.  The int cannot
> -		 * be a bit field and it must be either u8/u16/u32/u64.
> -		 */
> -		if (BITS_PER_BYTE_MASKED(nr_bits) ||
> -		    BTF_INT_OFFSET(int_type_data) ||
> -		    (nr_bytes != sizeof(u8) && nr_bytes != sizeof(u16) &&
> -		     nr_bytes != sizeof(u32) && nr_bytes != sizeof(u64))) {
> -			btf_verifier_log_type(env, v->t,
> -					      "Invalid array of int");
> -			return -EINVAL;
> -		}
> +	if (btf_type_is_int(elem_type) && !btf_type_int_is_regular(elem_type)) {
> +		btf_verifier_log_type(env, v->t, "Invalid array of int");
> +		return -EINVAL;
>   	}
>   
>   	if (array->nelems && elem_size > U32_MAX / array->nelems) {
>