From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yonghong Song Subject: Re: [PATCH bpf-next 3/7] bpf: btf: Check array->index_type Date: Mon, 21 May 2018 21:41:11 -0700 Message-ID: References: <20180519001650.4043980-1-kafai@fb.com> <20180519001650.4043980-4-kafai@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Alexei Starovoitov , Daniel Borkmann , To: Martin KaFai Lau , Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:50690 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750979AbeEVEmA (ORCPT ); Tue, 22 May 2018 00:42:00 -0400 In-Reply-To: <20180519001650.4043980-4-kafai@fb.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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) { >