diff mbox series

[bpf,1/4] bpf: Fix helper writes to read-only maps

Message ID 20240823222033.31006-1-daniel@iogearbox.net (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf,1/4] bpf: Fix helper writes to read-only maps | expand

Checks

Context Check Description
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-4 fail Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test
bpf/vmtest-bpf-VM_Test-8 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for s390x-gcc / test
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-12 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-16 success Logs for x86_64-gcc / veristat
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-22 fail Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-18 fail Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-llvm-18 / test
bpf/vmtest-bpf-VM_Test-14 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-17 fail Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-21 fail Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-llvm-17 / test
bpf/vmtest-bpf-VM_Test-15 success Logs for x86_64-gcc / test
bpf/vmtest-bpf-VM_Test-13 fail Logs for x86_64-gcc / build / build for x86_64 with gcc
netdev/series_format warning Series does not have a cover letter
netdev/tree_selection success Clearly marked for bpf, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 2 blamed authors not CCed: ast@kernel.org rdna@fb.com; 21 maintainers not CCed: andrii@kernel.org 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 rdna@fb.com mathieu.desnoyers@efficios.com song@kernel.org ast@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 kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 16 this patch: 16
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: 122 this patch: 122
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 66 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 15 this patch: 15
netdev/source_inline success Was 0 now: 0

Commit Message

Daniel Borkmann Aug. 23, 2024, 10:20 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.

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>
---
 kernel/bpf/helpers.c     | 4 ++--
 kernel/bpf/syscall.c     | 2 +-
 kernel/bpf/verifier.c    | 3 ++-
 kernel/trace/bpf_trace.c | 4 ++--
 net/core/filter.c        | 4 ++--
 5 files changed, 9 insertions(+), 8 deletions(-)

Comments

Shung-Hsi Yu Aug. 26, 2024, 6:37 a.m. UTC | #1
On Sat, Aug 24, 2024 at 12:20:30AM GMT, Daniel Borkmann wrote:
> 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.
> 
> 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>
[...]

check_raw_mode_ok() might need an update as well since it currently does
not take ARG_PTR_TO_{LONG,INT} | MEM_UNINIT into account.

Aside from that LGTM (for this patch).

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>


