From: Song Liu <songliubraving@fb.com>
To: Jann Horn <jannh@google.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
Martin Lau <kafai@fb.com>, Yonghong Song <yhs@fb.com>,
Andrii Nakryiko <andriin@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf] bpf: Use pointer type whitelist for XADD
Date: Thu, 16 Apr 2020 20:52:20 +0000 [thread overview]
Message-ID: <9229787E-6243-4F22-B809-819A3F24B5DD@fb.com> (raw)
In-Reply-To: <20200415204743.206086-1-jannh@google.com>
> On Apr 15, 2020, at 1:47 PM, Jann Horn <jannh@google.com> wrote:
>
> At the moment, check_xadd() uses a blacklist to decide whether a given
> pointer type should be usable with the XADD instruction. Out of all the
> pointer types that check_mem_access() accepts, only four are currently let
> through by check_xadd():
>
> PTR_TO_MAP_VALUE
> PTR_TO_CTX rejected
> PTR_TO_STACK
> PTR_TO_PACKET rejected
> PTR_TO_PACKET_META rejected
> PTR_TO_FLOW_KEYS rejected
> PTR_TO_SOCKET rejected
> PTR_TO_SOCK_COMMON rejected
> PTR_TO_TCP_SOCK rejected
> PTR_TO_XDP_SOCK rejected
> PTR_TO_TP_BUFFER
> PTR_TO_BTF_ID
>
> Looking at the currently permitted ones:
>
> - PTR_TO_MAP_VALUE: This makes sense and is the primary usecase for XADD.
> - PTR_TO_STACK: This doesn't make much sense, there is no concurrency on
> the BPF stack. It also causes confusion further down, because the first
> check_mem_access() won't check whether the stack slot being read from is
> STACK_SPILL and the second check_mem_access() assumes in
> check_stack_write() that the value being written is a normal scalar.
> This means that unprivileged users can leak kernel pointers.
> - PTR_TO_TP_BUFFER: This is a local output buffer without concurrency.
> - PTR_TO_BTF_ID: This is read-only, XADD can't work. When the verifier
> tries to verify XADD on such memory, the first check_ptr_to_btf_access()
> invocation gets confused by value_regno not being a valid array index
> and writes to out-of-bounds memory.
>
> Limit XADD to PTR_TO_MAP_VALUE, since everything else at least doesn't make
> sense, and is sometimes broken on top of that.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> I'm just sending this on the public list, since the worst-case impact for
> non-root users is leaking kernel pointers to userspace. In a context where
> you can reach BPF (no sandboxing), I don't think that kernel ASLR is very
> effective at the moment anyway.
IIUC, this is to fix leaking kernel pointers? If this is accurate, we should
include this information in the commit log.
>
> This breaks ten unit tests that assume that XADD is possible on the stack,
> and I'm not sure how all of them should be fixed up; I'd appreciate it if
> someone else could figure out how to fix them. I think some of them might
> be using XADD to cast pointers to numbers, or something like that? But I'm
> not sure.
Could you please list which tests are broken by this? We need to be careful
because some tools probably depend on this.
Thanks,
Song
next prev parent reply other threads:[~2020-04-16 20:52 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-15 20:47 [PATCH bpf] bpf: Use pointer type whitelist for XADD Jann Horn
2020-04-16 20:52 ` Song Liu [this message]
2020-04-16 21:11 ` Alexei Starovoitov
2020-04-16 22:34 ` Jann Horn
2020-04-17 0:41 ` Alexei Starovoitov
2020-04-17 1:39 ` Jann Horn
2020-04-17 2:07 ` Alexei Starovoitov
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=9229787E-6243-4F22-B809-819A3F24B5DD@fb.com \
--to=songliubraving@fb.com \
--cc=andriin@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jannh@google.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--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).