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

Daniel Borkmann <daniel@xxxxxxxxxxxxx> · Wed, 4 Sep 2024 18:02:57 +0200

On 8/28/24 12:37 AM, Andrii Nakryiko wrote:
On Fri, Aug 23, 2024 at 3:21 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxx>
Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
---
  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







