On Fri, Sep 6, 2024 at 2:03 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Sep 6, 2024 at 6:56 AM 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.
> >
> > 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>).
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > ---
> > v1 -> v2:
> > - switch to MEM_FIXED_SIZE
> > v3 -> v4:
> > - fixed 64bit in strto{u,}l (Alexei)
> >
> > 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(-)
> >
>
> Very neat. I only have stylistic nits, but LGTM anyways
>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> [...]
>
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 5404bb964d83..0587d0c2375a 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -537,7 +537,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,
>
> nit: I wouldn't wrap the line here and everywhere else
>
> > + .arg4_size = sizeof(s64),
> > };
> >
> > BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
> > @@ -563,7 +565,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(u64),
> > };
> >
>
> [...]
>
> > static const struct bpf_reg_types spin_lock_types = {
> > .types = {
> > PTR_TO_MAP_VALUE,
> > @@ -8453,8 +8433,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,
> > @@ -9020,6 +8998,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);
>
> nit: we should take advantage of 100 character lines and make this streamlined:
>
> @@ -9016,11 +9016,10 @@ static int check_func_arg(struct
> bpf_verifier_env *env, u32 arg,
> * next is_mem_size argument below.
> */
> meta->raw_mode = arg_type & MEM_UNINIT;
> - if (arg_type & MEM_FIXED_SIZE) {
> - err = check_helper_mem_access(env, regno,
> - fn->arg_size[arg], false,
> - meta);
> - }
> + if (arg_type & MEM_FIXED_SIZE)
> + err = check_helper_mem_access(env, regno,
> fn->arg_size[arg], false, meta);
> + if (arg_type & MEM_ALIGNED)
> + err = err ?: check_ptr_alignment(env, reg, 0,
> fn->arg_size[arg], true);
> break;
Agree that it's a bit cleaner this way.
