| Message ID | 20210324022211.1718762-3-revest@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Add a snprintf eBPF helper | expand |
| 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 | success | Errors and warnings before: 12172 this patch: 12172 |
| 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 |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 12678 this patch: 12678 |
| netdev/header_inline | success | Link |
On Tue, Mar 23, 2021 at 7:23 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 zero 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 | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 39 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, > }; > [...] > + > + map_off = reg->off + reg->var_off.value; > + err = map->ops->map_direct_value_addr(map, &map_addr, map_off); > + if (err) > + return err; > + > + str_ptr = (char *)(long)(map_addr); > + if (!strnchr(str_ptr + map_off, > + map->value_size - reg->off - map_off, 0)) you are double subtracting reg->off here. isn't map->value_size - map_off what you want? > + verbose(env, "string is not zero-terminated\n"); I'd prefer `return -EINVAL;`, but at least set err, otherwise what's the point? > } > > return err; > -- > 2.31.0.291.g576ba9dcdaf-goog >
On Fri, Mar 26, 2021 at 11:23 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Tue, Mar 23, 2021 at 7:23 PM Florent Revest <revest@chromium.org> wrote: > > + > > + map_off = reg->off + reg->var_off.value; > > + err = map->ops->map_direct_value_addr(map, &map_addr, map_off); > > + if (err) > > + return err; > > + > > + str_ptr = (char *)(long)(map_addr); > > + if (!strnchr(str_ptr + map_off, > > + map->value_size - reg->off - map_off, 0)) > > you are double subtracting reg->off here. isn't map->value_size - > map_off what you want? Good catch! > > + verbose(env, "string is not zero-terminated\n"); > > I'd prefer `return -EINVAL;`, but at least set err, otherwise what's the point? Ah yeah, absolutely.
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 e26c5170c953..9e03608725b4 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,42 @@ 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; + u64 map_addr; + char *str_ptr; + + if (reg->type != PTR_TO_MAP_VALUE || !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_map_access(env, regno, reg->off, + map->value_size - reg->off, false); + 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; + + str_ptr = (char *)(long)(map_addr); + if (!strnchr(str_ptr + map_off, + map->value_size - reg->off - map_off, 0)) + verbose(env, "string is not zero-terminated\n"); } return err;
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 zero 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 | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)