Re: [PATCH bpf-next v3 1/6] bpf: Fix helper writes to read-only maps

[Daniel Borkmann <daniel@iogearbox.net>]

On 9/5/24 9:39 PM, Alexei Starovoitov wrote:
> On Thu, Sep 5, 2024 at 6:48 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 3956be5d6440..d2c8945e8297 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -539,7 +539,9 @@ const struct bpf_func_proto bpf_strtol_proto = {
>>          .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
>>          .arg2_type      = ARG_CONST_SIZE,
>>          .arg3_type      = ARG_ANYTHING,
>> -       .arg4_type      = ARG_PTR_TO_LONG,
>> +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
>> +                         MEM_UNINIT | MEM_ALIGNED,
>> +       .arg4_size      = sizeof(long),
>>   };
>>
>>   BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
>> @@ -567,7 +569,9 @@ const struct bpf_func_proto bpf_strtoul_proto = {
>>          .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
>>          .arg2_type      = ARG_CONST_SIZE,
>>          .arg3_type      = ARG_ANYTHING,
>> -       .arg4_type      = ARG_PTR_TO_LONG,
>> +       .arg4_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
>> +                         MEM_UNINIT | MEM_ALIGNED,
>> +       .arg4_size      = sizeof(unsigned long),
> 
> This is not correct.
> ARG_PTR_TO_LONG is bpf-side "long", not kernel side "long".
> 
>> -static int int_ptr_type_to_size(enum bpf_arg_type type)
>> -{
>> -       if (type == ARG_PTR_TO_INT)
>> -               return sizeof(u32);
>> -       else if (type == ARG_PTR_TO_LONG)
>> -               return sizeof(u64);
> 
> as seen here.
> 
> BPF_CALL_4(bpf_strto[u]l, ... long *, res)
> are buggy.

Right, the int_ptr_type_to_size() checks mem based on u64 vs writing
long in the helper which mismatches on 32bit archs.

> but they call __bpf_strtoll which takes 'long long' correctly.
> 
> The fix for BPF_CALL_4(bpf_strto[u]l and uapi/bpf.h is orthogonal,
> but this patch shouldn't make the verifier see it as sizeof(long).

Ok, so I'll fix the BPF_CALL signatures for the affected helpers as
one more patch and also align arg*_size to {s,u}64 then so that there's
no mismatch.

Thanks,
Daniel