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 15:13, Kui-Feng Lee wrote:
> 
> 
> 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.
> 

According to the off-line discussion, the changes for unsupported_ops[]
should be in a separate patchset. The check of (void
**)(st_ops->cfi_stubs + moff)) will be removed. Changes of check_member
should be removed as well.