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

[Daniel Borkmann <daniel@iogearbox.net>]

On 9/6/24 12:56 AM, Daniel Borkmann wrote:
> On 9/5/24 10:27 PM, Daniel Borkmann wrote:
>> 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.
> 
> Not fixing up BPF_CALL signatures but aligning .arg*_size to sizeof(u64)
> would fwiw keep things as today. This has the downside that on 32bit archs
> one could end up leaking out 4b of uninit mem (as verifier assumes fixed
> 64bit and in case of write there is no need to init the var as verifier
> thinks the helper will fill it all). Ugly bit is the func proto is enabled
> in bpf_base_func_proto() which is ofc available for unpriv (if someone
> actually has it turned on..).
> 
> Fixing up BPF_CALL signatures for bpf_strto{u,}l where res pointer becomes
> {s,u}64 and .arg*_size fixed 8b, would be nicer, but assuming this includes
> also the uapi helper description, then we'll also have to end up adapting
> selftests (given compiler warns on ptr type mismatch) :/
> 
> One option could be we fix up BPF_CALL sites, but not the uapi helper such
> that selftests stay as they are. For 64bit no change, but 32bit archs this
> will be subtle as we write beyond the passed/expected long inside the helper.

Nevermind, scratch the incorrect last part, only this option would do the
trick since from bpf pov its bpf-side "long" (as its unchanged in the uapi
header which gets pulled into the prog).

Thanks,
Daniel