Re: [PATCH bpf-next] bpf: Fix error checks against bpf_get_btf_vmlinux().

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

On 1/25/24 16:54, Martin KaFai Lau wrote:
> On 1/25/24 3:31 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Check whether the returned pointer is NULL. Previously, it was assumed 
>> that
>> an error code would be returned if BTF is not available or fails to
>> parse. However, it actually returns NULL if BTF is disabled.
>>
>> In the function check_struct_ops_btf_id(), we have stopped using
>> btf_vmlinux as a backup because attach_btf is never null when 
>> attach_btf_id
>> is set. However, the function test_libbpf_probe_prog_types() in
>> libbpf_probes.c does not set both attach_btf_obj_fd and attach_btf_id,
>> resulting in attach_btf being null, and it expects ENOTSUPP as a
>> result. So, if attach_btf_id is not set, it will return ENOTSUPP.
>>
>> Reported-by: syzbot+88f0aafe5f950d7489d7@syzkaller.appspotmail.com
>> Closes: 
>> https://lore.kernel.org/bpf/00000000000040d68a060fc8db8c@google.com/
> 
> There were two different syzbot report. Both should be tagged here as 
> Reported-by.

Sure!

> 
>> Fixes: fcc2c1fb0651 ("bpf: pass attached BTF to the bpf_struct_ops 
>> subsystem")
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 2 ++
>>   kernel/bpf/verifier.c       | 8 +++++++-
>>   2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index defc052e4622..0decd862dfe0 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -669,6 +669,8 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>           btf = bpf_get_btf_vmlinux();
>>           if (IS_ERR(btf))
>>               return ERR_CAST(btf);
>> +        if (!btf)
>> +            return ERR_PTR(-ENOTSUPP);
>>       }
>>       st_ops_desc = bpf_struct_ops_find_value(btf, 
>> attr->btf_vmlinux_value_type_id);
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index fe833e831cb6..64a927784c54 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20298,7 +20298,13 @@ static int check_struct_ops_btf_id(struct 
>> bpf_verifier_env *env)
>>           return -EINVAL;
>>       }
>> -    btf = prog->aux->attach_btf ?: bpf_get_btf_vmlinux();
>> +    if (!prog->aux->attach_btf_id)
>> +        return -ENOTSUPP;
>> +
>> +    btf = prog->aux->attach_btf;
>> +    if (!btf)
> 
> The commit message mentioned "attach_btf is never null when 
> attach_btf_id is set". Then why this test is still needed when the above 
> has just tested the attach_btf_id. attach_btf must be valid here as long 
> as attach_btf_id is set. This should have been guaranteed by syscall.c, no?

Yes, you are right.

> 
>> +        return -ENOTSUPP;
>> +
>>       if (btf_is_module(btf)) {
>>           /* Make sure st_ops is valid through the lifetime of env */
>>           env->attach_btf_mod = btf_try_get_module(btf);
>