diff mbox series

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

Message ID 20240905134813.874-1-daniel@iogearbox.net (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v3,1/6] bpf: Fix helper writes to read-only maps | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format warning Series does not have a cover letter
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 226 this patch: 226
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 18 maintainers not CCed: edumazet@google.com sdf@fomichev.me eddyz87@gmail.com haoluo@google.com mattbobrowski@google.com rostedt@goodmis.org mhiramat@kernel.org linux-trace-kernel@vger.kernel.org mathieu.desnoyers@efficios.com kpsingh@kernel.org song@kernel.org martin.lau@linux.dev john.fastabend@gmail.com pabeni@redhat.com kuba@kernel.org netdev@vger.kernel.org jolsa@kernel.org yonghong.song@linux.dev
netdev/build_clang success Errors and warnings before: 291 this patch: 291
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 6867 this patch: 6867
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 22 this patch: 22
netdev/source_inline success Was 0 now: 0

Commit Message

Daniel Borkmann Sept. 5, 2024, 1:48 p.m. UTC 
Lonial found an issue that despite user- and BPF-side frozen BPF map
(like in case of .rodata), it was still possible to write into it from
a BPF program side through specific helpers having ARG_PTR_TO_{LONG,INT}
as arguments.

In check_func_arg() when the argument is as mentioned, the meta->raw_mode
is never set. Later, check_helper_mem_access(), under the case of
PTR_TO_MAP_VALUE as register base type, it assumes BPF_READ for the
subsequent call to check_map_access_type() and given the BPF map is
read-only it succeeds.

The helpers really need to be annotated as ARG_PTR_TO_{LONG,INT} | MEM_UNINIT
when results are written into them as opposed to read out of them. The
latter indicates that it's okay to pass a pointer to uninitialized memory
as the memory is written to anyway.

However, ARG_PTR_TO_{LONG,INT} is a special case of ARG_PTR_TO_FIXED_SIZE_MEM
just with additional alignment requirement. So it is better to just get
rid of the ARG_PTR_TO_{LONG,INT} special cases altogether and reuse the
fixed size memory types. For this, add MEM_ALIGNED to additionally ensure
alignment given these helpers write directly into the args via *<ptr> = val.
The .arg*_size has been initialized reflecting the actual sizeof(*<ptr>).
In some of the helpers these are long types, in others these are fixed
integer types.

MEM_ALIGNED can only be used in combination with MEM_FIXED_SIZE annotated
argument types, since in !MEM_FIXED_SIZE cases the verifier does not know
the buffer size a priori and therefore cannot blindly write *<ptr> = val.

Fixes: 57c3bb725a3d ("bpf: Introduce ARG_PTR_TO_{INT,LONG} arg types")
Reported-by: Lonial Con <kongln9170@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1 -> v2:
 - const volatile long (Andrii)

 include/linux/bpf.h      |  7 +++++--
 kernel/bpf/helpers.c     |  8 ++++++--
 kernel/bpf/syscall.c     |  4 +++-
 kernel/bpf/verifier.c    | 38 +++++---------------------------------
 kernel/trace/bpf_trace.c |  8 ++++++--
 net/core/filter.c        |  8 ++++++--
 6 files changed, 31 insertions(+), 42 deletions(-)

Comments

Alexei Starovoitov Sept. 5, 2024, 7:39 p.m. UTC | #1 
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.
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).

pw-bot: cr
Daniel Borkmann Sept. 5, 2024, 8:27 p.m. UTC | #2 
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
Daniel Borkmann Sept. 5, 2024, 10:56 p.m. UTC | #3 
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.

Last option is to have it like in this patch to reflect actual long in
.arg*_size still no change 64bit and 32bit becomes also correct, just
quirk that it's not fixed/portable size. Thoughts on the options? All ugly,
but last one looked most sane to me fwiw.

Thanks,
Daniel
Daniel Borkmann Sept. 5, 2024, 11:14 p.m. UTC | #4 
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
Alexei Starovoitov Sept. 6, 2024, 12:15 a.m. UTC | #5 
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?

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.
Daniel Borkmann Sept. 6, 2024, 7:43 a.m. UTC | #6 
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..
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6f87fb014fba..6a61ed4266b6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -695,6 +695,11 @@  enum bpf_type_flag {
 	/* DYNPTR points to xdp_buff */
 	DYNPTR_TYPE_XDP		= BIT(16 + BPF_BASE_TYPE_BITS),
 
+	/* Memory must be aligned on some architectures, used in combination with
+	 * MEM_FIXED_SIZE.
+	 */
+	MEM_ALIGNED		= BIT(17 + BPF_BASE_TYPE_BITS),
+
 	__BPF_TYPE_FLAG_MAX,
 	__BPF_TYPE_LAST_FLAG	= __BPF_TYPE_FLAG_MAX - 1,
 };
