[RFC,bpf-next,v4,2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types.

Commit Message

Kui-Feng Lee Feb. 2, 2024, 10:05 p.m. UTC

From: Kui-Feng Lee <[e-mail]>

The verifier calls btf_struct_access() to check the access for
PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
(including union). We add the support of scalar types and array types.

btf_reloc_array_access() is responsible for relocating the access from the
whole array to an element in the array. That means to adjust the offset
relatively to the start of an element and change the type to the type of
the element. With this relocation, we can check the access against the
element type instead of the array type itself.

After relocation, the struct types, including union types, will continue
the loop of btf_struct_walk(). Other types are treated as scalar types,
including pointers, and return from btf_struct_access().

Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Martin KaFai Lau Feb. 3, 2024, 12:52 a.m. UTC | #1

On 2/2/24 2:05 PM, [e-mail] wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> The verifier calls btf_struct_access() to check the access for
> PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
> (including union). We add the support of scalar types and array types.
> 
> btf_reloc_array_access() is responsible for relocating the access from the
> whole array to an element in the array. That means to adjust the offset
> relatively to the start of an element and change the type to the type of
> the element. With this relocation, we can check the access against the
> element type instead of the array type itself.
> 
> After relocation, the struct types, including union types, will continue
> the loop of btf_struct_walk(). Other types are treated as scalar types,
> including pointers, and return from btf_struct_access().

Unless there is an immediate use case to support PTR_MAYBE_NULL on a non-struct 
pointer, I would suggest to separate the other pointer type support from the 
current PTR_MAYBE_NULL feature patchset. afaik, they are orthogonal.

> 
> Signed-off-by: Kui-Feng Lee <[e-mail]>
> ---
>   kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 0847035bba99..d3f94d04c69d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
>   	return -EINVAL;
>   }
>   
> +/* Relocate the access relatively to the beginning of an element in an
> + * array.
> + *
> + * The offset is adjusted relatively to the beginning of the element and the
> + * type is adjusted to the type of the element.
> + *
> + * Return NULL for scalar, enum, and pointer type.
> + * Return a btf_type pointer for struct and union.
> + */
> +static const struct btf_type *
> +btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf *btf,
> +		       const struct btf_type *t, int *off, int size)
> +{
> +	const struct btf_type *rt, *elem_type;
> +	u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
> +	u32 elem_idx;
> +
> +	rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
> +				&total_nelems, &rt_id);
> +	if (IS_ERR(rt))
> +		return rt;
> +	if (btf_type_is_array(rt)) {
> +		if (*off >= rt_size) {
> +			bpf_log(log, "access out of range of type %s with offset %d and size %u\n",
> +				__btf_name_by_offset(btf, t->name_off), *off, rt_size);
> +			return ERR_PTR(-EACCES);
> +		}
> +
> +		/* Multi-dimensional arrays are flattened by
> +		 * __btf_resolve_size(). Check the comment in
> +		 * btf_struct_walk().
> +		 */
> +		elem_size = rt_size / total_nelems;
> +		elem_idx = *off / elem_size;
> +		/* Relocate the offset relatively to the start of the
> +		 * element at elem_idx.
> +		 */
> +		*off -= elem_idx * elem_size;
> +		rt = elem_type;
> +		rt_size = elem_size;
> +	}
> +
> +	if (btf_type_is_struct(rt))
> +		return rt;
> +
> +	if (*off + size > rt_size) {
> +		bpf_log(log, "access beyond the range of type %s with offset %d and size %d\n",
> +			__btf_name_by_offset(btf, rt->name_off), *off, size);
> +		return ERR_PTR(-EACCES);
> +	}
> +
> +	/* The access is accepted as a scalar. */
> +	return NULL;
> +}
> +
>   int btf_struct_access(struct bpf_verifier_log *log,
>   		      const struct bpf_reg_state *reg,
>   		      int off, int size, enum bpf_access_type atype __maybe_unused,
> @@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log *log,
>   	}
>   
>   	t = btf_type_by_id(btf, id);
> +	t = btf_reloc_array_access(log, btf, t, &off, size);
> +	if (IS_ERR(t))
> +		return PTR_ERR(t);
> +	if (!t)
> +		return SCALAR_VALUE;
> +
>   	do {
>   		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
>

Kui-Feng Lee Feb. 3, 2024, 1:03 a.m. UTC | #2

On 2/2/24 16:52, Martin KaFai Lau wrote:
> On 2/2/24 2:05 PM, [e-mail] wrote:
>> From: Kui-Feng Lee <[e-mail]>
>>
>> The verifier calls btf_struct_access() to check the access for
>> PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
>> (including union). We add the support of scalar types and array types.
>>
>> btf_reloc_array_access() is responsible for relocating the access from 
>> the
>> whole array to an element in the array. That means to adjust the offset
>> relatively to the start of an element and change the type to the type of
>> the element. With this relocation, we can check the access against the
>> element type instead of the array type itself.
>>
>> After relocation, the struct types, including union types, will continue
>> the loop of btf_struct_walk(). Other types are treated as scalar types,
>> including pointers, and return from btf_struct_access().
> 
> Unless there is an immediate use case to support PTR_MAYBE_NULL on a 
> non-struct pointer, I would suggest to separate the other pointer type 
> support from the current PTR_MAYBE_NULL feature patchset. afaik, they 
> are orthogonal.


Totally agree!


> 
>>
>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>> ---
>>   kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 0847035bba99..d3f94d04c69d 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct 
>> bpf_verifier_log *log, const struct btf *btf,
>>       return -EINVAL;
>>   }
>> +/* Relocate the access relatively to the beginning of an element in an
>> + * array.
>> + *
>> + * The offset is adjusted relatively to the beginning of the element 
>> and the
>> + * type is adjusted to the type of the element.
>> + *
>> + * Return NULL for scalar, enum, and pointer type.
>> + * Return a btf_type pointer for struct and union.
>> + */
>> +static const struct btf_type *
>> +btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf 
>> *btf,
>> +               const struct btf_type *t, int *off, int size)
>> +{
>> +    const struct btf_type *rt, *elem_type;
>> +    u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
>> +    u32 elem_idx;
>> +
>> +    rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
>> +                &total_nelems, &rt_id);
>> +    if (IS_ERR(rt))
>> +        return rt;
>> +    if (btf_type_is_array(rt)) {
>> +        if (*off >= rt_size) {
>> +            bpf_log(log, "access out of range of type %s with offset 
>> %d and size %u\n",
>> +                __btf_name_by_offset(btf, t->name_off), *off, rt_size);
>> +            return ERR_PTR(-EACCES);
>> +        }
>> +
>> +        /* Multi-dimensional arrays are flattened by
>> +         * __btf_resolve_size(). Check the comment in
>> +         * btf_struct_walk().
>> +         */
>> +        elem_size = rt_size / total_nelems;
>> +        elem_idx = *off / elem_size;
>> +        /* Relocate the offset relatively to the start of the
>> +         * element at elem_idx.
>> +         */
>> +        *off -= elem_idx * elem_size;
>> +        rt = elem_type;
>> +        rt_size = elem_size;
>> +    }
>> +
>> +    if (btf_type_is_struct(rt))
>> +        return rt;
>> +
>> +    if (*off + size > rt_size) {
>> +        bpf_log(log, "access beyond the range of type %s with offset 
>> %d and size %d\n",
>> +            __btf_name_by_offset(btf, rt->name_off), *off, size);
>> +        return ERR_PTR(-EACCES);
>> +    }
>> +
>> +    /* The access is accepted as a scalar. */
>> +    return NULL;
>> +}
>> +
>>   int btf_struct_access(struct bpf_verifier_log *log,
>>                 const struct bpf_reg_state *reg,
>>                 int off, int size, enum bpf_access_type atype 
>> __maybe_unused,
>> @@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log 
>> *log,
>>       }
>>       t = btf_type_by_id(btf, id);
>> +    t = btf_reloc_array_access(log, btf, t, &off, size);
>> +    if (IS_ERR(t))
>> +        return PTR_ERR(t);
>> +    if (!t)
>> +        return SCALAR_VALUE;
>> +
>>       do {
>>           err = btf_struct_walk(log, btf, t, off, size, &id, 
>> &tmp_flag, field_name);
>

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 0847035bba99..d3f94d04c69d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6590,6 +6590,61 @@  static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 	return -EINVAL;
 }
 
