LinuxLists.cc - [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST

2021-03-10 22:04:47

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

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 <[email protected]>
---
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);
+ 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;
--
2.30.1.766.gb4fecdf3b7-goog

2021-03-11 00:09:11

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

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 <[email protected]>

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/[email protected]

Attachments:

(No filename) (15.49 kB)
.config.gz (28.62 kB)
Download all attachments

2021-03-11 01:03:27

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

Attachments:

(No filename) (10.83 kB)
.config.gz (40.03 kB)
Download all attachments

2021-03-16 11:16:52

by Andrii Nakryiko

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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 <[email protected]>
> ---
> 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
>

2021-03-17 00:12:20

by Florent Revest

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
<[email protected]> wrote:
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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 ?

2021-03-17 00:46:27

by Andrii Nakryiko

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <[email protected]> wrote:
>
> On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> <[email protected]> wrote:
> > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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.

2021-03-17 00:53:21

by Florent Revest

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
<[email protected]> wrote:
> On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <[email protected]> wrote:
> > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > <[email protected]> wrote:
> > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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. :)

2021-03-17 01:21:25

by Andrii Nakryiko

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <[email protected]> wrote:
>
> On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> <[email protected]> wrote:
> > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <[email protected]> wrote:
> > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > <[email protected]> wrote:
> > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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. :)

2021-03-17 10:34:52

by Florent Revest

[permalink] [raw]

Subject: Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

On Wed, Mar 17, 2021 at 2:02 AM Andrii Nakryiko
<[email protected]> wrote:
> On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <[email protected]> wrote:
> > On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> > <[email protected]> wrote:
> > > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <[email protected]> wrote:
> > > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > > <[email protected]> wrote:
> > > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <[email protected]> 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 :)