| Message ID | 20230126233439.3739120-3-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add skb + xdp dynptrs | expand |
Hi Joanne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/bpf-Allow-sk_buff-and-xdp_buff-as-valid-kfunc-arg-types/20230128-170947
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230126233439.3739120-3-joannelkoong%40gmail.com
patch subject: [PATCH v8 bpf-next 2/5] bpf: Allow initializing dynptrs in kfuncs
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230128/202301281922.okIebogn-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7993ffba3295a3a3c01c4b62099117b5abd48242
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joanne-Koong/bpf-Allow-sk_buff-and-xdp_buff-as-valid-kfunc-arg-types/20230128-170947
git checkout 7993ffba3295a3a3c01c4b62099117b5abd48242
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash kernel/bpf/ net/core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> kernel/bpf/verifier.c:6150:5: warning: no previous prototype for 'process_dynptr_func' [-Wmissing-prototypes]
6150 | int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
| ^~~~~~~~~~~~~~~~~~~
vim +/process_dynptr_func +6150 kernel/bpf/verifier.c
6124
6125 /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK
6126 * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR.
6127 *
6128 * In both cases we deal with the first 8 bytes, but need to mark the next 8
6129 * bytes as STACK_DYNPTR in case of PTR_TO_STACK. In case of
6130 * CONST_PTR_TO_DYNPTR, we are guaranteed to get the beginning of the object.
6131 *
6132 * Mutability of bpf_dynptr is at two levels, one is at the level of struct
6133 * bpf_dynptr itself, i.e. whether the helper is receiving a pointer to struct
6134 * bpf_dynptr or pointer to const struct bpf_dynptr. In the former case, it can
6135 * mutate the view of the dynptr and also possibly destroy it. In the latter
6136 * case, it cannot mutate the bpf_dynptr itself but it can still mutate the
6137 * memory that dynptr points to.
6138 *
6139 * The verifier will keep track both levels of mutation (bpf_dynptr's in
6140 * reg->type and the memory's in reg->dynptr.type), but there is no support for
6141 * readonly dynptr view yet, hence only the first case is tracked and checked.
6142 *
6143 * This is consistent with how C applies the const modifier to a struct object,
6144 * where the pointer itself inside bpf_dynptr becomes const but not what it
6145 * points to.
6146 *
6147 * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument
6148 * type, and declare it as 'const struct bpf_dynptr *' in their prototype.
6149 */
> 6150 int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
6151 enum bpf_arg_type arg_type)
6152 {
6153 struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
6154 int err;
6155
6156 /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
6157 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
6158 */
6159 if ((arg_type & (MEM_UNINIT | MEM_RDONLY)) == (MEM_UNINIT | MEM_RDONLY)) {
6160 verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n");
6161 return -EFAULT;
6162 }
6163
6164 /* MEM_UNINIT - Points to memory that is an appropriate candidate for
6165 * constructing a mutable bpf_dynptr object.
6166 *
6167 * Currently, this is only possible with PTR_TO_STACK
6168 * pointing to a region of at least 16 bytes which doesn't
6169 * contain an existing bpf_dynptr.
6170 *
6171 * MEM_RDONLY - Points to a initialized bpf_dynptr that will not be
6172 * mutated or destroyed. However, the memory it points to
6173 * may be mutated.
6174 *
6175 * None - Points to a initialized dynptr that can be mutated and
6176 * destroyed, including mutation of the memory it points
6177 * to.
6178 */
6179 if (arg_type & MEM_UNINIT) {
6180 int i, spi;
6181
6182 if (base_type(reg->type) == CONST_PTR_TO_DYNPTR) {
6183 verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n");
6184 return -EFAULT;
6185 }
6186
6187 /* For -ERANGE (i.e. spi not falling into allocated stack slots),
6188 * check_mem_access will check and update stack bounds, so this
6189 * is okay.
6190 */
6191 spi = dynptr_get_spi(env, reg);
6192 if (spi < 0 && spi != -ERANGE)
6193 return spi;
6194
6195 /* we write BPF_DW bits (8 bytes) at a time */
6196 for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) {
6197 err = check_mem_access(env, insn_idx, regno,
6198 i, BPF_DW, BPF_WRITE, -1, false);
6199 if (err)
6200 return err;
6201 }
6202
6203 /* Please note that we allow overwriting existing unreferenced STACK_DYNPTR
6204 * slots (mark_stack_slots_dynptr calls destroy_if_dynptr_stack_slot
6205 * to ensure dynptr objects at the slots we are touching are completely
6206 * destructed before we reinitialize them for a new one). For referenced
6207 * ones, destroy_if_dynptr_stack_slot returns an error early instead of
6208 * delaying it until the end where the user will get "Unreleased
6209 * reference" error.
6210 */
6211 err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx);
6212 } else /* MEM_RDONLY and None case from above */ {
6213 /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */
6214 if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) {
6215 verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n");
6216 return -EINVAL;
6217 }
6218
6219 if (!is_dynptr_reg_valid_init(env, reg)) {
6220 verbose(env, "Expected an initialized dynptr as arg #%d\n",
6221 regno);
6222 return -EINVAL;
6223 }
6224
6225 /* Fold modifiers (in this case, MEM_RDONLY) when checking expected type */
6226 if (!is_dynptr_type_expected(env, reg, arg_type & ~MEM_RDONLY)) {
6227 const char *err_extra = "";
6228
6229 switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
6230 case DYNPTR_TYPE_LOCAL:
6231 err_extra = "local";
6232 break;
6233 case DYNPTR_TYPE_RINGBUF:
6234 err_extra = "ringbuf";
6235 break;
6236 default:
6237 err_extra = "<unknown>";
6238 break;
6239 }
6240 verbose(env,
6241 "Expected a dynptr of type %s as arg #%d\n",
6242 err_extra, regno);
6243 return -EINVAL;
6244 }
6245
6246 err = mark_dynptr_read(env, reg);
6247 }
6248 return err;
6249 }
6250
Hi Joanne,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Joanne-Koong/bpf-Allow-sk_buff-and-xdp_buff-as-valid-kfunc-arg-types/20230128-170947
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230126233439.3739120-3-joannelkoong%40gmail.com
patch subject: [PATCH v8 bpf-next 2/5] bpf: Allow initializing dynptrs in kfuncs
config: i386-randconfig-a012-20230123 (https://download.01.org/0day-ci/archive/20230128/202301282240.BuACl7kr-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7993ffba3295a3a3c01c4b62099117b5abd48242
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Joanne-Koong/bpf-Allow-sk_buff-and-xdp_buff-as-valid-kfunc-arg-types/20230128-170947
git checkout 7993ffba3295a3a3c01c4b62099117b5abd48242
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash kernel/bpf/ net/core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> kernel/bpf/verifier.c:6150:5: warning: no previous prototype for function 'process_dynptr_func' [-Wmissing-prototypes]
int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
^
kernel/bpf/verifier.c:6150:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
^
static
1 warning generated.
vim +/process_dynptr_func +6150 kernel/bpf/verifier.c
6124
6125 /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK
6126 * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR.
6127 *
6128 * In both cases we deal with the first 8 bytes, but need to mark the next 8
6129 * bytes as STACK_DYNPTR in case of PTR_TO_STACK. In case of
6130 * CONST_PTR_TO_DYNPTR, we are guaranteed to get the beginning of the object.
6131 *
6132 * Mutability of bpf_dynptr is at two levels, one is at the level of struct
6133 * bpf_dynptr itself, i.e. whether the helper is receiving a pointer to struct
6134 * bpf_dynptr or pointer to const struct bpf_dynptr. In the former case, it can
6135 * mutate the view of the dynptr and also possibly destroy it. In the latter
6136 * case, it cannot mutate the bpf_dynptr itself but it can still mutate the
6137 * memory that dynptr points to.
6138 *
6139 * The verifier will keep track both levels of mutation (bpf_dynptr's in
6140 * reg->type and the memory's in reg->dynptr.type), but there is no support for
6141 * readonly dynptr view yet, hence only the first case is tracked and checked.
6142 *
6143 * This is consistent with how C applies the const modifier to a struct object,
6144 * where the pointer itself inside bpf_dynptr becomes const but not what it
6145 * points to.
6146 *
6147 * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument
6148 * type, and declare it as 'const struct bpf_dynptr *' in their prototype.
6149 */
> 6150 int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
6151 enum bpf_arg_type arg_type)
6152 {
6153 struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
6154 int err;
6155
6156 /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
6157 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
6158 */
6159 if ((arg_type & (MEM_UNINIT | MEM_RDONLY)) == (MEM_UNINIT | MEM_RDONLY)) {
6160 verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n");
6161 return -EFAULT;
6162 }
6163
6164 /* MEM_UNINIT - Points to memory that is an appropriate candidate for
6165 * constructing a mutable bpf_dynptr object.
6166 *
6167 * Currently, this is only possible with PTR_TO_STACK
6168 * pointing to a region of at least 16 bytes which doesn't
6169 * contain an existing bpf_dynptr.
6170 *
6171 * MEM_RDONLY - Points to a initialized bpf_dynptr that will not be
6172 * mutated or destroyed. However, the memory it points to
6173 * may be mutated.
6174 *
6175 * None - Points to a initialized dynptr that can be mutated and
6176 * destroyed, including mutation of the memory it points
6177 * to.
6178 */
6179 if (arg_type & MEM_UNINIT) {
6180 int i, spi;
6181
6182 if (base_type(reg->type) == CONST_PTR_TO_DYNPTR) {
6183 verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n");
6184 return -EFAULT;
6185 }
6186
6187 /* For -ERANGE (i.e. spi not falling into allocated stack slots),
6188 * check_mem_access will check and update stack bounds, so this
6189 * is okay.
6190 */
6191 spi = dynptr_get_spi(env, reg);
6192 if (spi < 0 && spi != -ERANGE)
6193 return spi;
6194
6195 /* we write BPF_DW bits (8 bytes) at a time */
6196 for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) {
6197 err = check_mem_access(env, insn_idx, regno,
6198 i, BPF_DW, BPF_WRITE, -1, false);
6199 if (err)
6200 return err;
6201 }
6202
6203 /* Please note that we allow overwriting existing unreferenced STACK_DYNPTR
6204 * slots (mark_stack_slots_dynptr calls destroy_if_dynptr_stack_slot
6205 * to ensure dynptr objects at the slots we are touching are completely
6206 * destructed before we reinitialize them for a new one). For referenced
6207 * ones, destroy_if_dynptr_stack_slot returns an error early instead of
6208 * delaying it until the end where the user will get "Unreleased
6209 * reference" error.
6210 */
6211 err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx);
6212 } else /* MEM_RDONLY and None case from above */ {
6213 /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */
6214 if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) {
6215 verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n");
6216 return -EINVAL;
6217 }
6218
6219 if (!is_dynptr_reg_valid_init(env, reg)) {
6220 verbose(env, "Expected an initialized dynptr as arg #%d\n",
6221 regno);
6222 return -EINVAL;
6223 }
6224
6225 /* Fold modifiers (in this case, MEM_RDONLY) when checking expected type */
6226 if (!is_dynptr_type_expected(env, reg, arg_type & ~MEM_RDONLY)) {
6227 const char *err_extra = "";
6228
6229 switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
6230 case DYNPTR_TYPE_LOCAL:
6231 err_extra = "local";
6232 break;
6233 case DYNPTR_TYPE_RINGBUF:
6234 err_extra = "ringbuf";
6235 break;
6236 default:
6237 err_extra = "<unknown>";
6238 break;
6239 }
6240 verbose(env,
6241 "Expected a dynptr of type %s as arg #%d\n",
6242 err_extra, regno);
6243 return -EINVAL;
6244 }
6245
6246 err = mark_dynptr_read(env, reg);
6247 }
6248 return err;
6249 }
6250
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index aa83de1fe755..bee10101222d 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -618,9 +618,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, enum bpf_arg_type arg_type); int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, u32 regno, u32 mem_size); -struct bpf_call_arg_meta; -int process_dynptr_func(struct bpf_verifier_env *env, int regno, - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta); /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6bd097e0d45f..aedf28ef7f63 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -264,7 +264,6 @@ struct bpf_call_arg_meta { u32 ret_btf_id; u32 subprogno; struct btf_field *kptr_field; - u8 uninit_dynptr_regno; }; struct btf *btf_vmlinux; @@ -946,41 +945,24 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, return 0; } -static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg, - int spi) -{ - if (reg->type == CONST_PTR_TO_DYNPTR) - return false; - - /* For -ERANGE (i.e. spi not falling into allocated stack slots), we - * will do check_mem_access to check and update stack bounds later, so - * return true for that case. - */ - if (spi < 0) - return spi == -ERANGE; - /* We allow overwriting existing unreferenced STACK_DYNPTR slots, see - * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to - * ensure dynptr objects at the slots we are touching are completely - * destructed before we reinitialize them for a new one. For referenced - * ones, destroy_if_dynptr_stack_slot returns an error early instead of - * delaying it until the end where the user will get "Unreleased - * reference" error. - */ - return true; -} - -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg, - int spi) +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int i; + int i, spi; - /* This already represents first slot of initialized bpf_dynptr */ + /* This already represents first slot of initialized bpf_dynptr. + * + * Please also note that CONST_PTR_TO_DYNPTR already has fixed and + * var_off as 0 due to check_func_arg_reg_off's logic, so we don't + * need to check its offset and alignment. + */ if (reg->type == CONST_PTR_TO_DYNPTR) return true; + spi = dynptr_get_spi(env, reg); if (spi < 0) return false; + if (!state->stack[spi].spilled_ptr.dynptr.first_slot) return false; @@ -6165,11 +6147,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument * type, and declare it as 'const struct bpf_dynptr *' in their prototype. */ -int process_dynptr_func(struct bpf_verifier_env *env, int regno, - enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) +int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx, + enum bpf_arg_type arg_type) { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; - int spi = 0; + int err; /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): @@ -6178,15 +6160,6 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n"); return -EFAULT; } - /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to - * check_func_arg_reg_off's logic. We only need to check offset - * and its alignment for PTR_TO_STACK. - */ - if (reg->type == PTR_TO_STACK) { - spi = dynptr_get_spi(env, reg); - if (spi < 0 && spi != -ERANGE) - return spi; - } /* MEM_UNINIT - Points to memory that is an appropriate candidate for * constructing a mutable bpf_dynptr object. @@ -6204,32 +6177,47 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, * to. */ if (arg_type & MEM_UNINIT) { - if (!is_dynptr_reg_valid_uninit(env, reg, spi)) { - verbose(env, "Dynptr has to be an uninitialized dynptr\n"); - return -EINVAL; + int i, spi; + + if (base_type(reg->type) == CONST_PTR_TO_DYNPTR) { + verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n"); + return -EFAULT; } - /* We only support one dynptr being uninitialized at the moment, - * which is sufficient for the helper functions we have right now. + /* For -ERANGE (i.e. spi not falling into allocated stack slots), + * check_mem_access will check and update stack bounds, so this + * is okay. */ - if (meta->uninit_dynptr_regno) { - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); - return -EFAULT; + spi = dynptr_get_spi(env, reg); + if (spi < 0 && spi != -ERANGE) + return spi; + + /* we write BPF_DW bits (8 bytes) at a time */ + for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { + err = check_mem_access(env, insn_idx, regno, + i, BPF_DW, BPF_WRITE, -1, false); + if (err) + return err; } - meta->uninit_dynptr_regno = regno; + /* Please note that we allow overwriting existing unreferenced STACK_DYNPTR + * slots (mark_stack_slots_dynptr calls destroy_if_dynptr_stack_slot + * to ensure dynptr objects at the slots we are touching are completely + * destructed before we reinitialize them for a new one). For referenced + * ones, destroy_if_dynptr_stack_slot returns an error early instead of + * delaying it until the end where the user will get "Unreleased + * reference" error. + */ + err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx); } else /* MEM_RDONLY and None case from above */ { - int err; - /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */ if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) { verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n"); return -EINVAL; } - if (!is_dynptr_reg_valid_init(env, reg, spi)) { - verbose(env, - "Expected an initialized dynptr as arg #%d\n", + if (!is_dynptr_reg_valid_init(env, reg)) { + verbose(env, "Expected an initialized dynptr as arg #%d\n", regno); return -EINVAL; } @@ -6256,10 +6244,8 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, } err = mark_dynptr_read(env, reg); - if (err) - return err; } - return 0; + return err; } static bool arg_type_is_mem_size(enum bpf_arg_type type) @@ -6623,7 +6609,8 @@ static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_call_arg_meta *meta, - const struct bpf_func_proto *fn) + const struct bpf_func_proto *fn, + int insn_idx) { u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; @@ -6832,7 +6819,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, err = check_mem_size_reg(env, reg, regno, true, meta); break; case ARG_PTR_TO_DYNPTR: - err = process_dynptr_func(env, regno, arg_type, meta); + err = process_dynptr_func(env, regno, insn_idx, arg_type); if (err) return err; break; @@ -8044,7 +8031,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn meta.func_id = func_id; /* check args */ for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { - err = check_func_arg(env, i, &meta, fn); + err = check_func_arg(env, i, &meta, fn, insn_idx); if (err) return err; } @@ -8069,30 +8056,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs = cur_regs(env); - /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot - * be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr - * is safe to do directly. - */ - if (meta.uninit_dynptr_regno) { - if (regs[meta.uninit_dynptr_regno].type == CONST_PTR_TO_DYNPTR) { - verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n"); - return -EFAULT; - } - /* we write BPF_DW bits (8 bytes) at a time */ - for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { - err = check_mem_access(env, insn_idx, meta.uninit_dynptr_regno, - i, BPF_DW, BPF_WRITE, -1, false); - if (err) - return err; - } - - err = mark_stack_slots_dynptr(env, ®s[meta.uninit_dynptr_regno], - fn->arg_type[meta.uninit_dynptr_regno - BPF_REG_1], - insn_idx); - if (err) - return err; - } - if (meta.release_regno) { err = -EINVAL; /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot @@ -9111,7 +9074,8 @@ static int process_kf_arg_ptr_to_list_node(struct bpf_verifier_env *env, return ref_set_release_on_unlock(env, reg->ref_obj_id); } -static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta) +static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta, + int insn_idx) { const char *func_name = meta->func_name, *ref_tname; const struct btf *btf = meta->btf; @@ -9305,7 +9269,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return -EINVAL; } - ret = process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR | MEM_RDONLY, NULL); + ret = process_dynptr_func(env, regno, insn_idx, + ARG_PTR_TO_DYNPTR | MEM_RDONLY); if (ret < 0) return ret; break; @@ -9471,7 +9436,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, } /* Check the arguments */ - err = check_kfunc_args(env, &meta); + err = check_kfunc_args(env, &meta, insn_idx); if (err < 0) return err; /* In case of release function, we get register number of refcounted
This change allows kfuncs to take in an uninitialized dynptr as a parameter. Before this change, only helper functions could successfully use uninitialized dynptrs. This change moves the memory access check (and stack state growing) into process_dynptr_func(), which both helpers and kfuncs call. This change also includes some light tidying up of existing code (eg remove unused parameter, move spi checking logic, remove unneeded checks) Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- include/linux/bpf_verifier.h | 3 - kernel/bpf/verifier.c | 139 +++++++++++++---------------------- 2 files changed, 52 insertions(+), 90 deletions(-)