+/* Relocate the access relatively to the beginning of an element in an
+ * array.
+ *
+ * The offset is adjusted relatively to the beginning of the element and the
+ * type is adjusted to the type of the element.
+ *
+ * Return NULL for scalar, enum, and pointer type.
+ * Return a btf_type pointer for struct and union.
+ */
+static const struct btf_type *
+btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf *btf,
+		       const struct btf_type *t, int *off, int size)
+{
+	const struct btf_type *rt, *elem_type;
+	u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
+	u32 elem_idx;
+
+	rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
+				&total_nelems, &rt_id);
+	if (IS_ERR(rt))
+		return rt;
+	if (btf_type_is_array(rt)) {
+		if (*off >= rt_size) {
+			bpf_log(log, "access out of range of type %s with offset %d and size %u\n",
+				__btf_name_by_offset(btf, t->name_off), *off, rt_size);
+			return ERR_PTR(-EACCES);
+		}
+
+		/* Multi-dimensional arrays are flattened by
+		 * __btf_resolve_size(). Check the comment in
+		 * btf_struct_walk().
+		 */
+		elem_size = rt_size / total_nelems;
+		elem_idx = *off / elem_size;
+		/* Relocate the offset relatively to the start of the
+		 * element at elem_idx.
+		 */
+		*off -= elem_idx * elem_size;
+		rt = elem_type;
+		rt_size = elem_size;
+	}
+
+	if (btf_type_is_struct(rt))
+		return rt;
+
+	if (*off + size > rt_size) {
+		bpf_log(log, "access beyond the range of type %s with offset %d and size %d\n",
+			__btf_name_by_offset(btf, rt->name_off), *off, size);
+		return ERR_PTR(-EACCES);
+	}
+
+	/* The access is accepted as a scalar. */
+	return NULL;
+}
+
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct bpf_reg_state *reg,
 		      int off, int size, enum bpf_access_type atype __maybe_unused,
@@ -6625,6 +6680,12 @@  int btf_struct_access(struct bpf_verifier_log *log,
 	}
 
 	t = btf_type_by_id(btf, id);
+	t = btf_reloc_array_access(log, btf, t, &off, size);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	if (!t)
+		return SCALAR_VALUE;
+
 	do {
 		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);