As a future refactoring it seems like we'd be better off turning
ARG_PTR_TO_{LONG,INT} into the more generalized
ARG_PTR_TO_FIXED_SIZE_MEM?
Andrii Nakryiko Aug. 27, 2024, 10:37 p.m. UTC | #2
On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> 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.
>
> 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>
> ---
>  kernel/bpf/helpers.c     | 4 ++--
>  kernel/bpf/syscall.c     | 2 +-
>  kernel/bpf/verifier.c    | 3 ++-
>  kernel/trace/bpf_trace.c | 4 ++--
>  net/core/filter.c        | 4 ++--
>  5 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b5f0adae8293..356a58aeb79b 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -538,7 +538,7 @@ 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_LONG | MEM_UNINIT,
>  };
>
>  BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> @@ -566,7 +566,7 @@ 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_LONG | MEM_UNINIT,
>  };
>
>  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 bf6c5f685ea2..6d5942a6f41f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5952,7 +5952,7 @@ 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_LONG | MEM_UNINIT,
>  };
>
>  static const struct bpf_func_proto *
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d8520095ca03..70b0474e03a6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>         case ARG_PTR_TO_INT:
>         case ARG_PTR_TO_LONG:
>         {
> -               int size = int_ptr_type_to_size(arg_type);
> +               int size = int_ptr_type_to_size(base_type(arg_type));
>
> +               meta->raw_mode = arg_type & MEM_UNINIT;

given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
that memory, why not just set meta->raw_mode unconditionally and not
touch helper definitions?

also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
it? key should always be immutable, so can't be written into, no?

>                 err = check_helper_mem_access(env, regno, size, false, meta);
>                 if (err)
>                         return err;
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cd098846e251..95c3409ff374 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1226,7 +1226,7 @@ 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_LONG | MEM_UNINIT,
>  };
>
>  BPF_CALL_2(get_func_ret, void *, ctx, u64 *, value)
> @@ -1242,7 +1242,7 @@ 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_LONG | MEM_UNINIT,
>  };
>
>  BPF_CALL_1(get_func_arg_cnt, void *, ctx)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index f3c72cf86099..2ff210cb068c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6346,7 +6346,7 @@ 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_INT | MEM_UNINIT,
>         .arg4_type      = ARG_ANYTHING,
>         .arg5_type      = ARG_ANYTHING,
>  };
> @@ -6357,7 +6357,7 @@ 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_INT | MEM_UNINIT,
>         .arg4_type      = ARG_ANYTHING,
>         .arg5_type      = ARG_ANYTHING,
>  };
> --
> 2.43.0
>
>
Daniel Borkmann Sept. 4, 2024, 4:02 p.m. UTC | #3
On 8/28/24 12:37 AM, Andrii Nakryiko wrote:
> On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> 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.
>>
>> 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>
>> ---
>>   kernel/bpf/helpers.c     | 4 ++--
>>   kernel/bpf/syscall.c     | 2 +-
>>   kernel/bpf/verifier.c    | 3 ++-
>>   kernel/trace/bpf_trace.c | 4 ++--
>>   net/core/filter.c        | 4 ++--
>>   5 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index b5f0adae8293..356a58aeb79b 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -538,7 +538,7 @@ 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_LONG | MEM_UNINIT,
>>   };
>>
>>   BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
>> @@ -566,7 +566,7 @@ 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_LONG | MEM_UNINIT,
>>   };
>>
>>   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 bf6c5f685ea2..6d5942a6f41f 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -5952,7 +5952,7 @@ 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_LONG | MEM_UNINIT,
>>   };
>>
>>   static const struct bpf_func_proto *
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d8520095ca03..70b0474e03a6 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>>          case ARG_PTR_TO_INT:
>>          case ARG_PTR_TO_LONG:
>>          {
>> -               int size = int_ptr_type_to_size(arg_type);
>> +               int size = int_ptr_type_to_size(base_type(arg_type));
>>
>> +               meta->raw_mode = arg_type & MEM_UNINIT;
> 
> given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
> that memory, why not just set meta->raw_mode unconditionally and not
> touch helper definitions?
> 
> also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
> it? key should always be immutable, so can't be written into, no?

That does not look right agree.. presumably copied over from mem_types for reading not
writing memory (just that none of the helpers using the arg type to actually read mem).

Also, I'm currently looking into whether it's possible to just remove the ARG_PTR_TO_{INT,LONG}
and make that a case of ARG_PTR_TO_FIXED_SIZE_MEM | MEM_UNINIT where we just specify the
arg's size in the func proto. Two special arg cases less to look after in verifier then.

Thanks,
Daniel
Andrii Nakryiko Sept. 4, 2024, 5:53 p.m. UTC | #4
On Wed, Sep 4, 2024 at 9:02 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/28/24 12:37 AM, Andrii Nakryiko wrote:
> > On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> 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.
> >>
> >> 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>
> >> ---
> >>   kernel/bpf/helpers.c     | 4 ++--
> >>   kernel/bpf/syscall.c     | 2 +-
> >>   kernel/bpf/verifier.c    | 3 ++-
> >>   kernel/trace/bpf_trace.c | 4 ++--
> >>   net/core/filter.c        | 4 ++--
> >>   5 files changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> >> index b5f0adae8293..356a58aeb79b 100644
> >> --- a/kernel/bpf/helpers.c
> >> +++ b/kernel/bpf/helpers.c
> >> @@ -538,7 +538,7 @@ 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_LONG | MEM_UNINIT,
> >>   };
> >>
> >>   BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> >> @@ -566,7 +566,7 @@ 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_LONG | MEM_UNINIT,
> >>   };
> >>
> >>   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 bf6c5f685ea2..6d5942a6f41f 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -5952,7 +5952,7 @@ 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_LONG | MEM_UNINIT,
> >>   };
> >>
> >>   static const struct bpf_func_proto *
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index d8520095ca03..70b0474e03a6 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -8877,8 +8877,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >>          case ARG_PTR_TO_INT:
> >>          case ARG_PTR_TO_LONG:
> >>          {
> >> -               int size = int_ptr_type_to_size(arg_type);
> >> +               int size = int_ptr_type_to_size(base_type(arg_type));
> >>
> >> +               meta->raw_mode = arg_type & MEM_UNINIT;
> >
> > given all existing ARG_PTR_TO_{INT,LONG} use cases just write into
> > that memory, why not just set meta->raw_mode unconditionally and not
> > touch helper definitions?
> >
> > also, isn't it suspicious that int_ptr_types have PTR_TO_MAP_KEY in
> > it? key should always be immutable, so can't be written into, no?
>
> That does not look right agree.. presumably copied over from mem_types for reading not
> writing memory (just that none of the helpers using the arg type to actually read mem).
>
> Also, I'm currently looking into whether it's possible to just remove the ARG_PTR_TO_{INT,LONG}
> and make that a case of ARG_PTR_TO_FIXED_SIZE_MEM | MEM_UNINIT where we just specify the
> arg's size in the func proto. Two special arg cases less to look after in verifier then.
>

When I looked at this last time, my conclusion was that
PTR_TO_{INT,LONG} just have extra alignment checks, which might be
important on some architectures. Not sure how to go about that. We
could probably implement this as another MEM_ALIGNED modifier or
something, not sure.

> Thanks,
> Daniel
diff mbox series

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b5f0adae8293..356a58aeb79b 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -538,7 +538,7 @@  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_LONG | MEM_UNINIT,
 };
 
 BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
@@ -566,7 +566,7 @@  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_LONG | MEM_UNINIT,
 };
 
 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 bf6c5f685ea2..6d5942a6f41f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5952,7 +5952,7 @@  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_LONG | MEM_UNINIT,
 };
 
 static const struct bpf_func_proto *
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d8520095ca03..70b0474e03a6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8877,8 +8877,9 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	case ARG_PTR_TO_INT:
 	case ARG_PTR_TO_LONG:
 	{
-		int size = int_ptr_type_to_size(arg_type);
+		int size = int_ptr_type_to_size(base_type(arg_type));
 
+		meta->raw_mode = arg_type & MEM_UNINIT;
 		err = check_helper_mem_access(env, regno, size, false, meta);
 		if (err)
 			return err;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..95c3409ff374 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1226,7 +1226,7 @@  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_LONG | MEM_UNINIT,
 };
 
 BPF_CALL_2(get_func_ret, void *, ctx, u64 *, value)
@@ -1242,7 +1242,7 @@  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_LONG | MEM_UNINIT,
 };
 
 BPF_CALL_1(get_func_arg_cnt, void *, ctx)
diff --git a/net/core/filter.c b/net/core/filter.c
index f3c72cf86099..2ff210cb068c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6346,7 +6346,7 @@  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_INT | MEM_UNINIT,
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };
@@ -6357,7 +6357,7 @@  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_INT | MEM_UNINIT,
 	.arg4_type      = ARG_ANYTHING,
 	.arg5_type      = ARG_ANYTHING,
 };