Re: [PATCH bpf-next v1 8/9] bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem.

From: Hao Luo <haoluo@google.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, KP Singh <kpsingh@kernel.org>,
	bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v1 8/9] bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem.
Date: Tue, 7 Dec 2021 19:49:24 -0800	[thread overview]
Message-ID: <CA+khW7h3VM+7CESWeFgheMkg20JckbxidC6Quy-02_kFJL96tA@mail.gmail.com> (raw)
In-Reply-To: <CAEf4Bzb3nyWbS4=e7M8Am5BvMSPbMhMzXPKvd3spk+D9vZg99g@mail.gmail.com>

On Mon, Dec 6, 2021 at 10:24 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Dec 6, 2021 at 3:22 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 tags the bpf_args compatible with immutable memory with
> > MEM_RDONLY flag. The arguments that don't have this flag will be
> > only compatible with mutable memory types, preventing the helper
> > from modifying a read-only memory. The bpf_args that have
> > MEM_RDONLY are compatible with both mutable memory and immutable
> > memory.
> >
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> >  include/linux/bpf.h      |  4 ++-
> >  kernel/bpf/btf.c         |  2 +-
> >  kernel/bpf/cgroup.c      |  2 +-
> >  kernel/bpf/helpers.c     |  8 ++---
> >  kernel/bpf/ringbuf.c     |  2 +-
> >  kernel/bpf/syscall.c     |  2 +-
> >  kernel/bpf/verifier.c    | 14 +++++++--
> >  kernel/trace/bpf_trace.c | 26 ++++++++--------
> >  net/core/filter.c        | 64 ++++++++++++++++++++--------------------
> >  9 files changed, 67 insertions(+), 57 deletions(-)
> >
[...]
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 44af65f07a82..a7c015a7b52d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4966,6 +4966,7 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env,
> >         return 0;
> >  }
> >
> > +
>
> nit: unnecessary extra empty line?
>

Ack.

> >  struct bpf_reg_types {
> >         const enum bpf_reg_type types[10];
> >         u32 *btf_id;
> > @@ -5012,7 +5013,6 @@ static const struct bpf_reg_types mem_types = {
> >                 PTR_TO_MAP_VALUE,
> >                 PTR_TO_MEM,
> >                 PTR_TO_BUF,
> > -               PTR_TO_BUF | MEM_RDONLY,
> >         },
> >  };
> >
> > @@ -5074,6 +5074,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >         struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> >         enum bpf_reg_type expected, type = reg->type;
> >         const struct bpf_reg_types *compatible;
> > +       u32 compatible_flags;
> >         int i, j;
> >
> >         compatible = compatible_reg_types[base_type(arg_type)];
> > @@ -5082,6 +5083,13 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >                 return -EFAULT;
> >         }
> >
> > +       /* If arg_type is tagged with MEM_RDONLY, it's compatible with both
> > +        * RDONLY and non-RDONLY reg values. Therefore fold this flag before
> > +        * comparison. PTR_MAYBE_NULL is similar.
> > +        */
> > +       compatible_flags = arg_type & (MEM_RDONLY | PTR_MAYBE_NULL);
> > +       type &= ~compatible_flags;
> > +
>
> wouldn't
>
> type &= ~MEM_RDONLY; /* clear read-only flag, if any */
> type &= ~PTR_MAYBE_NULL; /* clear nullable flag, if any */
>
> be cleaner and more straightforward?
>
>

No problem. Sounds good to me.

> >         for (i = 0; i < ARRAY_SIZE(compatible->types); i++) {
> >                 expected = compatible->types[i];
> >                 if (expected == NOT_INIT)
>
> [...]