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 7/9] bpf: Make per_cpu_ptr return rdonly PTR_TO_MEM.
Date: Tue, 7 Dec 2021 19:54:00 -0800 [thread overview]
Message-ID: <CA+khW7jtOweO4it68=ggqbe7QbdhPukE+FkgmAiTs-PeR28AiQ@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzbSA1+vE4vA6FSbJfUZDyYvyHJbiK1j5yD=vGbGA5EEhg@mail.gmail.com>
On Mon, Dec 6, 2021 at 10:18 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Dec 6, 2021 at 3:22 PM Hao Luo <haoluo@google.com> wrote:
> >
> > Tag the return type of {per, this}_cpu_ptr with RDONLY_MEM. The
> > returned value of this pair of helpers is kernel object, which
> > can not be updated by bpf programs. Previously these two helpers
> > return PTR_OT_MEM for kernel objects of scalar type, which allows
> > one to directly modify the memory. Now with RDONLY_MEM tagging,
> > the verifier will reject programs that writes into RDONLY_MEM.
> >
> > Fixes: 63d9b80dcf2c ("bpf: Introduce bpf_this_cpu_ptr()")
> > Fixes: eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()")
> > Fixes: 4976b718c355 ("bpf: Introduce pseudo_btf_id")
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> > kernel/bpf/helpers.c | 4 ++--
> > kernel/bpf/verifier.c | 33 ++++++++++++++++++++++++++++-----
> > 2 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 293d9314ec7f..a5e349c9d3e3 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -667,7 +667,7 @@ BPF_CALL_2(bpf_per_cpu_ptr, const void *, ptr, u32, cpu)
> > const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
> > .func = bpf_per_cpu_ptr,
> > .gpl_only = false,
> > - .ret_type = RET_PTR_TO_MEM_OR_BTF_ID | PTR_MAYBE_NULL,
> > + .ret_type = RET_PTR_TO_MEM_OR_BTF_ID | PTR_MAYBE_NULL | MEM_RDONLY,
> > .arg1_type = ARG_PTR_TO_PERCPU_BTF_ID,
> > .arg2_type = ARG_ANYTHING,
> > };
> > @@ -680,7 +680,7 @@ BPF_CALL_1(bpf_this_cpu_ptr, const void *, percpu_ptr)
> > const struct bpf_func_proto bpf_this_cpu_ptr_proto = {
> > .func = bpf_this_cpu_ptr,
> > .gpl_only = false,
> > - .ret_type = RET_PTR_TO_MEM_OR_BTF_ID,
> > + .ret_type = RET_PTR_TO_MEM_OR_BTF_ID | MEM_RDONLY,
> > .arg1_type = ARG_PTR_TO_PERCPU_BTF_ID,
> > };
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index f8b804918c35..44af65f07a82 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4296,16 +4296,32 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> > mark_reg_unknown(env, regs, value_regno);
> > }
> > }
> > - } else if (reg->type == PTR_TO_MEM) {
> > + } else if (base_type(reg->type) == PTR_TO_MEM) {
> > + bool rdonly_mem = type_is_rdonly_mem(reg->type);
> > +
> > + if (type_may_be_null(reg->type)) {
> > + verbose(env, "R%d invalid mem access '%s'\n", regno,
> > + reg_type_str(reg->type));
>
> see, here you'll get "invalid mem access 'ptr_to_mem'" while it's
> actually ptr_to_mem_or_null. Like verifier logs are not hard enough to
> follow, now they will be also misleading.
>
I think formatting string inside reg_type_str() can have this problem
solved, preserving the previous behavior. I'll try that in v2.
> > + return -EACCES;
> > + }
> > +
> > + if (t == BPF_WRITE && rdonly_mem) {
> > + verbose(env, "R%d cannot write into rdonly %s\n",
> > + regno, reg_type_str(reg->type));
> > + return -EACCES;
> > + }
> > +
> > if (t == BPF_WRITE && value_regno >= 0 &&
> > is_pointer_value(env, value_regno)) {
> > verbose(env, "R%d leaks addr into mem\n", value_regno);
> > return -EACCES;
> > }
> > +
> > err = check_mem_region_access(env, regno, off, size,
> > reg->mem_size, false);
> > - if (!err && t == BPF_READ && value_regno >= 0)
> > - mark_reg_unknown(env, regs, value_regno);
> > + if (!err && value_regno >= 0)
> > + if (t == BPF_READ || rdonly_mem)
>
> why two nested ifs for one condition?
>
No particular reason. I think it helped me understand the logic
better. But I'm fine with combining them into one 'if'.
> > + mark_reg_unknown(env, regs, value_regno);
> > } else if (reg->type == PTR_TO_CTX) {
> > enum bpf_reg_type reg_type = SCALAR_VALUE;
> > struct btf *btf = NULL;
>
> [...]
next prev parent reply other threads:[~2021-12-08 3:54 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-06 23:22 [PATCH bpf-next v1 0/9] Introduce composable bpf types Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 1/9] bpf: Introduce composable reg, ret and arg types Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 2/9] bpf: Replace ARG_XXX_OR_NULL with ARG_XXX | PTR_MAYBE_NULL Hao Luo
2021-12-07 5:45 ` Andrii Nakryiko
2021-12-07 18:52 ` Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 3/9] bpf: Replace RET_XXX_OR_NULL with RET_XXX " Hao Luo
2021-12-07 5:51 ` Andrii Nakryiko
2021-12-07 19:05 ` Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 4/9] bpf: Replace PTR_TO_XXX_OR_NULL with PTR_TO_XXX " Hao Luo
2021-12-07 6:08 ` Andrii Nakryiko
2021-12-08 3:37 ` Hao Luo
2021-12-08 20:03 ` Andrii Nakryiko
2021-12-09 21:45 ` Alexei Starovoitov
2021-12-10 19:56 ` Hao Luo
2021-12-13 1:51 ` Alexei Starovoitov
2021-12-13 2:02 ` Alexei Starovoitov
2021-12-17 0:32 ` Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 5/9] bpf: Introduce MEM_RDONLY flag Hao Luo
2021-12-07 6:14 ` Andrii Nakryiko
2021-12-08 3:41 ` Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 6/9] bpf: Convert PTR_TO_MEM_OR_NULL to composable types Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 7/9] bpf: Make per_cpu_ptr return rdonly PTR_TO_MEM Hao Luo
2021-12-07 6:18 ` Andrii Nakryiko
2021-12-08 3:54 ` Hao Luo [this message]
2021-12-10 17:42 ` Andrii Nakryiko
2021-12-10 18:36 ` Hao Luo
2021-12-06 23:22 ` [PATCH bpf-next v1 8/9] bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem Hao Luo
2021-12-07 6:23 ` Andrii Nakryiko
2021-12-08 3:49 ` Hao Luo
2021-12-09 20:04 ` Hao Luo
2021-12-10 17:55 ` Andrii Nakryiko
2021-12-06 23:22 ` [PATCH bpf-next v1 9/9] bpf/selftests: Test PTR_TO_RDONLY_MEM Hao Luo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+khW7jtOweO4it68=ggqbe7QbdhPukE+FkgmAiTs-PeR28AiQ@mail.gmail.com' \
--to=haoluo@google.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).