On Mon, May 09, 2022 at 03:42:53PM -0700, Joanne Koong wrote:
> This patch adds the bulk of the verifier work for supporting dynamic
> pointers (dynptrs) in bpf. This patch implements malloc-type dynptrs
> through 2 new APIs (bpf_dynptr_alloc and bpf_dynptr_put) that can be
> called by a bpf program. Malloc-type dynptrs are dynptrs that dynamically
> allocate memory on behalf of the program.
>
> A bpf_dynptr is opaque to the bpf program. It is a 16-byte structure
> defined internally as:
>
> struct bpf_dynptr_kern {
> void *data;
> u32 size;
> u32 offset;
> } __aligned(8);
>
> The upper 8 bits of *size* is reserved (it contains extra metadata about
> read-only status and dynptr type); consequently, a dynptr only supports
> memory less than 16 MB.
>
Small nit: s/less than/up to?
[...]
> +/* the implementation of the opaque uapi struct bpf_dynptr */
> +struct bpf_dynptr_kern {
> + void *data;
> + /* Size represents the number of usable bytes in the dynptr.
> + * If for example the offset is at 200 for a malloc dynptr with
> + * allocation size 256, the number of usable bytes is 56.
> + *
> + * The upper 8 bits are reserved.
> + * Bit 31 denotes whether the dynptr is read-only.
> + * Bits 28-30 denote the dynptr type.
It's pretty clear from context, but just for completeness, could you also
explicitly specify what bits 0 - 27 denote (24 - 27 reserved, 0 - 23 size)?
> + */
> + u32 size;
> + u32 offset;
> +} __aligned(8);
> +
> +enum bpf_dynptr_type {
> + BPF_DYNPTR_TYPE_INVALID,
> + /* Memory allocated dynamically by the kernel for the dynptr */
> + BPF_DYNPTR_TYPE_MALLOC,
> +};
> +
> +/* Since the upper 8 bits of dynptr->size is reserved, the
> + * maximum supported size is 2^24 - 1.
> + */
> +#define DYNPTR_MAX_SIZE ((1UL << 24) - 1)
> +#define DYNPTR_SIZE_MASK 0xFFFFFF
> +#define DYNPTR_TYPE_SHIFT 28
> +#define DYNPTR_TYPE_MASK 0x7
Should we add a static_assert(DYNPTR_SIZE_MASK >= DYNPTR_MAX_SIZE);
Potentially overkill, but if we're going to have separate macros for them
it might be prudent to add it?
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0fe1dea520ae..8cdedc776987 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -187,6 +187,10 @@ struct bpf_verifier_stack_elem {
> POISON_POINTER_DELTA))
> #define BPF_MAP_PTR(X) ((struct bpf_map *)((X) & ~BPF_MAP_PTR_UNPRIV))
>
> +static bool arg_type_is_mem_size(enum bpf_arg_type type);
> +static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx);
> +static int release_reference(struct bpf_verifier_env *env, int ref_obj_id);
> +
> static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux)
> {
> return BPF_MAP_PTR(aux->map_ptr_state) == BPF_MAP_PTR_POISON;
> @@ -259,6 +263,7 @@ struct bpf_call_arg_meta {
> u32 ret_btf_id;
> u32 subprogno;
> struct bpf_map_value_off_desc *kptr_off_desc;
> + u8 uninit_dynptr_regno;
> };
>
> struct btf *btf_vmlinux;
> @@ -580,6 +585,7 @@ static char slot_type_char[] = {
> [STACK_SPILL] = 'r',
> [STACK_MISC] = 'm',
> [STACK_ZERO] = '0',
> + [STACK_DYNPTR] = 'd',
> };
>
> static void print_liveness(struct bpf_verifier_env *env,
> @@ -595,6 +601,25 @@ static void print_liveness(struct bpf_verifier_env *env,
> verbose(env, "D");
> }
>
> +static inline int get_spi(s32 off)
> +{
> + return (-off - 1) / BPF_REG_SIZE;
> +}
Small / optional nit: It's probably harmless to leave this as inline as the
compiler will almost certainly inline it for you, but to that point, it's
probably not necessary to mark this as inline. It looks like most other
static functions in verifier.c are non-inline, so IMO it's probably best to
follow that lead.
[...]
> static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> @@ -5725,7 +5885,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>
> skip_type_check:
> if (arg_type_is_release(arg_type)) {
> - if (!reg->ref_obj_id && !register_is_null(reg)) {
> + if (arg_type_is_dynptr(arg_type)) {
> + struct bpf_func_state *state = func(env, reg);
> + int spi = get_spi(reg->off);
> +
> + if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> + !state->stack[spi].spilled_ptr.id) {
> + verbose(env, "arg %d is an unacquired reference\n", regno);
> + return -EINVAL;
> + }
> + } else if (!reg->ref_obj_id && !register_is_null(reg)) {
> verbose(env, "R%d must be referenced when passed to release function\n",
> regno);
> return -EINVAL;
> @@ -5837,6 +6006,43 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
>
> err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
> + } else if (arg_type_is_dynptr(arg_type)) {
> + /* Can't pass in a dynptr at a weird offset */
> + if (reg->off % BPF_REG_SIZE) {
> + verbose(env, "cannot pass in non-zero dynptr offset\n");
> + return -EINVAL;
> + }
Should this check be moved to check_func_arg_reg_off()?
> +
> + if (arg_type & MEM_UNINIT) {
> + if (!is_dynptr_reg_valid_uninit(env, reg)) {
> + verbose(env, "Arg #%d dynptr has to be an uninitialized dynptr\n",
> + arg + BPF_REG_1);
> + return -EINVAL;
> + }
> +
> + /* We only support one dynptr being uninitialized at the moment,
> + * which is sufficient for the helper functions we have right now.
> + */
> + if (meta->uninit_dynptr_regno) {
> + verbose(env, "verifier internal error: more than one uninitialized dynptr arg\n");
> + return -EFAULT;
> + }
> +
> + meta->uninit_dynptr_regno = arg + BPF_REG_1;
Can this be simplified to:
meta->uninit_dynptr_regno = regno;
[...]
Looks good otherwise, thanks!
Acked-by: David Vernet <void@xxxxxxxxxxxxx>
