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

[Daniel Borkmann <daniel@iogearbox.net>]

On 9/6/24 2:15 AM, Alexei Starovoitov wrote:
> On Thu, Sep 5, 2024 at 4:14 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> 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).
> 
> Right. From bpf side 'long *' is ok-sh and it's ok to stay this way
> in uapi/bpf.h and from there in bpf_helper_defs.h,
> but BPF_CALL(bpf_strol..) needs to change.
> And if we fix that we should probably change uapi/bpf.h to stay consistent.
> Maybe we should use 'u64 *' everywhere then?

I'd love to also change uapi/bpf.h, just that this needs changes in existing
BPF selftests as otherwise the build errors out e.g. on regular x86-64 with
the following (without the -Werror this is 'just' a warning):

   [...]
   progs/verifier_const.c:28:36: error: incompatible pointer types passing 'long *' to parameter of type '__s64 *' (aka 'long long *') [-Werror,-Wincompatible-pointer-types]
      28 |         bpf_strtol(buff, sizeof(buff), 0, &bar);
         |                                           ^~~~
   [...]

One could argue that these two helpers are still fairly niche, but otoh,
they've been around since 2019. :/ So I guess it's probably better to stay
with the uapi/bpf.h inconsistency that they remain at {unsigned,} long
instead of __{u,s}64 such that user code does not need to be adapted to the
signature change.

> On 32-bit archs bpf_strtol helpers were broken,
> since they were converting string to 'long long', but assigning
> result into 32-bit 'long *',
> so upper bits will be seen as uninited from bpf prog pov.
> This series are fixing the error path of uninit, but looks like
> non-error path was broken on 32-bit archs too.
> 
> Thankfully bpf_strto[u]l are the only helpers that take 'long *'.
> Other helpers use 'u64 *' in similar situations.

Agree, thankfully just those which slipped through..