On Thu, Sep 5, 2024 at 4:14 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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?
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.
