diff mbox series

[bpf-next,1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

Message ID 20210310220211.1454516-2-revest@chromium.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Add a snprintf eBPF helper | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 4 maintainers not CCed: netdev@vger.kernel.org john.fastabend@gmail.com kafai@fb.com songliubraving@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 12198 this patch: 12200
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 12704 this patch: 12704
netdev/header_inline success Link

Commit Message

Florent Revest March 10, 2021, 10:02 p.m. UTC
This type provides the guarantee that an argument is going to be a const
pointer to somewhere in a read-only map value. It also checks that this
pointer is followed by a NULL character before the end of the map value.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 include/linux/bpf.h   |  1 +
 kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

kernel test robot March 11, 2021, 12:04 a.m. UTC | #1
Hi Florent,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: openrisc-randconfig-r023-20210308 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/cbb95ec99fafe0955aeada270c9be3d1477c3866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/bpf/verifier.c: In function 'check_func_arg':
>> kernel/bpf/verifier.c:4918:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    4918 |   map_ptr = (char *)(map_addr);
         |             ^
   In file included from include/linux/bpf_verifier.h:9,
                    from kernel/bpf/verifier.c:12:
   kernel/bpf/verifier.c: In function 'jit_subprogs':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:11728:16: note: in expansion of macro 'BPF_CAST_CALL'
   11728 |    insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) -
         |                ^~~~~~~~~~~~~
   kernel/bpf/verifier.c: In function 'do_misc_fixups':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12136:17: note: in expansion of macro 'BPF_CAST_CALL'
   12136 |     insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12140:17: note: in expansion of macro 'BPF_CAST_CALL'
   12140 |     insn->imm = BPF_CAST_CALL(ops->map_update_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12144:17: note: in expansion of macro 'BPF_CAST_CALL'
   12144 |     insn->imm = BPF_CAST_CALL(ops->map_delete_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12148:17: note: in expansion of macro 'BPF_CAST_CALL'
   12148 |     insn->imm = BPF_CAST_CALL(ops->map_push_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12152:17: note: in expansion of macro 'BPF_CAST_CALL'
   12152 |     insn->imm = BPF_CAST_CALL(ops->map_pop_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12156:17: note: in expansion of macro 'BPF_CAST_CALL'
   12156 |     insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32,  u64)' {aka 'int (* const)(struct bpf_map *, unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12160:17: note: in expansion of macro 'BPF_CAST_CALL'
   12160 |     insn->imm = BPF_CAST_CALL(ops->map_redirect) -
         |                 ^~~~~~~~~~~~~


vim +4918 kernel/bpf/verifier.c

  4695	
  4696	static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
  4697				  struct bpf_call_arg_meta *meta,
  4698				  const struct bpf_func_proto *fn)
  4699	{
  4700		u32 regno = BPF_REG_1 + arg;
  4701		struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
  4702		enum bpf_arg_type arg_type = fn->arg_type[arg];
  4703		enum bpf_reg_type type = reg->type;
  4704		int err = 0;
  4705	
  4706		if (arg_type == ARG_DONTCARE)
  4707			return 0;
  4708	
  4709		err = check_reg_arg(env, regno, SRC_OP);
  4710		if (err)
  4711			return err;
  4712	
  4713		if (arg_type == ARG_ANYTHING) {
  4714			if (is_pointer_value(env, regno)) {
  4715				verbose(env, "R%d leaks addr into helper function\n",
  4716					regno);
  4717				return -EACCES;
  4718			}
  4719			return 0;
  4720		}
  4721	
  4722		if (type_is_pkt_pointer(type) &&
  4723		    !may_access_direct_pkt_data(env, meta, BPF_READ)) {
  4724			verbose(env, "helper access to the packet is not allowed\n");
  4725			return -EACCES;
  4726		}
  4727	
  4728		if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4729		    arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
  4730		    arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
  4731			err = resolve_map_arg_type(env, meta, &arg_type);
  4732			if (err)
  4733				return err;
  4734		}
  4735	
  4736		if (register_is_null(reg) && arg_type_may_be_null(arg_type))
  4737			/* A NULL register has a SCALAR_VALUE type, so skip
  4738			 * type checking.
  4739			 */
  4740			goto skip_type_check;
  4741	
  4742		err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]);
  4743		if (err)
  4744			return err;
  4745	
  4746		if (type == PTR_TO_CTX) {
  4747			err = check_ctx_reg(env, reg, regno);
  4748			if (err < 0)
  4749				return err;
  4750		}
  4751	
  4752	skip_type_check:
  4753		if (reg->ref_obj_id) {
  4754			if (meta->ref_obj_id) {
  4755				verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
  4756					regno, reg->ref_obj_id,
  4757					meta->ref_obj_id);
  4758				return -EFAULT;
  4759			}
  4760			meta->ref_obj_id = reg->ref_obj_id;
  4761		}
  4762	
  4763		if (arg_type == ARG_CONST_MAP_PTR) {
  4764			/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
  4765			meta->map_ptr = reg->map_ptr;
  4766		} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
  4767			/* bpf_map_xxx(..., map_ptr, ..., key) call:
  4768			 * check that [key, key + map->key_size) are within
  4769			 * stack limits and initialized
  4770			 */
  4771			if (!meta->map_ptr) {
  4772				/* in function declaration map_ptr must come before
  4773				 * map_key, so that it's verified and known before
  4774				 * we have to check map_key here. Otherwise it means
  4775				 * that kernel subsystem misconfigured verifier
  4776				 */
  4777				verbose(env, "invalid map_ptr to access map->key\n");
  4778				return -EACCES;
  4779			}
  4780			err = check_helper_mem_access(env, regno,
  4781						      meta->map_ptr->key_size, false,
  4782						      NULL);
  4783		} else if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4784			   (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
  4785			    !register_is_null(reg)) ||
  4786			   arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
  4787			/* bpf_map_xxx(..., map_ptr, ..., value) call:
  4788			 * check [value, value + map->value_size) validity
  4789			 */
  4790			if (!meta->map_ptr) {
  4791				/* kernel subsystem misconfigured verifier */
  4792				verbose(env, "invalid map_ptr to access map->value\n");
  4793				return -EACCES;
  4794			}
  4795			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
  4796			err = check_helper_mem_access(env, regno,
  4797						      meta->map_ptr->value_size, false,
  4798						      meta);
  4799		} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
  4800			if (!reg->btf_id) {
  4801				verbose(env, "Helper has invalid btf_id in R%d\n", regno);
  4802				return -EACCES;
  4803			}
  4804			meta->ret_btf = reg->btf;
  4805			meta->ret_btf_id = reg->btf_id;
  4806		} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
  4807			if (meta->func_id == BPF_FUNC_spin_lock) {
  4808				if (process_spin_lock(env, regno, true))
  4809					return -EACCES;
  4810			} else if (meta->func_id == BPF_FUNC_spin_unlock) {
  4811				if (process_spin_lock(env, regno, false))
  4812					return -EACCES;
  4813			} else {
  4814				verbose(env, "verifier internal error\n");
  4815				return -EFAULT;
  4816			}
  4817		} else if (arg_type == ARG_PTR_TO_FUNC) {
  4818			meta->subprogno = reg->subprogno;
  4819		} else if (arg_type_is_mem_ptr(arg_type)) {
  4820			/* The access to this pointer is only checked when we hit the
  4821			 * next is_mem_size argument below.
  4822			 */
  4823			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
  4824		} else if (arg_type_is_mem_size(arg_type)) {
  4825			bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
  4826	
  4827			/* This is used to refine r0 return value bounds for helpers
  4828			 * that enforce this value as an upper bound on return values.
  4829			 * See do_refine_retval_range() for helpers that can refine
  4830			 * the return value. C type of helper is u32 so we pull register
  4831			 * bound from umax_value however, if negative verifier errors
  4832			 * out. Only upper bounds can be learned because retval is an
  4833			 * int type and negative retvals are allowed.
  4834			 */
  4835			meta->msize_max_value = reg->umax_value;
  4836	
  4837			/* The register is SCALAR_VALUE; the access check
  4838			 * happens using its boundaries.
  4839			 */
  4840			if (!tnum_is_const(reg->var_off))
  4841				/* For unprivileged variable accesses, disable raw
  4842				 * mode so that the program is required to
  4843				 * initialize all the memory that the helper could
  4844				 * just partially fill up.
  4845				 */
  4846				meta = NULL;
  4847	
  4848			if (reg->smin_value < 0) {
  4849				verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
  4850					regno);
  4851				return -EACCES;
  4852			}
  4853	
  4854			if (reg->umin_value == 0) {
  4855				err = check_helper_mem_access(env, regno - 1, 0,
  4856							      zero_size_allowed,
  4857							      meta);
  4858				if (err)
  4859					return err;
  4860			}
  4861	
  4862			if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
  4863				verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
  4864					regno);
  4865				return -EACCES;
  4866			}
  4867			err = check_helper_mem_access(env, regno - 1,
  4868						      reg->umax_value,
  4869						      zero_size_allowed, meta);
  4870			if (!err)
  4871				err = mark_chain_precision(env, regno);
  4872		} else if (arg_type_is_alloc_size(arg_type)) {
  4873			if (!tnum_is_const(reg->var_off)) {
  4874				verbose(env, "R%d is not a known constant'\n",
  4875					regno);
  4876				return -EACCES;
  4877			}
  4878			meta->mem_size = reg->var_off.value;
  4879		} else if (arg_type_is_int_ptr(arg_type)) {
  4880			int size = int_ptr_type_to_size(arg_type);
  4881	
  4882			err = check_helper_mem_access(env, regno, size, false, meta);
  4883			if (err)
  4884				return err;
  4885			err = check_ptr_alignment(env, reg, 0, size, true);
  4886		} else if (arg_type == ARG_PTR_TO_CONST_STR) {
  4887			struct bpf_map *map = reg->map_ptr;
  4888			int map_off, i;
  4889			u64 map_addr;
  4890			char *map_ptr;
  4891	
  4892			if (!map || !bpf_map_is_rdonly(map)) {
  4893				verbose(env, "R%d does not point to a readonly map'\n", regno);
  4894				return -EACCES;
  4895			}
  4896	
  4897			if (!tnum_is_const(reg->var_off)) {
  4898				verbose(env, "R%d is not a constant address'\n", regno);
  4899				return -EACCES;
  4900			}
  4901	
  4902			if (!map->ops->map_direct_value_addr) {
  4903				verbose(env, "no direct value access support for this map type\n");
  4904				return -EACCES;
  4905			}
  4906	
  4907			err = check_helper_mem_access(env, regno,
  4908						      map->value_size - reg->off,
  4909						      false, meta);
  4910			if (err)
  4911				return err;
  4912	
  4913			map_off = reg->off + reg->var_off.value;
  4914			err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
  4915			if (err)
  4916				return err;
  4917	
> 4918			map_ptr = (char *)(map_addr);
  4919			for (i = map_off; map_ptr[i] != '\0'; i++) {
  4920				if (i == map->value_size - 1) {
  4921					verbose(env, "map does not contain a NULL-terminated string\n");
  4922					return -EACCES;
  4923				}
  4924			}
  4925		}
  4926	
  4927		return err;
  4928	}
  4929	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot March 11, 2021, 1 a.m. UTC | #2
