| Message ID | 20211025231256.4030142-3-haoluo@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Prevent writing read-only memory | expand |
On Mon, Oct 25, 2021 at 04:12:55PM -0700, Hao Luo wrote: > Some helper functions may modify its arguments, for example, > bpf_d_path, bpf_get_stack etc. Previously, their argument types > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > to modify a read-only memory by passing it into one of such helper > functions. > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > annotate the arguments that may be modified by the helpers. For > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > acceptable. > > In short, when a helper may modify its input parameter, use > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > only used in bpf_iter prog as the type of key, which hasn't been > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > has no consumers. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > Changes since v1: > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > read-only and read-write mem arg types. > > include/linux/bpf.h | 9 +++++++++ > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/helpers.c | 2 +- > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > kernel/trace/bpf_trace.c | 6 +++--- > net/core/filter.c | 6 +++--- > 6 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 7b47e8f344cb..586ce67d63a9 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -341,6 +341,15 @@ enum bpf_arg_type { > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > + * ARG_PTR_TO_MEM, this arg_type is not > + * compatible with RDONLY memory. If the > + * argument may be updated by the helper, > + * use this type. > + */ > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > + * ARG_PTR_TO_WRITABLE_MEM. > + */ Instead of adding new types, can we do something like this instead: diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index c8a78e830fca..5dbd2541aa86 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -68,7 +68,8 @@ struct bpf_reg_state { u32 btf_id; }; - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ /* Max size from any of the above. */ struct { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c6616e325803..ad46169d422b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4374,7 +4374,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn return -EACCES; } err = check_mem_region_access(env, regno, off, size, - reg->mem_size, false); + t == BPF_WRITE ? reg->wr_mem_size : reg->rd_mem_size, false); if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); } else if (reg->type == PTR_TO_CTX) { @@ -11511,7 +11511,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, goto err_put; } aux->btf_var.reg_type = PTR_TO_MEM; - aux->btf_var.mem_size = tsize; + aux->btf_var.rd_mem_size = tsize; + aux->btf_var.wr_mem_size = 0; } else { aux->btf_var.reg_type = PTR_TO_BTF_ID; aux->btf_var.btf = btf;
On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > Some helper functions may modify its arguments, for example, > bpf_d_path, bpf_get_stack etc. Previously, their argument types > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > to modify a read-only memory by passing it into one of such helper > functions. > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > annotate the arguments that may be modified by the helpers. For > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > acceptable. > > In short, when a helper may modify its input parameter, use > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. This is inconsistent with the other uses where we have something that's writable by default and mark it as RDONLY if it's read-only. Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the memory? It will also be safer default: if helper defines ARG_PTR_TO_MEM but never writes to it, worst thing that can happen would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it without fixing helper definition. The other way is more dangerous. If ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction and actually writes into the memory, then we are in much bigger trouble. > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > only used in bpf_iter prog as the type of key, which hasn't been > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > has no consumers. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > Changes since v1: > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > read-only and read-write mem arg types. > > include/linux/bpf.h | 9 +++++++++ > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/helpers.c | 2 +- > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > kernel/trace/bpf_trace.c | 6 +++--- > net/core/filter.c | 6 +++--- > 6 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 7b47e8f344cb..586ce67d63a9 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -341,6 +341,15 @@ enum bpf_arg_type { > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > + * ARG_PTR_TO_MEM, this arg_type is not > + * compatible with RDONLY memory. If the > + * argument may be updated by the helper, > + * use this type. > + */ > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > + * ARG_PTR_TO_WRITABLE_MEM. > + */ > __BPF_ARG_TYPE_MAX, > }; > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > index 03145d45e3d5..683269b7cd92 100644 > --- a/kernel/bpf/cgroup.c > +++ b/kernel/bpf/cgroup.c > @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { > .gpl_only = false, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 14531757087f..db98385ee7af 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { > .func = bpf_snprintf, > .gpl_only = true, > .ret_type = RET_INTEGER, > - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > .arg2_type = ARG_CONST_SIZE_OR_ZERO, > .arg3_type = ARG_PTR_TO_CONST_STR, > .arg4_type = ARG_PTR_TO_MEM_OR_NULL, > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index ae3ff297240e..d8817c3d990e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) > type == ARG_PTR_TO_CTX_OR_NULL || > type == ARG_PTR_TO_SOCKET_OR_NULL || > type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > type == ARG_PTR_TO_STACK_OR_NULL; > } > > @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) > { > return type == ARG_PTR_TO_MEM || > type == ARG_PTR_TO_MEM_OR_NULL || > + type == ARG_PTR_TO_WRITABLE_MEM || > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > type == ARG_PTR_TO_UNINIT_MEM; > } > > @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { > PTR_TO_MEM, > PTR_TO_RDONLY_BUF, > PTR_TO_RDWR_BUF, > + PTR_TO_RDONLY_MEM, > + }, > +}; > + > +static const struct bpf_reg_types writable_mem_types = { > + .types = { > + PTR_TO_STACK, > + PTR_TO_PACKET, > + PTR_TO_PACKET_META, > + PTR_TO_MAP_KEY, > + PTR_TO_MAP_VALUE, > + PTR_TO_MEM, > + PTR_TO_RDWR_BUF, > }, > }; > > @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, > + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, > [ARG_PTR_TO_INT] = &int_ptr_types, > [ARG_PTR_TO_LONG] = &int_ptr_types, > [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index cbcd0d6fca7c..5815845222de 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_BTF_ID, > .arg1_btf_id = &bpf_d_path_btf_ids[0], > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > .allowed = bpf_d_path_allowed, > }; > @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > .func = bpf_snprintf_btf, > .gpl_only = false, > .ret_type = RET_INTEGER, > - .arg1_type = ARG_PTR_TO_MEM, > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, > .arg2_type = ARG_CONST_SIZE, > .arg3_type = ARG_PTR_TO_MEM, > .arg4_type = ARG_CONST_SIZE, > @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > .arg4_type = ARG_ANYTHING, > }; > diff --git a/net/core/filter.c b/net/core/filter.c > index 8e8d3b49c297..7dadabe12ceb 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { > .gpl_only = true, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { > .gpl_only = false, > .ret_type = RET_INTEGER, > .arg1_type = ARG_PTR_TO_CTX, > - .arg2_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > .arg3_type = ARG_CONST_SIZE, > .arg4_type = ARG_ANYTHING, > }; > -- > 2.33.0.1079.g6e70778dc9-goog >
On Mon, Oct 25, 2021 at 8:48 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 04:12:55PM -0700, Hao Luo wrote: > > Some helper functions may modify its arguments, for example, > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > to modify a read-only memory by passing it into one of such helper > > functions. > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > annotate the arguments that may be modified by the helpers. For > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > acceptable. > > > > In short, when a helper may modify its input parameter, use > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > only used in bpf_iter prog as the type of key, which hasn't been > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > has no consumers. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > Changes since v1: > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > read-only and read-write mem arg types. > > > > include/linux/bpf.h | 9 +++++++++ > > kernel/bpf/cgroup.c | 2 +- > > kernel/bpf/helpers.c | 2 +- > > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > > kernel/trace/bpf_trace.c | 6 +++--- > > net/core/filter.c | 6 +++--- > > 6 files changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 7b47e8f344cb..586ce67d63a9 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -341,6 +341,15 @@ enum bpf_arg_type { > > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > > + * ARG_PTR_TO_MEM, this arg_type is not > > + * compatible with RDONLY memory. If the > > + * argument may be updated by the helper, > > + * use this type. > > + */ > > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > > + * ARG_PTR_TO_WRITABLE_MEM. > > + */ > > Instead of adding new types, > can we do something like this instead: > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index c8a78e830fca..5dbd2541aa86 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -68,7 +68,8 @@ struct bpf_reg_state { > u32 btf_id; > }; > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ This seems more confusing, it's technically possible to express a memory pointer from which you can read X bytes, but can write Y bytes. I actually liked the idea that helpers will be explicit about whether they can write into a memory or only read from it. Apart from a few more lines of code, are there any downsides to having PTR_TO_MEM vs PTR_TO_RDONLY_MEM? BTW, this made me think about read-only (from the BPF side) maps. Seems like we have some special handling around that right now, but if we had PTR_TO_RDONLY_MEM and PTR_TO_MEM, could we have used that as a more uniform way to enforce read-only access to memory? > > /* Max size from any of the above. */ > struct { > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c6616e325803..ad46169d422b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4374,7 +4374,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn > return -EACCES; > } > err = check_mem_region_access(env, regno, off, size, > - reg->mem_size, false); > + t == BPF_WRITE ? reg->wr_mem_size : reg->rd_mem_size, false); > if (!err && t == BPF_READ && value_regno >= 0) > mark_reg_unknown(env, regs, value_regno); > } else if (reg->type == PTR_TO_CTX) { > @@ -11511,7 +11511,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env, > goto err_put; > } > aux->btf_var.reg_type = PTR_TO_MEM; > - aux->btf_var.mem_size = tsize; > + aux->btf_var.rd_mem_size = tsize; > + aux->btf_var.wr_mem_size = 0; > } else { > aux->btf_var.reg_type = PTR_TO_BTF_ID; > aux->btf_var.btf = btf; >
On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > Some helper functions may modify its arguments, for example, > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > to modify a read-only memory by passing it into one of such helper > > functions. > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > annotate the arguments that may be modified by the helpers. For > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > acceptable. > > > > In short, when a helper may modify its input parameter, use > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > This is inconsistent with the other uses where we have something > that's writable by default and mark it as RDONLY if it's read-only. > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > memory? It will also be safer default: if helper defines > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > without fixing helper definition. The other way is more dangerous. If > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > and actually writes into the memory, then we are in much bigger > trouble. > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I find it's not intuitive for developers when adding helpers. The majority of PTR_TO_MEM arguments are read-only. When adding a new helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) to match the default case (that is, read-only argument). Requiring explicitly adding RDONLY to most cases seems a little unintuitive to me. But other than that, I agree that narrowing ARG_PTR_TO_MEM down to writable memory fosters more strict checks and safer behavior. But when people add helpers, they are clearly aware which argument will be modified and which will not. IMHO I do trust that the developers and the reviewers can choose the right type at the review time. :) > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > only used in bpf_iter prog as the type of key, which hasn't been > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > has no consumers. > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > --- > > Changes since v1: > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > read-only and read-write mem arg types. > > > > include/linux/bpf.h | 9 +++++++++ > > kernel/bpf/cgroup.c | 2 +- > > kernel/bpf/helpers.c | 2 +- > > kernel/bpf/verifier.c | 18 ++++++++++++++++++ > > kernel/trace/bpf_trace.c | 6 +++--- > > net/core/filter.c | 6 +++--- > > 6 files changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 7b47e8f344cb..586ce67d63a9 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -341,6 +341,15 @@ enum bpf_arg_type { > > ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ > > ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ > > ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ > > + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to > > + * ARG_PTR_TO_MEM, this arg_type is not > > + * compatible with RDONLY memory. If the > > + * argument may be updated by the helper, > > + * use this type. > > + */ > > + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to > > + * ARG_PTR_TO_WRITABLE_MEM. > > + */ > > __BPF_ARG_TYPE_MAX, > > }; > > > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c > > index 03145d45e3d5..683269b7cd92 100644 > > --- a/kernel/bpf/cgroup.c > > +++ b/kernel/bpf/cgroup.c > > @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index 14531757087f..db98385ee7af 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { > > .func = bpf_snprintf, > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, > > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > > .arg2_type = ARG_CONST_SIZE_OR_ZERO, > > .arg3_type = ARG_PTR_TO_CONST_STR, > > .arg4_type = ARG_PTR_TO_MEM_OR_NULL, > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index ae3ff297240e..d8817c3d990e 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) > > type == ARG_PTR_TO_CTX_OR_NULL || > > type == ARG_PTR_TO_SOCKET_OR_NULL || > > type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || > > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > > type == ARG_PTR_TO_STACK_OR_NULL; > > } > > > > @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) > > { > > return type == ARG_PTR_TO_MEM || > > type == ARG_PTR_TO_MEM_OR_NULL || > > + type == ARG_PTR_TO_WRITABLE_MEM || > > + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || > > type == ARG_PTR_TO_UNINIT_MEM; > > } > > > > @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { > > PTR_TO_MEM, > > PTR_TO_RDONLY_BUF, > > PTR_TO_RDWR_BUF, > > + PTR_TO_RDONLY_MEM, > > + }, > > +}; > > + > > +static const struct bpf_reg_types writable_mem_types = { > > + .types = { > > + PTR_TO_STACK, > > + PTR_TO_PACKET, > > + PTR_TO_PACKET_META, > > + PTR_TO_MAP_KEY, > > + PTR_TO_MAP_VALUE, > > + PTR_TO_MEM, > > + PTR_TO_RDWR_BUF, > > }, > > }; > > > > @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > > [ARG_PTR_TO_UNINIT_MEM] = &mem_types, > > [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, > > [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, > > + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, > > + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, > > [ARG_PTR_TO_INT] = &int_ptr_types, > > [ARG_PTR_TO_LONG] = &int_ptr_types, > > [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index cbcd0d6fca7c..5815845222de 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_BTF_ID, > > .arg1_btf_id = &bpf_d_path_btf_ids[0], > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > > .allowed = bpf_d_path_allowed, > > }; > > @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > > .func = bpf_snprintf_btf, > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > - .arg1_type = ARG_PTR_TO_MEM, > > + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg2_type = ARG_CONST_SIZE, > > .arg3_type = ARG_PTR_TO_MEM, > > .arg4_type = ARG_CONST_SIZE, > > @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, > > .arg3_type = ARG_CONST_SIZE_OR_ZERO, > > .arg4_type = ARG_ANYTHING, > > }; > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 8e8d3b49c297..7dadabe12ceb 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { > > .gpl_only = true, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > .arg1_type = ARG_PTR_TO_CTX, > > - .arg2_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, > > .arg3_type = ARG_CONST_SIZE, > > .arg4_type = ARG_ANYTHING, > > }; > > -- > > 2.33.0.1079.g6e70778dc9-goog > >
On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > Instead of adding new types, > > can we do something like this instead: > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > index c8a78e830fca..5dbd2541aa86 100644 > > --- a/include/linux/bpf_verifier.h > > +++ b/include/linux/bpf_verifier.h > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > u32 btf_id; > > }; > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > This seems more confusing, it's technically possible to express a > memory pointer from which you can read X bytes, but can write Y bytes. I'm fine it being a new flag instead of wr_mem_size. > I actually liked the idea that helpers will be explicit about whether > they can write into a memory or only read from it. > > Apart from a few more lines of code, are there any downsides to having > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? because it's a churn and non scalable long term. It's not just PTR_TO_RDONLY_MEM. It's also ARG_PTR_TO_RDONLY_MEM, and RET_PTR_TO_RDONLY_MEM, and PTR_TO_RDONLY_MEM_OR_NULL and *_OR_BTF_ID, and *_OR_BTF_ID_OR_NULL. It felt that expressing readonly-ness as a flag in bpf_reg_state will make it easier to get right in the code and extend in the future. May be we will have a kernel vs user flag for PTR_TO_MEM in the future. If we use different name to express that we will have: PTR_TO_USER_RDONLY_MEM and PTR_TO_USER_MEM plus all variants of ARG_* and RET_* and *_OR_NULL. With a flag approach it will be just another flag in bpf_reg_state.
On Tue, Oct 26, 2021 at 11:00 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > Instead of adding new types, > > > can we do something like this instead: > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > index c8a78e830fca..5dbd2541aa86 100644 > > > --- a/include/linux/bpf_verifier.h > > > +++ b/include/linux/bpf_verifier.h > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > u32 btf_id; > > > }; > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > This seems more confusing, it's technically possible to express a > > memory pointer from which you can read X bytes, but can write Y bytes. > > I'm fine it being a new flag instead of wr_mem_size. > > > I actually liked the idea that helpers will be explicit about whether > > they can write into a memory or only read from it. > > > > Apart from a few more lines of code, are there any downsides to having > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > because it's a churn and non scalable long term. > It's not just PTR_TO_RDONLY_MEM. > It's also ARG_PTR_TO_RDONLY_MEM, > and RET_PTR_TO_RDONLY_MEM, > and PTR_TO_RDONLY_MEM_OR_NULL > and *_OR_BTF_ID, > and *_OR_BTF_ID_OR_NULL. > It felt that expressing readonly-ness as a flag in bpf_reg_state > will make it easier to get right in the code and extend in the future. > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > If we use different name to express that we will have: > PTR_TO_USER_RDONLY_MEM and > PTR_TO_USER_MEM > plus all variants of ARG_* and RET_* and *_OR_NULL. > With a flag approach it will be just another flag in bpf_reg_state. Totally agree. Adding a variant incurs exponential cost. Introducing another dimension in future may need to go over all the MEM, RDONLY_MEM, MEM_OR_NULL, RDONLY_MEM_OR_NULL, multiplied by ARG_*, RET_*, etc. It's a pain. I have that in mind and start thinking more about how can we do a more scalable flag approach.
On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > Instead of adding new types, > > > can we do something like this instead: > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > index c8a78e830fca..5dbd2541aa86 100644 > > > --- a/include/linux/bpf_verifier.h > > > +++ b/include/linux/bpf_verifier.h > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > u32 btf_id; > > > }; > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > This seems more confusing, it's technically possible to express a > > memory pointer from which you can read X bytes, but can write Y bytes. > > I'm fine it being a new flag instead of wr_mem_size. > > > I actually liked the idea that helpers will be explicit about whether > > they can write into a memory or only read from it. > > > > Apart from a few more lines of code, are there any downsides to having > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > because it's a churn and non scalable long term. > It's not just PTR_TO_RDONLY_MEM. > It's also ARG_PTR_TO_RDONLY_MEM, > and RET_PTR_TO_RDONLY_MEM, > and PTR_TO_RDONLY_MEM_OR_NULL > and *_OR_BTF_ID, > and *_OR_BTF_ID_OR_NULL. > It felt that expressing readonly-ness as a flag in bpf_reg_state > will make it easier to get right in the code and extend in the future. That's true, but while it's easy to add a flag to bpf_reg_state, it's not easy to do the same for BPF helper input (ARG_PTR_xxx) and output (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR with flags, it seems more consistent to keep the same pure enum approach for reg_state. > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > If we use different name to express that we will have: > PTR_TO_USER_RDONLY_MEM and > PTR_TO_USER_MEM > plus all variants of ARG_* and RET_* and *_OR_NULL. > With a flag approach it will be just another flag in bpf_reg_state. All true, but then maybe we should rethink how we do all those enums. And instead of having all the _OR_NULL variants, it should be ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the basic set of register/input/output type enums? Same for ARG_RDONLY flag. Same could technically be done for USER vs KERNEL memory in the future. It's definitely a bunch of code changes, but if we are worried about an explosion of enum values, it might be the right move? On the other hand, if there are all those different variations and each is handled slightly differently, we'll have to have different logic for each of them. And whether it's an enum + flags, or a few more enumerators, doesn't change anything fundamentally. I feel like enums make code discovery a bit simpler in practice, but it's subjective.
On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote: > > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > > > Some helper functions may modify its arguments, for example, > > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > > to modify a read-only memory by passing it into one of such helper > > > functions. > > > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > > annotate the arguments that may be modified by the helpers. For > > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > > acceptable. > > > > > > In short, when a helper may modify its input parameter, use > > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > This is inconsistent with the other uses where we have something > > that's writable by default and mark it as RDONLY if it's read-only. > > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > > memory? It will also be safer default: if helper defines > > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > > without fixing helper definition. The other way is more dangerous. If > > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > > and actually writes into the memory, then we are in much bigger > > trouble. > > > > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I > find it's not intuitive for developers when adding helpers. The > majority of PTR_TO_MEM arguments are read-only. When adding a new > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) > to match the default case (that is, read-only argument). Requiring > explicitly adding RDONLY to most cases seems a little unintuitive to > me. > > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to > writable memory fosters more strict checks and safer behavior. But > when people add helpers, they are clearly aware which argument will be > modified and which will not. IMHO I do trust that the developers and > the reviewers can choose the right type at the review time. :) I don't trust myself, and neither should you :) See 5b029a32cfe4 ("bpf: Fix ringbuf helper function compatibility") as an example of the things that shouldn't have happened, but slipped through my own testing and code review anyway. And there were multiple cases where we accidentally enabled stuff that we shouldn't or didn't check something that should have been prevented. All that is to say that if we can have safer behavior by default (not as enforced by humans), then it's better. In this sense, ARG_PTR_TO_MEM meaning writable access is safer, because even if we accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst thing is that users won't be able to use helper in some situation and hopefully will report this and we'll fix it. The alternative is that a helper declares the argument as read-only memory (accidentally, because it's shorter enum and sort of default), but actually does the write to that memory. Now we have a much bigger issue. > > > > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > > only used in bpf_iter prog as the type of key, which hasn't been > > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > > has no consumers. > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > --- > > > Changes since v1: > > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > > read-only and read-write mem arg types. > > > [...]
On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > Instead of adding new types, > > > > can we do something like this instead: > > > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > > index c8a78e830fca..5dbd2541aa86 100644 > > > > --- a/include/linux/bpf_verifier.h > > > > +++ b/include/linux/bpf_verifier.h > > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > > u32 btf_id; > > > > }; > > > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > > This seems more confusing, it's technically possible to express a > > > memory pointer from which you can read X bytes, but can write Y bytes. > > > > I'm fine it being a new flag instead of wr_mem_size. > > > > > I actually liked the idea that helpers will be explicit about whether > > > they can write into a memory or only read from it. > > > > > > Apart from a few more lines of code, are there any downsides to having > > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > > > because it's a churn and non scalable long term. > > It's not just PTR_TO_RDONLY_MEM. > > It's also ARG_PTR_TO_RDONLY_MEM, > > and RET_PTR_TO_RDONLY_MEM, > > and PTR_TO_RDONLY_MEM_OR_NULL > > and *_OR_BTF_ID, > > and *_OR_BTF_ID_OR_NULL. > > It felt that expressing readonly-ness as a flag in bpf_reg_state > > will make it easier to get right in the code and extend in the future. > > That's true, but while it's easy to add a flag to bpf_reg_state, it's > not easy to do the same for BPF helper input (ARG_PTR_xxx) and output > (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR > with flags, it seems more consistent to keep the same pure enum > approach for reg_state. > > > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > > If we use different name to express that we will have: > > PTR_TO_USER_RDONLY_MEM and > > PTR_TO_USER_MEM > > plus all variants of ARG_* and RET_* and *_OR_NULL. > > With a flag approach it will be just another flag in bpf_reg_state. > > All true, but then maybe we should rethink how we do all those enums. > And instead of having all the _OR_NULL variants, it should be > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the > basic set of register/input/output type enums? Same for ARG_RDONLY > flag. Same could technically be done for USER vs KERNEL memory in the > future. Exactly. OR_NULL is such a flag and we already struggled to differentiate that flag with truly_not_equal_to_NULL and may_be_NULL. That's why all bpf_skc* helpers have additional run-time !NULL check. ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner. And ARG_RDONLY would fit that model well. > It's definitely a bunch of code changes, but if we are worried about > an explosion of enum values, it might be the right move? > > On the other hand, if there are all those different variations and > each is handled slightly differently, we'll have to have different > logic for each of them. And whether it's an enum + flags, or a few > more enumerators, doesn't change anything fundamentally. I feel like > enums make code discovery a bit simpler in practice, but it's > subjective. I think it's a bit of a mess already. ARG_PTR_TO_BTF_ID has may_be_NULL flag. Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON. but RET_PTR_TO_BTF_ID doesn't. PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either. imo cleaning up OR_NULL will be very nice. RDONLY would be an addition on top. We can probably fold UNINIT as a flag too. All that will be a big change, but I think it's worth it.
On Tue, Oct 26, 2021 at 11:53 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 10:51 AM Hao Luo <haoluo@google.com> wrote: > > > > On Mon, Oct 25, 2021 at 10:06 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Mon, Oct 25, 2021 at 4:13 PM Hao Luo <haoluo@google.com> wrote: > > > > > > > > Some helper functions may modify its arguments, for example, > > > > bpf_d_path, bpf_get_stack etc. Previously, their argument types > > > > were marked as ARG_PTR_TO_MEM, which is compatible with read-only > > > > mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate > > > > to modify a read-only memory by passing it into one of such helper > > > > functions. > > > > > > > > This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to > > > > annotate the arguments that may be modified by the helpers. For > > > > arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, > > > > while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not > > > > acceptable. > > > > > > > > In short, when a helper may modify its input parameter, use > > > > ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. > > > > > > This is inconsistent with the other uses where we have something > > > that's writable by default and mark it as RDONLY if it's read-only. > > > Same here, why not keep ARG_PTR_TO_MEM to mean "writable memory", and > > > add ARG_PTR_TO_RDONLY_MEM for helpers that are not writing into the > > > memory? It will also be safer default: if helper defines > > > ARG_PTR_TO_MEM but never writes to it, worst thing that can happen > > > would be that you won't be able to pass REG_PTR_TO_RDONLY_MEM into it > > > without fixing helper definition. The other way is more dangerous. If > > > ARG_PTR_TO_MEM means read-only mem and helper forgot this distinction > > > and actually writes into the memory, then we are in much bigger > > > trouble. > > > > > > > My original implementation was adding ARG_PTR_TO_RDONLY_MEM. But I > > find it's not intuitive for developers when adding helpers. The > > majority of PTR_TO_MEM arguments are read-only. When adding a new > > helper, I would expect the default arg type (that is, ARG_PTR_TO_MEM) > > to match the default case (that is, read-only argument). Requiring > > explicitly adding RDONLY to most cases seems a little unintuitive to > > me. > > > > But other than that, I agree that narrowing ARG_PTR_TO_MEM down to > > writable memory fosters more strict checks and safer behavior. But > > when people add helpers, they are clearly aware which argument will be > > modified and which will not. IMHO I do trust that the developers and > > the reviewers can choose the right type at the review time. :) > > I don't trust myself, and neither should you :) See 5b029a32cfe4 > ("bpf: Fix ringbuf helper function compatibility") as an example of > the things that shouldn't have happened, but slipped through my own > testing and code review anyway. And there were multiple cases where we > accidentally enabled stuff that we shouldn't or didn't check something > that should have been prevented. > > All that is to say that if we can have safer behavior by default (not > as enforced by humans), then it's better. In this sense, > ARG_PTR_TO_MEM meaning writable access is safer, because even if we > accidentally forget to mark some input as ARG_PTR_TO_RDONLY_MEM, worst > thing is that users won't be able to use helper in some situation and > hopefully will report this and we'll fix it. The alternative is that a > helper declares the argument as read-only memory (accidentally, > because it's shorter enum and sort of default), but actually does the > write to that memory. Now we have a much bigger issue. > Acknowledged. Will make this change in this next iteration. > > > > > > > > > > So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM > > > > is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is > > > > only used in bpf_iter prog as the type of key, which hasn't been > > > > used in the affected helper functions. PTR_TO_RDONLY_MEM currently > > > > has no consumers. > > > > > > > > Signed-off-by: Hao Luo <haoluo@google.com> > > > > --- > > > > Changes since v1: > > > > - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate > > > > read-only and read-write mem arg types. > > > > > > [...]
On Tue, Oct 26, 2021 at 12:23 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 11:45 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 26, 2021 at 10:59 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Mon, Oct 25, 2021 at 10:14 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > Instead of adding new types, > > > > > can we do something like this instead: > > > > > > > > > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > > > > > index c8a78e830fca..5dbd2541aa86 100644 > > > > > --- a/include/linux/bpf_verifier.h > > > > > +++ b/include/linux/bpf_verifier.h > > > > > @@ -68,7 +68,8 @@ struct bpf_reg_state { > > > > > u32 btf_id; > > > > > }; > > > > > > > > > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > + u32 rd_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > + u32 wr_mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > > > > > > > > This seems more confusing, it's technically possible to express a > > > > memory pointer from which you can read X bytes, but can write Y bytes. > > > > > > I'm fine it being a new flag instead of wr_mem_size. > > > > > > > I actually liked the idea that helpers will be explicit about whether > > > > they can write into a memory or only read from it. > > > > > > > > Apart from a few more lines of code, are there any downsides to having > > > > PTR_TO_MEM vs PTR_TO_RDONLY_MEM? > > > > > > because it's a churn and non scalable long term. > > > It's not just PTR_TO_RDONLY_MEM. > > > It's also ARG_PTR_TO_RDONLY_MEM, > > > and RET_PTR_TO_RDONLY_MEM, > > > and PTR_TO_RDONLY_MEM_OR_NULL > > > and *_OR_BTF_ID, > > > and *_OR_BTF_ID_OR_NULL. > > > It felt that expressing readonly-ness as a flag in bpf_reg_state > > > will make it easier to get right in the code and extend in the future. > > > > That's true, but while it's easy to add a flag to bpf_reg_state, it's > > not easy to do the same for BPF helper input (ARG_PTR_xxx) and output > > (RET_PTR_xxx) restrictions. So unless we extend ARG_PTR and RET_PTR > > with flags, it seems more consistent to keep the same pure enum > > approach for reg_state. > > > > > May be we will have a kernel vs user flag for PTR_TO_MEM in the future. > > > If we use different name to express that we will have: > > > PTR_TO_USER_RDONLY_MEM and > > > PTR_TO_USER_MEM > > > plus all variants of ARG_* and RET_* and *_OR_NULL. > > > With a flag approach it will be just another flag in bpf_reg_state. > > > > All true, but then maybe we should rethink how we do all those enums. > > And instead of having all the _OR_NULL variants, it should be > > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE flag that can be or-ed with the > > basic set of register/input/output type enums? Same for ARG_RDONLY > > flag. Same could technically be done for USER vs KERNEL memory in the > > future. > > Exactly. OR_NULL is such a flag and we already struggled to > differentiate that flag with truly_not_equal_to_NULL and may_be_NULL. > That's why all bpf_skc* helpers have additional run-time !NULL check. > > ARG_NULLABLE/REG_NULLABLE/RET_NULLABLE would make it cleaner. > And ARG_RDONLY would fit that model well. > > > It's definitely a bunch of code changes, but if we are worried about > > an explosion of enum values, it might be the right move? > > > > On the other hand, if there are all those different variations and > > each is handled slightly differently, we'll have to have different > > logic for each of them. And whether it's an enum + flags, or a few > > more enumerators, doesn't change anything fundamentally. I feel like > > enums make code discovery a bit simpler in practice, but it's > > subjective. > > I think it's a bit of a mess already. > ARG_PTR_TO_BTF_ID has may_be_NULL flag. > Just like ARG_PTR_TO_BTF_ID_SOCK_COMMON. > but RET_PTR_TO_BTF_ID doesn't. > PTR_TO_BTF_ID doesn't have that may_be_NULL assumption either. > > imo cleaning up OR_NULL will be very nice. > RDONLY would be an addition on top. > We can probably fold UNINIT as a flag too. > > All that will be a big change, but I think it's worth it. Yep, agree.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 7b47e8f344cb..586ce67d63a9 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -341,6 +341,15 @@ enum bpf_arg_type { ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */ ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */ ARG_PTR_TO_TIMER, /* pointer to bpf_timer */ + ARG_PTR_TO_WRITABLE_MEM, /* pointer to valid memory. Compared to + * ARG_PTR_TO_MEM, this arg_type is not + * compatible with RDONLY memory. If the + * argument may be updated by the helper, + * use this type. + */ + ARG_PTR_TO_WRITABLE_MEM_OR_NULL, /* pointer to memory or null, similar to + * ARG_PTR_TO_WRITABLE_MEM. + */ __BPF_ARG_TYPE_MAX, }; diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 03145d45e3d5..683269b7cd92 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1666,7 +1666,7 @@ static const struct bpf_func_proto bpf_sysctl_get_name_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 14531757087f..db98385ee7af 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1008,7 +1008,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { .func = bpf_snprintf, .gpl_only = true, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, + .arg1_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, .arg2_type = ARG_CONST_SIZE_OR_ZERO, .arg3_type = ARG_PTR_TO_CONST_STR, .arg4_type = ARG_PTR_TO_MEM_OR_NULL, diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index ae3ff297240e..d8817c3d990e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -486,6 +486,7 @@ static bool arg_type_may_be_null(enum bpf_arg_type type) type == ARG_PTR_TO_CTX_OR_NULL || type == ARG_PTR_TO_SOCKET_OR_NULL || type == ARG_PTR_TO_ALLOC_MEM_OR_NULL || + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || type == ARG_PTR_TO_STACK_OR_NULL; } @@ -4971,6 +4972,8 @@ static bool arg_type_is_mem_ptr(enum bpf_arg_type type) { return type == ARG_PTR_TO_MEM || type == ARG_PTR_TO_MEM_OR_NULL || + type == ARG_PTR_TO_WRITABLE_MEM || + type == ARG_PTR_TO_WRITABLE_MEM_OR_NULL || type == ARG_PTR_TO_UNINIT_MEM; } @@ -5075,6 +5078,19 @@ static const struct bpf_reg_types mem_types = { PTR_TO_MEM, PTR_TO_RDONLY_BUF, PTR_TO_RDWR_BUF, + PTR_TO_RDONLY_MEM, + }, +}; + +static const struct bpf_reg_types writable_mem_types = { + .types = { + PTR_TO_STACK, + PTR_TO_PACKET, + PTR_TO_PACKET_META, + PTR_TO_MAP_KEY, + PTR_TO_MAP_VALUE, + PTR_TO_MEM, + PTR_TO_RDWR_BUF, }, }; @@ -5125,6 +5141,8 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { [ARG_PTR_TO_UNINIT_MEM] = &mem_types, [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types, [ARG_PTR_TO_ALLOC_MEM_OR_NULL] = &alloc_mem_types, + [ARG_PTR_TO_WRITABLE_MEM] = &writable_mem_types, + [ARG_PTR_TO_WRITABLE_MEM_OR_NULL] = &writable_mem_types, [ARG_PTR_TO_INT] = &int_ptr_types, [ARG_PTR_TO_LONG] = &int_ptr_types, [ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types, diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index cbcd0d6fca7c..5815845222de 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -945,7 +945,7 @@ static const struct bpf_func_proto bpf_d_path_proto = { .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_BTF_ID, .arg1_btf_id = &bpf_d_path_btf_ids[0], - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .allowed = bpf_d_path_allowed, }; @@ -1002,7 +1002,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { .func = bpf_snprintf_btf, .gpl_only = false, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM, + .arg1_type = ARG_PTR_TO_WRITABLE_MEM, .arg2_type = ARG_CONST_SIZE, .arg3_type = ARG_PTR_TO_MEM, .arg4_type = ARG_CONST_SIZE, @@ -1433,7 +1433,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM_OR_NULL, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .arg4_type = ARG_ANYTHING, }; diff --git a/net/core/filter.c b/net/core/filter.c index 8e8d3b49c297..7dadabe12ceb 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5639,7 +5639,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -5694,7 +5694,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -6977,7 +6977,7 @@ static const struct bpf_func_proto bpf_sock_ops_load_hdr_opt_proto = { .gpl_only = false, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_WRITABLE_MEM, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, };
Some helper functions may modify its arguments, for example, bpf_d_path, bpf_get_stack etc. Previously, their argument types were marked as ARG_PTR_TO_MEM, which is compatible with read-only mem types, such as PTR_TO_RDONLY_BUF. Therefore it's legitimate to modify a read-only memory by passing it into one of such helper functions. This patch introduces a new arg type ARG_PTR_TO_WRITABLE_MEM to annotate the arguments that may be modified by the helpers. For arguments that are of ARG_PTR_TO_MEM, it's ok to take any mem type, while for ARG_PTR_TO_WRITABLE_MEM, readonly mem reg types are not acceptable. In short, when a helper may modify its input parameter, use ARG_PTR_TO_WRITABLE_MEM instead of ARG_PTR_TO_MEM. So far the difference between ARG_PTR_TO_MEM and ARG_PTR_TO_WRITABLE_MEM is PTR_TO_RDONLY_BUF and PTR_TO_RDONLY_MEM. PTR_TO_RDONLY_BUF is only used in bpf_iter prog as the type of key, which hasn't been used in the affected helper functions. PTR_TO_RDONLY_MEM currently has no consumers. Signed-off-by: Hao Luo <haoluo@google.com> --- Changes since v1: - new patch, introduced ARG_PTR_TO_WRITABLE_MEM to differentiate read-only and read-write mem arg types. include/linux/bpf.h | 9 +++++++++ kernel/bpf/cgroup.c | 2 +- kernel/bpf/helpers.c | 2 +- kernel/bpf/verifier.c | 18 ++++++++++++++++++ kernel/trace/bpf_trace.c | 6 +++--- net/core/filter.c | 6 +++--- 6 files changed, 35 insertions(+), 8 deletions(-)