* Re: [PATCH 4.9] bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN
2017-12-23 2:26 [PATCH 4.9] bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN Ben Hutchings
@ 2017-12-23 3:51 ` Alexei Starovoitov
2017-12-23 4:31 ` kasan for bpf Alexei Starovoitov
2017-12-27 20:04 ` Patch "bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN" has been added to the 4.9-stable tree gregkh
2 siblings, 0 replies; 5+ messages in thread
From: Alexei Starovoitov @ 2017-12-23 3:51 UTC (permalink / raw)
To: Ben Hutchings
Cc: Greg Kroah-Hartman, stable, netdev, Edward Cree, Jann Horn,
Alexei Starovoitov
On Sat, Dec 23, 2017 at 02:26:17AM +0000, Ben Hutchings wrote:
> An UNKNOWN_VALUE is not supposed to be derived from a pointer, unless
> pointer leaks are allowed. Therefore, states_equal() must not treat
> a state with a pointer in a register as "equal" to a state with an
> UNKNOWN_VALUE in that register.
>
> This was fixed differently upstream, but the code around here was
> largely rewritten in 4.14 by commit f1174f77b50c "bpf/verifier: rework
> value tracking". The bug can be detected by the bpf/verifier sub-test
> "pointer/scalar confusion in state equality check (way 1)".
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: Edward Cree <ecree@solarflare.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* kasan for bpf
2017-12-23 2:26 [PATCH 4.9] bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN Ben Hutchings
2017-12-23 3:51 ` Alexei Starovoitov
@ 2017-12-23 4:31 ` Alexei Starovoitov
2017-12-23 16:03 ` David Miller
2017-12-27 20:04 ` Patch "bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN" has been added to the 4.9-stable tree gregkh
2 siblings, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2017-12-23 4:31 UTC (permalink / raw)
To: Jann Horn
Cc: netdev, Daniel Borkmann, Ben Hutchings, David S. Miller,
linux-kernel, kernel-team
Hi All,
the recent bugs make people question the safety of bpf and not a surprise
that some folks propose to set kernel.unprivileged_bpf_disabled = 1 by default.
I think it will be wrong long term decision, since it will steer
bpf into "root only" mentality. The verifier checks will become sloppier
and developers will worry less about missing safety checks.
I'd like bpf to go the other way. The root-only programs should be
treated with the same respect as unprivileged.
Today out of 15 program types only one BPF_PROG_TYPE_SOCKET_FILTER is
allowed for unpriv while in many cases like XDP, CLS, ACT, SKB, SOCK progs
could be relaxed to cap_net_admin and sometimes to unpriv.
I think we need to disallow leaking kernel address in XDP, CLS and
all other networking programs. They should be operating on packets
and should never leak addresses. The existing approach:
env->allow_ptr_leaks = capable(CAP_SYS_ADMIN);
if (env->allow_ptr_leaks) ...;
if (env->allow_ptr_leaks) ...;
is just a big hammer.
Realistically only tracing programs need to deal with kernel data and
they probably should be restricted further with additional sysctl or CAPs.
So instead of existing two classes of root/unpriv we need several:
- unpriv prog type
- all prog types for unpriv but execute via bpf_prog_run only (for testing)
- net_admin without leaks (that can run in containers)
- root without leaks
- root with leaks for tracing only
As far as unpriv I think it's clear that the verifier only
is not enough and we need to optionally add run-time checks.
I think 'kasan for bpf' by default would be good first step.
The basic idea is that all accessible memory (stack, ctx, maps)
will have shadow bits and every load/store will be automatically
instrumented with shadow memory checks.
'kasan for bpf' will be on by default for unpriv and extra
sysctl to turn it off in setups that cannot tolerate
slightly degraded performance.
For root and other classes of programs we will be able to
turn it on for testing as well.
I think that will stop many exploits except side channel attacks.
Thoughts?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: kasan for bpf
2017-12-23 4:31 ` kasan for bpf Alexei Starovoitov
@ 2017-12-23 16:03 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-12-23 16:03 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: jannh, netdev, daniel, ben, linux-kernel, kernel-team
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 22 Dec 2017 20:31:56 -0800
> Thoughts?
Even though you propose it as the opposite, it sounds like a crutch
for the verifier.
If we strictly control objects that the eBPF program can access,
verifier ensures this, and all other objects go through helpers,
then I cannot see what kasan for bpf can buy us.
To me it tells the world "yes, verifier and carefully designed helpers
are insufficient" and that's not the message I have been giving to
rooms full of hundreds of people listening to my xdp/bpf
presentations.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Patch "bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN" has been added to the 4.9-stable tree
2017-12-23 2:26 [PATCH 4.9] bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN Ben Hutchings
2017-12-23 3:51 ` Alexei Starovoitov
2017-12-23 4:31 ` kasan for bpf Alexei Starovoitov
@ 2017-12-27 20:04 ` gregkh
2 siblings, 0 replies; 5+ messages in thread
From: gregkh @ 2017-12-27 20:04 UTC (permalink / raw)
To: ben, ast, daniel, ecree, gregkh, jannh; +Cc: stable, stable-commits
This is a note to let you know that I've just added the patch titled
bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
bpf-verifier-fix-states_equal-comparison-of-pointer-and-unknown.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From ben@decadent.org.uk Wed Dec 27 21:04:06 2017
From: Ben Hutchings <ben@decadent.org.uk>
Date: Sat, 23 Dec 2017 02:26:17 +0000
Subject: bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org, netdev@vger.kernel.org, Edward Cree <ecree@solarflare.com>, Jann Horn <jannh@google.com>, Alexei Starovoitov <ast@kernel.org>
Message-ID: <20171223022617.GO2971@decadent.org.uk>
Content-Disposition: inline
From: Ben Hutchings <ben@decadent.org.uk>
An UNKNOWN_VALUE is not supposed to be derived from a pointer, unless
pointer leaks are allowed. Therefore, states_equal() must not treat
a state with a pointer in a register as "equal" to a state with an
UNKNOWN_VALUE in that register.
This was fixed differently upstream, but the code around here was
largely rewritten in 4.14 by commit f1174f77b50c "bpf/verifier: rework
value tracking". The bug can be detected by the bpf/verifier sub-test
"pointer/scalar confusion in state equality check (way 1)".
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Edward Cree <ecree@solarflare.com>
Cc: Jann Horn <jannh@google.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2722,11 +2722,12 @@ static bool states_equal(struct bpf_veri
/* If we didn't map access then again we don't care about the
* mismatched range values and it's ok if our old type was
- * UNKNOWN and we didn't go to a NOT_INIT'ed reg.
+ * UNKNOWN and we didn't go to a NOT_INIT'ed or pointer reg.
*/
if (rold->type == NOT_INIT ||
(!varlen_map_access && rold->type == UNKNOWN_VALUE &&
- rcur->type != NOT_INIT))
+ rcur->type != NOT_INIT &&
+ !__is_pointer_value(env->allow_ptr_leaks, rcur)))
continue;
/* Don't care about the reg->id in this case. */
Patches currently in stable-queue which might be from ben@decadent.org.uk are
queue-4.9/bpf-verifier-fix-states_equal-comparison-of-pointer-and-unknown.patch
^ permalink raw reply [flat|nested] 5+ messages in thread