[bpf-next] bpf: Check cfi_stubs before registering a struct_ops type.

Commit Message

Kui-Feng Lee Feb. 15, 2024, 2:24 a.m. UTC

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

Recently, cfi_stubs were introduced. However, existing struct_ops types
that are not in the upstream may not be aware of this, resulting in kernel
crashes. By rejecting struct_ops types that do not provide cfi_stubs during
registration, these crashes can be avoided.

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

Comments

Martin KaFai Lau Feb. 15, 2024, 6:23 p.m. UTC | #1

On 2/14/24 6:24 PM, [e-mail] wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> Recently, cfi_stubs were introduced. However, existing struct_ops types
> that are not in the upstream may not be aware of this, resulting in kernel
> crashes. By rejecting struct_ops types that do not provide cfi_stubs during
> registration, these crashes can be avoided.
> 
> Signed-off-by: Kui-Feng Lee <[e-mail]>
> ---
>   kernel/bpf/bpf_struct_ops.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 0d7be97a2411..e35958142dce 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>   	}
>   	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>   
> +	if (!st_ops->cfi_stubs) {

How about *(void **)(st_ops->cfi_stubs + moff) ? Does it need a NULL check?

Please add a test.

> +		pr_warn("The struct_ops %s has no cfi_stubs\n", st_ops->name);
> +		return -EINVAL;
> +	}
> +
>   	type_id = btf_find_by_name_kind(btf, st_ops->name,
>   					BTF_KIND_STRUCT);
>   	if (type_id < 0) {

Kui-Feng Lee Feb. 15, 2024, 7:19 p.m. UTC | #2

On 2/15/24 10:23, Martin KaFai Lau wrote:
> On 2/14/24 6:24 PM, [e-mail] wrote:
>> From: Kui-Feng Lee <[e-mail]>
>>
>> Recently, cfi_stubs were introduced. However, existing struct_ops types
>> that are not in the upstream may not be aware of this, resulting in 
>> kernel
>> crashes. By rejecting struct_ops types that do not provide cfi_stubs 
>> during
>> registration, these crashes can be avoided.
>>
>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 0d7be97a2411..e35958142dce 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>       }
>>       sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>> +    if (!st_ops->cfi_stubs) {
> 
> How about *(void **)(st_ops->cfi_stubs + moff) ? Does it need a NULL check?

This NULL check is necessary to prevent the crash but good to have.

> 
> Please add a test.

Got it!

> 
>> +        pr_warn("The struct_ops %s has no cfi_stubs\n", st_ops->name);
>> +        return -EINVAL;
>> +    }
>> +
>>       type_id = btf_find_by_name_kind(btf, st_ops->name,
>>                       BTF_KIND_STRUCT);
>>       if (type_id < 0) {
>

Kui-Feng Lee Feb. 15, 2024, 7:20 p.m. UTC | #3

On 2/15/24 11:19, Kui-Feng Lee wrote:
> 
> 
> On 2/15/24 10:23, Martin KaFai Lau wrote:
>> On 2/14/24 6:24 PM, [e-mail] wrote:
>>> From: Kui-Feng Lee <[e-mail]>
>>>
>>> Recently, cfi_stubs were introduced. However, existing struct_ops types
>>> that are not in the upstream may not be aware of this, resulting in 
>>> kernel
>>> crashes. By rejecting struct_ops types that do not provide cfi_stubs 
>>> during
>>> registration, these crashes can be avoided.
>>>
>>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>>> ---
>>>   kernel/bpf/bpf_struct_ops.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 0d7be97a2411..e35958142dce 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -302,6 +302,11 @@ int bpf_struct_ops_desc_init(struct 
>>> bpf_struct_ops_desc *st_ops_desc,
>>>       }
>>>       sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>>> +    if (!st_ops->cfi_stubs) {
>>
>> How about *(void **)(st_ops->cfi_stubs + moff) ? Does it need a NULL 
>> check?
> 
> This NULL check is necessary to prevent the crash but good to have.
                     ^^^ I mean "not necessary"
> 
>>
>> Please add a test.
> 
> Got it!
> 
>>
>>> +        pr_warn("The struct_ops %s has no cfi_stubs\n", st_ops->name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       type_id = btf_find_by_name_kind(btf, st_ops->name,
>>>                       BTF_KIND_STRUCT);
>>>       if (type_id < 0) {
>>

Patch

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 0d7be97a2411..e35958142dce 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -302,6 +302,11 @@  int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
 	}
 	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
+	if (!st_ops->cfi_stubs) {
+		pr_warn("The struct_ops %s has no cfi_stubs\n", st_ops->name);
+		return -EINVAL;
+	}
+
 	type_id = btf_find_by_name_kind(btf, st_ops->name,
 					BTF_KIND_STRUCT);
 	if (type_id < 0) {