Hi Florent,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s001-20210308 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-262-g5e674421-dirty
        # https://github.com/0day-ci/linux/commit/cbb95ec99fafe0955aeada270c9be3d1477c3866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   kernel/bpf/verifier.c:11728:76: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12136:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12140:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12144:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12148:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12152:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12156:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12160:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12204:38: sparse: sparse: subtraction of functions? Share your drugs
>> kernel/bpf/verifier.c:4918:36: sparse: sparse: non size-preserving integer to pointer cast

vim +4918 kernel/bpf/verifier.c

  4695	
  4696	static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
  4697				  struct bpf_call_arg_meta *meta,
  4698				  const struct bpf_func_proto *fn)
  4699	{
  4700		u32 regno = BPF_REG_1 + arg;
  4701		struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
  4702		enum bpf_arg_type arg_type = fn->arg_type[arg];
  4703		enum bpf_reg_type type = reg->type;
  4704		int err = 0;
  4705	
  4706		if (arg_type == ARG_DONTCARE)
  4707			return 0;
  4708	
  4709		err = check_reg_arg(env, regno, SRC_OP);
  4710		if (err)
  4711			return err;
  4712	
  4713		if (arg_type == ARG_ANYTHING) {
  4714			if (is_pointer_value(env, regno)) {
  4715				verbose(env, "R%d leaks addr into helper function\n",
  4716					regno);
  4717				return -EACCES;
  4718			}
  4719			return 0;
  4720		}
  4721	
  4722		if (type_is_pkt_pointer(type) &&
  4723		    !may_access_direct_pkt_data(env, meta, BPF_READ)) {
  4724			verbose(env, "helper access to the packet is not allowed\n");
  4725			return -EACCES;
  4726		}
  4727	
  4728		if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4729		    arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
  4730		    arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
  4731			err = resolve_map_arg_type(env, meta, &arg_type);
  4732			if (err)
  4733				return err;
  4734		}
  4735	
  4736		if (register_is_null(reg) && arg_type_may_be_null(arg_type))
  4737			/* A NULL register has a SCALAR_VALUE type, so skip
  4738			 * type checking.
  4739			 */
  4740			goto skip_type_check;
  4741	
  4742		err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]);
  4743		if (err)
  4744			return err;
  4745	
  4746		if (type == PTR_TO_CTX) {
  4747			err = check_ctx_reg(env, reg, regno);
  4748			if (err < 0)
  4749				return err;
  4750		}
  4751	
  4752	skip_type_check:
  4753		if (reg->ref_obj_id) {
  4754			if (meta->ref_obj_id) {
  4755				verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
  4756					regno, reg->ref_obj_id,
  4757					meta->ref_obj_id);
  4758				return -EFAULT;
  4759			}
  4760			meta->ref_obj_id = reg->ref_obj_id;
  4761		}
  4762	
  4763		if (arg_type == ARG_CONST_MAP_PTR) {
  4764			/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
  4765			meta->map_ptr = reg->map_ptr;
  4766		} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
  4767			/* bpf_map_xxx(..., map_ptr, ..., key) call:
  4768			 * check that [key, key + map->key_size) are within
  4769			 * stack limits and initialized
  4770			 */
  4771			if (!meta->map_ptr) {
  4772				/* in function declaration map_ptr must come before
  4773				 * map_key, so that it's verified and known before
  4774				 * we have to check map_key here. Otherwise it means
  4775				 * that kernel subsystem misconfigured verifier
  4776				 */
  4777				verbose(env, "invalid map_ptr to access map->key\n");
  4778				return -EACCES;
  4779			}
  4780			err = check_helper_mem_access(env, regno,
  4781						      meta->map_ptr->key_size, false,
  4782						      NULL);
  4783		} else if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4784			   (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
  4785			    !register_is_null(reg)) ||
  4786			   arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
  4787			/* bpf_map_xxx(..., map_ptr, ..., value) call:
  4788			 * check [value, value + map->value_size) validity
  4789			 */
  4790			if (!meta->map_ptr) {
  4791				/* kernel subsystem misconfigured verifier */
  4792				verbose(env, "invalid map_ptr to access map->value\n");
  4793				return -EACCES;
  4794			}
  4795			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
  4796			err = check_helper_mem_access(env, regno,
  4797						      meta->map_ptr->value_size, false,
  4798						      meta);
  4799		} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
  4800			if (!reg->btf_id) {
  4801				verbose(env, "Helper has invalid btf_id in R%d\n", regno);
  4802				return -EACCES;
  4803			}
  4804			meta->ret_btf = reg->btf;
  4805			meta->ret_btf_id = reg->btf_id;
  4806		} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
  4807			if (meta->func_id == BPF_FUNC_spin_lock) {
  4808				if (process_spin_lock(env, regno, true))
  4809					return -EACCES;
  4810			} else if (meta->func_id == BPF_FUNC_spin_unlock) {
  4811				if (process_spin_lock(env, regno, false))
  4812					return -EACCES;
  4813			} else {
  4814				verbose(env, "verifier internal error\n");
  4815				return -EFAULT;
  4816			}
  4817		} else if (arg_type == ARG_PTR_TO_FUNC) {
  4818			meta->subprogno = reg->subprogno;
  4819		} else if (arg_type_is_mem_ptr(arg_type)) {
  4820			/* The access to this pointer is only checked when we hit the
  4821			 * next is_mem_size argument below.
  4822			 */
  4823			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
  4824		} else if (arg_type_is_mem_size(arg_type)) {
  4825			bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
  4826	
  4827			/* This is used to refine r0 return value bounds for helpers
  4828			 * that enforce this value as an upper bound on return values.
  4829			 * See do_refine_retval_range() for helpers that can refine
  4830			 * the return value. C type of helper is u32 so we pull register
  4831			 * bound from umax_value however, if negative verifier errors
  4832			 * out. Only upper bounds can be learned because retval is an
  4833			 * int type and negative retvals are allowed.
  4834			 */
  4835			meta->msize_max_value = reg->umax_value;
  4836	
  4837			/* The register is SCALAR_VALUE; the access check
  4838			 * happens using its boundaries.
  4839			 */
  4840			if (!tnum_is_const(reg->var_off))
  4841				/* For unprivileged variable accesses, disable raw
  4842				 * mode so that the program is required to
  4843				 * initialize all the memory that the helper could
  4844				 * just partially fill up.
  4845				 */
  4846				meta = NULL;
  4847	
  4848			if (reg->smin_value < 0) {
  4849				verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
  4850					regno);
  4851				return -EACCES;
  4852			}
  4853	
  4854			if (reg->umin_value == 0) {
  4855				err = check_helper_mem_access(env, regno - 1, 0,
  4856							      zero_size_allowed,
  4857							      meta);
  4858				if (err)
  4859					return err;
  4860			}
  4861	
  4862			if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
  4863				verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
  4864					regno);
  4865				return -EACCES;
  4866			}
  4867			err = check_helper_mem_access(env, regno - 1,
  4868						      reg->umax_value,
  4869						      zero_size_allowed, meta);
  4870			if (!err)
  4871				err = mark_chain_precision(env, regno);
  4872		} else if (arg_type_is_alloc_size(arg_type)) {
  4873			if (!tnum_is_const(reg->var_off)) {
  4874				verbose(env, "R%d is not a known constant'\n",
  4875					regno);
  4876				return -EACCES;
  4877			}
  4878			meta->mem_size = reg->var_off.value;
  4879		} else if (arg_type_is_int_ptr(arg_type)) {
  4880			int size = int_ptr_type_to_size(arg_type);
  4881	
  4882			err = check_helper_mem_access(env, regno, size, false, meta);
  4883			if (err)
  4884				return err;
  4885			err = check_ptr_alignment(env, reg, 0, size, true);
  4886		} else if (arg_type == ARG_PTR_TO_CONST_STR) {
  4887			struct bpf_map *map = reg->map_ptr;
  4888			int map_off, i;
  4889			u64 map_addr;
  4890			char *map_ptr;
  4891	
  4892			if (!map || !bpf_map_is_rdonly(map)) {
  4893				verbose(env, "R%d does not point to a readonly map'\n", regno);
  4894				return -EACCES;
  4895			}
  4896	
  4897			if (!tnum_is_const(reg->var_off)) {
  4898				verbose(env, "R%d is not a constant address'\n", regno);
  4899				return -EACCES;
  4900			}
  4901	
  4902			if (!map->ops->map_direct_value_addr) {
  4903				verbose(env, "no direct value access support for this map type\n");
  4904				return -EACCES;
  4905			}
  4906	
  4907			err = check_helper_mem_access(env, regno,
  4908						      map->value_size - reg->off,
  4909						      false, meta);
  4910			if (err)
  4911				return err;
  4912	
  4913			map_off = reg->off + reg->var_off.value;
  4914			err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
  4915			if (err)
  4916				return err;
  4917	
> 4918			map_ptr = (char *)(map_addr);
  4919			for (i = map_off; map_ptr[i] != '\0'; i++) {
  4920				if (i == map->value_size - 1) {
  4921					verbose(env, "map does not contain a NULL-terminated string\n");
  4922					return -EACCES;
  4923				}
  4924			}
  4925		}
  4926	
  4927		return err;
  4928	}
  4929	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Andrii Nakryiko March 16, 2021, 1:03 a.m. UTC | #3
