On Mon, Oct 25, 2021 at 8:48 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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;
>
