Re: [PATCH net] bpf: disallow arithmetic operations on context pointer

On Mon, 16 Oct 2017 17:16:24 +0100, Edward Cree wrote:

On 16/10/17 16:45, Jakub Kicinski wrote:

quoted

Commit f1174f77b50c ("bpf/verifier: rework value tracking")
removed the crafty selection of which pointer types are
allowed to be modified.  This is OK for most pointer types
since adjust_ptr_min_max_vals() will catch operations on
immutable pointers.  One exception is PTR_TO_CTX which is
now allowed to be offseted freely.

The intent of aforementioned commit was to allow context
access via modified registers.  The offset passed to  
->is_valid_access() verifier callback has been adjusted  
by the value of the variable offset.

What is missing, however, is taking the variable offset
into account when the context register is used.  Or in terms
of the code adding the offset to the value passed to the  
->convert_ctx_access() callback.  This leads to the following  

Good catch.
So the problem is just that convert_ctx_access() can't deal
 with it, yes?  Assuming that the offset is constant, because
 otherwise we'd reject it anyway, we _could_ stash that offset
 in insn_aux_data, and reject any paths that tried to change it
 subsequently; and then convert_ctx_access() could be given the
 total (off + reg->off) and then the reg->off could be subtracted
 from the result, giving the right 'converted' insn offset.
That would then mean that your example,

quoted

eBPF user code:

     r1 += 68
     r0 = *(u32 *)(r1 + 8)
     exit

being translated to this in kernel space:

   0: (07) r1 += 68
   1: (61) r0 = *(u32 *)(r1 +180)
   2: (95) exit  

would instead convert offset 76 to something else, let's say
 200 just for the sake of argument, and produce
   1: (61) r0 = *(u32 *)(r1 + 132)
 which would then do the right thing at run time.

However, I don't know whether anyone would actually want this
 to be supported for their programs, and so I'm happy to
 disallow this for net and then maybe we can follow up in
 net-next with the change I describe above if it's useful.

That was my thinking too.  We would have to stash the offset and make
sure that it doesn't change on different paths, but it's probably not
suitable for -net, and nobody uses that today (although Alexei
mentioned there may be valid use cases, especially in tracing).

quoted

Offset 8 is corresponding to 180 in the kernel, but offset
76 is valid too.  Verifier will "accept" access to offset
68+8=76 but then "convert" access to offset 8 as 180.
Effective access to offset 248 is beyond the kernel context.
(This is a __sk_buff example on a debug-heavy kernel -
packet mark is 8 -> 180, 76 would be data.)

Dereferencing the modified context pointer is not as easy
as dereferencing other types, because we have to translate
the access to reading a field in kernel structures which is
usually at a different offset and often of a different size.
To allow modifying the pointer we would have to make sure
that given eBPF instruction will always access the same
field or the fields accessed are "compatible" in terms of
offset and size...

Disallow dereferencing modified context pointers and add
to selftests the test case described here.

Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
Signed-off-by: Jakub Kicinski <redacted>
---
Dave, a merge note - in net-next this will need env to be passed 
to verbose().

 kernel/bpf/verifier.c                       |  8 ++++++--
 tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8b8d6ba39e23..8499759d0c7a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		/* ctx accesses must be at a fixed offset, so that we can
 		 * determine what type of data were returned.
 		 */
-		if (!tnum_is_const(reg->var_off)) {
+		if (reg->off) {
+			verbose("derefence of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",  

This is slightly unclear, it's not that two adds is bad (e.g. r1 += 8;
 r0 = *(u32 *)r1 is bad too), it's that the offset must be in the load,
 not the register; your message might be accurate for some compilers but
 not in full generality (especially for assemblers without compiling).

I'm happy to hear better suggestions :)  I've spent quite a bit of time
scratching my head thinking how to phrase this best.  The first
part of the message is general enough IMHO, the second is targeted
mostly at C developers.

Also, sp. "dereference".

Argh.  aspell doesn't know that word, added to local dict now.