Re: [PATCH bpf-next v4 2/3] bpf: Check cfi_stubs before registering a struct_ops type.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/21/24 10:25, Martin KaFai Lau wrote:
> On 2/20/24 11:52 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> 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 <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 0d7be97a2411..c1c502caae08 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("struct %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) {
>> @@ -339,6 +344,7 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>       for_each_member(i, t, member) {
>>           const struct btf_type *func_proto;
>> +        u32 moff;
>>           mname = btf_name_by_offset(btf, member->name_off);
>>           if (!*mname) {
>> @@ -361,6 +367,17 @@ int bpf_struct_ops_desc_init(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>           if (!func_proto)
>>               continue;
>> +        moff = __btf_member_bit_offset(t, member) / 8;
>> +        err = st_ops->check_member ?
>> +            st_ops->check_member(t, member, NULL) : 0;
> 
> I don't think it is necessary to make check_member more complicated by 
> taking
> NULL prog. The struct_ops implementer then needs to handle this extra NULL
> prog case.
> 
> Have you thought about Alexei's earlier suggestion in v3 to reuse the NULL
> member in cfi_stubs to flag unsupported member and remove the 
> unsupported_ops[]
> from bpf_tcp_ca.c?
> 
> If the verifier can consistently reject loading unsupported bpf prog, it 
> will
> not reach the bpf_struct_ops_map_update_elem and then hits the NULL member
> in cfi_stubs during map_update_elem.
> 

Ok! I misunderstood previously. I will go this way.