@@ -732,8 +737,6 @@  enum bpf_arg_type {
 	ARG_ANYTHING,		/* any (initialized) argument is ok */
 	ARG_PTR_TO_SPIN_LOCK,	/* pointer to bpf_spin_lock */
 	ARG_PTR_TO_SOCK_COMMON,	/* pointer to sock_common */
-	ARG_PTR_TO_INT,		/* pointer to int */
-	ARG_PTR_TO_LONG,	/* pointer to long */
 	ARG_PTR_TO_SOCKET,	/* pointer to bpf_sock (fullsock) */
 	ARG_PTR_TO_BTF_ID,	/* pointer to in-kernel struct */
 	ARG_PTR_TO_RINGBUF_MEM,	/* pointer to dynamically reserved ringbuf memory */
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),
 };
 
 BPF_CALL_3(bpf_strncmp, const char *, s1, u32, s1_sz, const char *, s2)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fc62f5c4faf9..feb276771c03 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5954,7 +5954,9 @@  static const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
 	.arg1_type	= ARG_PTR_TO_MEM,
 	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
 	.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(u64),
 };
 
 static const struct bpf_func_proto *
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 217eb0eafa2a..efd9c453399e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8306,16 +8306,6 @@  static bool arg_type_is_dynptr(enum bpf_arg_type type)
 	return base_type(type) == ARG_PTR_TO_DYNPTR;
 }
 
-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);
-
-	return -EINVAL;
-}
-
 static int resolve_map_arg_type(struct bpf_verifier_env *env,
 				 const struct bpf_call_arg_meta *meta,
 				 enum bpf_arg_type *arg_type)
@@ -8388,16 +8378,6 @@  static const struct bpf_reg_types mem_types = {
 	},
 };
 
-static const struct bpf_reg_types int_ptr_types = {
-	.types = {
-		PTR_TO_STACK,
-		PTR_TO_PACKET,
-		PTR_TO_PACKET_META,
-		PTR_TO_MAP_KEY,
-		PTR_TO_MAP_VALUE,
-	},
-};
-
 static const struct bpf_reg_types spin_lock_types = {
 	.types = {
 		PTR_TO_MAP_VALUE,
@@ -8458,8 +8438,6 @@  static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_SPIN_LOCK]		= &spin_lock_types,
 	[ARG_PTR_TO_MEM]		= &mem_types,
 	[ARG_PTR_TO_RINGBUF_MEM]	= &ringbuf_mem_types,
-	[ARG_PTR_TO_INT]		= &int_ptr_types,
-	[ARG_PTR_TO_LONG]		= &int_ptr_types,
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
 	[ARG_PTR_TO_FUNC]		= &func_ptr_types,
 	[ARG_PTR_TO_STACK]		= &stack_ptr_types,
@@ -9025,6 +9003,11 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			err = check_helper_mem_access(env, regno,
 						      fn->arg_size[arg], false,
 						      meta);
+			if (err)
+				return err;
+			if (arg_type & MEM_ALIGNED)
+				err = check_ptr_alignment(env, reg, 0,
+							  fn->arg_size[arg], true);
 		}
 		break;
 	case ARG_CONST_SIZE:
@@ -9049,17 +9032,6 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		if (err)
 			return err;
 		break;
-	case ARG_PTR_TO_INT:
-	case ARG_PTR_TO_LONG:
-	{
-		int size = int_ptr_type_to_size(arg_type);
-
-		err = check_helper_mem_access(env, regno, size, false, meta);
-		if (err)
-			return err;
-		err = check_ptr_alignment(env, reg, 0, size, true);
-		break;
-	}
 	case ARG_PTR_TO_CONST_STR:
 	{
 		err = check_reg_const_str(env, reg, regno);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b69a39316c0c..dbf4dff33146 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1202,7 +1202,9 @@  static const struct bpf_func_proto bpf_get_func_arg_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_CTX,
 	.arg2_type	= ARG_ANYTHING,
-	.arg3_type	= ARG_PTR_TO_LONG,
+	.arg3_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u64),
 };
 
 BPF_CALL_2(get_func_ret, void *, ctx, u64 *, value)
@@ -1218,7 +1220,9 @@  static const struct bpf_func_proto bpf_get_func_ret_proto = {
 	.func		= get_func_ret,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_CTX,
-	.arg2_type	= ARG_PTR_TO_LONG,
+	.arg2_type	= ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg2_size	= sizeof(u64),
 };
 
 BPF_CALL_1(get_func_arg_cnt, void *, ctx)
diff --git a/net/core/filter.c b/net/core/filter.c
index ecf2ddf633bf..4be175f84eb9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6346,7 +6346,9 @@  static const struct bpf_func_proto bpf_skb_check_mtu_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type      = ARG_PTR_TO_CTX,
 	.arg2_type      = ARG_ANYTHING,
-	.arg3_type      = ARG_PTR_TO_INT,
+	.arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u32),
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };
@@ -6357,7 +6359,9 @@  static const struct bpf_func_proto bpf_xdp_check_mtu_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type      = ARG_PTR_TO_CTX,
 	.arg2_type      = ARG_ANYTHING,
-	.arg3_type      = ARG_PTR_TO_INT,
+	.arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
+			  MEM_UNINIT | MEM_ALIGNED,
+	.arg3_size	= sizeof(u32),
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };