On Wed, Sep 4, 2024 at 9:02 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> 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.
>
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