On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
>
> This type provides the guarantee that an argument is going to be a const
> pointer to somewhere in a read-only map value. It also checks that this
> pointer is followed by a NULL character before the end of the map value.
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  include/linux/bpf.h   |  1 +
>  kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a25730eaa148..7b5319d75b3e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -308,6 +308,7 @@ enum bpf_arg_type {
>         ARG_PTR_TO_PERCPU_BTF_ID,       /* pointer to in-kernel percpu type */
>         ARG_PTR_TO_FUNC,        /* pointer to a bpf program function */
>         ARG_PTR_TO_STACK_OR_NULL,       /* pointer to stack or NULL */
> +       ARG_PTR_TO_CONST_STR,   /* pointer to a null terminated read-only string */
>         __BPF_ARG_TYPE_MAX,
>  };
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f9096b049cd6..c99b2b67dc8d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU
>  static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
>  static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
>  static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
> +static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
>
>  static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>         [ARG_PTR_TO_MAP_KEY]            = &map_key_value_types,
> @@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>         [ARG_PTR_TO_PERCPU_BTF_ID]      = &percpu_btf_ptr_types,
>         [ARG_PTR_TO_FUNC]               = &func_ptr_types,
>         [ARG_PTR_TO_STACK_OR_NULL]      = &stack_ptr_types,
> +       [ARG_PTR_TO_CONST_STR]          = &const_str_ptr_types,
>  };
>
>  static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> @@ -4881,6 +4883,45 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                 if (err)
>                         return err;
>                 err = check_ptr_alignment(env, reg, 0, size, true);
> +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> +               struct bpf_map *map = reg->map_ptr;
> +               int map_off, i;
> +               u64 map_addr;
> +               char *map_ptr;
> +
> +               if (!map || !bpf_map_is_rdonly(map)) {
> +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> +                       return -EACCES;
> +               }
> +
> +               if (!tnum_is_const(reg->var_off)) {
> +                       verbose(env, "R%d is not a constant address'\n", regno);
> +                       return -EACCES;
> +               }
> +
> +               if (!map->ops->map_direct_value_addr) {
> +                       verbose(env, "no direct value access support for this map type\n");
> +                       return -EACCES;
> +               }
> +
> +               err = check_helper_mem_access(env, regno,
> +                                             map->value_size - reg->off,
> +                                             false, meta);

