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;
> }
> break;
> case ARG_CONST_SIZE:
> @@ -9044,17 +9027,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> if (err)
> return err;
> break;
[...]