you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
use check_map_access(). And double-check that register is of expected
type. just the presence of ref->map_ptr might not be sufficient?

> +               if (err)
> +                       return err;
> +
> +               map_off = reg->off + reg->var_off.value;
> +               err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> +               if (err)
> +                       return err;
> +
> +               map_ptr = (char *)(map_addr);

map_ptr is a very confusing name. str_ptr or value ptr?

> +               for (i = map_off; map_ptr[i] != '\0'; i++) {
> +                       if (i == map->value_size - 1) {

use strnchr()?

> +                               verbose(env, "map does not contain a NULL-terminated string\n");

map in the user-visible message is quite confusing, given that users
will probably use this through static variables, so maybe just "string
is not zero-terminated?" And it's not really a NULL, it's zero
character.

> +                               return -EACCES;
> +                       }
> +               }
>         }
>
>         return err;
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
Florent Revest March 16, 2021, 11:58 p.m. UTC | #4
On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > +               struct bpf_map *map = reg->map_ptr;
> > +               int map_off, i;
> > +               u64 map_addr;
> > +               char *map_ptr;
> > +
> > +               if (!map || !bpf_map_is_rdonly(map)) {
> > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > +                       return -EACCES;
> > +               }
> > +
> > +               if (!tnum_is_const(reg->var_off)) {
> > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > +                       return -EACCES;
> > +               }
> > +
> > +               if (!map->ops->map_direct_value_addr) {
> > +                       verbose(env, "no direct value access support for this map type\n");
> > +                       return -EACCES;
> > +               }
> > +
> > +               err = check_helper_mem_access(env, regno,
> > +                                             map->value_size - reg->off,
> > +                                             false, meta);
>
> you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> use check_map_access(). And double-check that register is of expected
> type. just the presence of ref->map_ptr might not be sufficient?

Sorry, just making sure I understand your comment correctly, are you
suggesting that we:
1- skip the check_map_access_type() currently done by
check_helper_mem_access()? or did you implicitly mean that we should
call it as well next to check_map_access() ?
2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
guaranteed by compatible_reg_types, just to stay on the safe side ?
Andrii Nakryiko March 17, 2021, 12:35 a.m. UTC | #5
On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
>
> On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > +               struct bpf_map *map = reg->map_ptr;
> > > +               int map_off, i;
> > > +               u64 map_addr;
> > > +               char *map_ptr;
> > > +
> > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               if (!tnum_is_const(reg->var_off)) {
> > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               if (!map->ops->map_direct_value_addr) {
> > > +                       verbose(env, "no direct value access support for this map type\n");
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               err = check_helper_mem_access(env, regno,
> > > +                                             map->value_size - reg->off,
> > > +                                             false, meta);
> >
> > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > use check_map_access(). And double-check that register is of expected
> > type. just the presence of ref->map_ptr might not be sufficient?
>
> Sorry, just making sure I understand your comment correctly, are you
> suggesting that we:
> 1- skip the check_map_access_type() currently done by
> check_helper_mem_access()? or did you implicitly mean that we should
> call it as well next to check_map_access() ?

check_helper_mem_access() will call check_map_access() for
PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
through check_helper_mem_access() if we know we need
check_map_access()? Less indirection, more explicit. So I meant
"replace check_helper_mem_access() with check_map_access()".

> 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> guaranteed by compatible_reg_types, just to stay on the safe side ?

I can't follow compatible_reg_types :( If it does, then I guess it's
fine without this check.
Florent Revest March 17, 2021, 12:45 a.m. UTC | #6
On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > +               struct bpf_map *map = reg->map_ptr;
> > > > +               int map_off, i;
> > > > +               u64 map_addr;
> > > > +               char *map_ptr;
> > > > +
> > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               if (!map->ops->map_direct_value_addr) {
> > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               err = check_helper_mem_access(env, regno,
> > > > +                                             map->value_size - reg->off,
> > > > +                                             false, meta);
> > >
> > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > use check_map_access(). And double-check that register is of expected
> > > type. just the presence of ref->map_ptr might not be sufficient?
> >
> > Sorry, just making sure I understand your comment correctly, are you
> > suggesting that we:
> > 1- skip the check_map_access_type() currently done by
> > check_helper_mem_access()? or did you implicitly mean that we should
> > call it as well next to check_map_access() ?
>
> check_helper_mem_access() will call check_map_access() for
> PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> through check_helper_mem_access() if we know we need
> check_map_access()? Less indirection, more explicit. So I meant
> "replace check_helper_mem_access() with check_map_access()".

Mhh I suspect there's still a misunderstanding, these function names
are really confusing ahah.
What about check_map_access*_type*. which is also called by
check_helper_mem_access (before check_map_access):
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329

Your message sounds like we should skip it so I was asking if that's
what you also implicitly meant or if you missed it?

> > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> > guaranteed by compatible_reg_types, just to stay on the safe side ?
>
> I can't follow compatible_reg_types :( If it does, then I guess it's
> fine without this check.

It's alright, I can keep an extra check just for safety. :)
Andrii Nakryiko March 17, 2021, 1:02 a.m. UTC | #7
On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote:
>
> On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > > +               struct bpf_map *map = reg->map_ptr;
> > > > > +               int map_off, i;
> > > > > +               u64 map_addr;
> > > > > +               char *map_ptr;
> > > > > +
> > > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               if (!map->ops->map_direct_value_addr) {
> > > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               err = check_helper_mem_access(env, regno,
> > > > > +                                             map->value_size - reg->off,
> > > > > +                                             false, meta);
> > > >
> > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > > use check_map_access(). And double-check that register is of expected
> > > > type. just the presence of ref->map_ptr might not be sufficient?
> > >
> > > Sorry, just making sure I understand your comment correctly, are you
> > > suggesting that we:
> > > 1- skip the check_map_access_type() currently done by
> > > check_helper_mem_access()? or did you implicitly mean that we should
> > > call it as well next to check_map_access() ?
> >
> > check_helper_mem_access() will call check_map_access() for
> > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> > through check_helper_mem_access() if we know we need
> > check_map_access()? Less indirection, more explicit. So I meant
> > "replace check_helper_mem_access() with check_map_access()".
>
> Mhh I suspect there's still a misunderstanding, these function names
> are really confusing ahah.
> What about check_map_access*_type*. which is also called by
> check_helper_mem_access (before check_map_access):
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329
>
> Your message sounds like we should skip it so I was asking if that's
> what you also implicitly meant or if you missed it?

ah, you meant READ/WRITE access? ok, let's keep
check_helper_mem_access() then, never mind me

>
> > > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> > > guaranteed by compatible_reg_types, just to stay on the safe side ?
> >
> > I can't follow compatible_reg_types :( If it does, then I guess it's
> > fine without this check.
>
> It's alright, I can keep an extra check just for safety. :)
Florent Revest March 17, 2021, 10:32 a.m. UTC | #8
On Wed, Mar 17, 2021 at 2:02 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote:
> > On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > > > +               struct bpf_map *map = reg->map_ptr;
> > > > > > +               int map_off, i;
> > > > > > +               u64 map_addr;
> > > > > > +               char *map_ptr;
> > > > > > +
> > > > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (!map->ops->map_direct_value_addr) {
> > > > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               err = check_helper_mem_access(env, regno,
> > > > > > +                                             map->value_size - reg->off,
> > > > > > +                                             false, meta);
> > > > >
> > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > > > use check_map_access(). And double-check that register is of expected
> > > > > type. just the presence of ref->map_ptr might not be sufficient?
> > > >
> > > > Sorry, just making sure I understand your comment correctly, are you
> > > > suggesting that we:
> > > > 1- skip the check_map_access_type() currently done by
> > > > check_helper_mem_access()? or did you implicitly mean that we should
> > > > call it as well next to check_map_access() ?
> > >
> > > check_helper_mem_access() will call check_map_access() for
> > > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> > > through check_helper_mem_access() if we know we need
> > > check_map_access()? Less indirection, more explicit. So I meant
> > > "replace check_helper_mem_access() with check_map_access()".
> >
> > Mhh I suspect there's still a misunderstanding, these function names
> > are really confusing ahah.
> > What about check_map_access*_type*. which is also called by
> > check_helper_mem_access (before check_map_access):
> > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329
> >
> > Your message sounds like we should skip it so I was asking if that's
> > what you also implicitly meant or if you missed it?
>
> ah, you meant READ/WRITE access? ok, let's keep
> check_helper_mem_access() then, never mind me

Ah cool, then we are on the same page :)
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a25730eaa148..7b5319d75b3e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,6 +308,7 @@  enum bpf_arg_type {
 	ARG_PTR_TO_PERCPU_BTF_ID,	/* pointer to in-kernel percpu type */
 	ARG_PTR_TO_FUNC,	/* pointer to a bpf program function */
 	ARG_PTR_TO_STACK_OR_NULL,	/* pointer to stack or NULL */
+	ARG_PTR_TO_CONST_STR,	/* pointer to a null terminated read-only string */
 	__BPF_ARG_TYPE_MAX,
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f9096b049cd6..c99b2b67dc8d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4601,6 +4601,7 @@  static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU
 static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
 static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
 static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
+static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
 
 static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_MAP_KEY]		= &map_key_value_types,
@@ -4631,6 +4632,7 @@  static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
 	[ARG_PTR_TO_FUNC]		= &func_ptr_types,
 	[ARG_PTR_TO_STACK_OR_NULL]	= &stack_ptr_types,
+	[ARG_PTR_TO_CONST_STR]		= &const_str_ptr_types,
 };
 
 static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
@@ -4881,6 +4883,45 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		if (err)
 			return err;
 		err = check_ptr_alignment(env, reg, 0, size, true);
+	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
+		struct bpf_map *map = reg->map_ptr;
+		int map_off, i;
+		u64 map_addr;
+		char *map_ptr;
+
+		if (!map || !bpf_map_is_rdonly(map)) {
+			verbose(env, "R%d does not point to a readonly map'\n", regno);
+			return -EACCES;
+		}
+
+		if (!tnum_is_const(reg->var_off)) {
+			verbose(env, "R%d is not a constant address'\n", regno);
+			return -EACCES;
+		}
+
+		if (!map->ops->map_direct_value_addr) {
+			verbose(env, "no direct value access support for this map type\n");
+			return -EACCES;
+		}
+
+		err = check_helper_mem_access(env, regno,
+					      map->value_size - reg->off,
+					      false, meta);
+		if (err)
+			return err;
+
+		map_off = reg->off + reg->var_off.value;
+		err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
+		if (err)
+			return err;
+
+		map_ptr = (char *)(map_addr);
+		for (i = map_off; map_ptr[i] != '\0'; i++) {
+			if (i == map->value_size - 1) {
+				verbose(env, "map does not contain a NULL-terminated string\n");
+				return -EACCES;
+			}
+		}
 	}
 
 	return err;