[PATCH net] net: filter: initialize A and X registers

[PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

exisiting BPF verifier allows uninitialized access to registers,
'ret A' is considered to be a valid filter.
So initialize A and X to zero to prevent leaking kernel memory
In the future BPF verifier will be rejecting such filters

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
Cc: Daniel Borkmann <dborkman@redhat.com>
---
 net/core/filter.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index cd58614..9d79ca0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -122,6 +122,13 @@ noinline u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
 	return 0;
 }
 
+/* Register mappings for user programs. */
+#define A_REG		0
+#define X_REG		7
+#define TMP_REG		8
+#define ARG2_REG	2
+#define ARG3_REG	3
+
 /**
  *	__sk_run_filter - run a filter on a given context
  *	@ctx: buffer to run the filter on
@@ -242,6 +249,8 @@ unsigned int __sk_run_filter(void *ctx, const struct sock_filter_int *insn)
 
 	regs[FP_REG]  = (u64) (unsigned long) &stack[ARRAY_SIZE(stack)];
 	regs[ARG1_REG] = (u64) (unsigned long) ctx;
+	regs[A_REG] = 0;
+	regs[X_REG] = 0;
 
 select_insn:
 	goto *jumptable[insn->code];
@@ -643,13 +652,6 @@ static u64 __get_raw_cpu_id(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
 	return raw_smp_processor_id();
 }
 
-/* Register mappings for user programs. */
-#define A_REG		0
-#define X_REG		7
-#define TMP_REG		8
-#define ARG2_REG	2
-#define ARG3_REG	3
-
 static bool convert_bpf_extensions(struct sock_filter *fp,
 				   struct sock_filter_int **insnp)
 {
-- 
1.7.9.5

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 22 Apr 2014 20:18:57 -0700

> exisiting BPF verifier allows uninitialized access to registers,
> 'ret A' is considered to be a valid filter.
> So initialize A and X to zero to prevent leaking kernel memory
> In the future BPF verifier will be rejecting such filters
> 
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

Has the code always been like this?

Did the eBPF changes introduce this problem either directly or
indirectly?

Re: [PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Tue, 22 Apr 2014 20:18:57 -0700
>
>> exisiting BPF verifier allows uninitialized access to registers,
>> 'ret A' is considered to be a valid filter.
>> So initialize A and X to zero to prevent leaking kernel memory
>> In the future BPF verifier will be rejecting such filters
>>
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>
> Has the code always been like this?

the previous interpreter had:
unsigned int sk_run_filter(const struct sk_buff *skb,
                           const struct sock_filter *fentry)
{
        void *ptr;
        u32 A = 0;                      /* Accumulator */
        u32 X = 0;                      /* Index Register */
...
so it wasn't affected.

> Did the eBPF changes introduce this problem either directly or
> indirectly?

this bug is an oversight on my side.
I believe in the net-next it should be fixed by making verifier smarter
instead of wasting run time cycles to initialize regs.
For now the same fix is needed for both net and net-next.
We can remove extra assignments when verifier becomes smarter.
ebpf verifier that I posted earlier had checks for uninitialized regs and stack.
I missed lack of uninit regs part in classic verifier when ebpf verifier and jit
were dropped from the patch set.
Sorry about this slip.

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/23/2014 06:59 AM, Alexei Starovoitov wrote:
> On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@davemloft.net> wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>
>>> exisiting BPF verifier allows uninitialized access to registers,
>>> 'ret A' is considered to be a valid filter.
>>> So initialize A and X to zero to prevent leaking kernel memory
>>> In the future BPF verifier will be rejecting such filters
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> Has the code always been like this?

I think it would be much cleaner to just prevent such filters that
only contain a 'ret A', or 'ret X' w/o a load into X from attaching.
By that, we would need to do slightly more work on the verifier
side (which seemed to allow such cases since ever), but therefore
can spare us the extra work in fast path.

Only 'ret A' doesn't make sense, and similarly a program that never
loads anything into X, but wants to return it. We already did a
similar thing for the scratch memory store where we checked stores
into M[] and loads from M[] into A. I can take a look if you want ...
I have some pending BPF cleanups anyway from before the merge window.

> the previous interpreter had:
> unsigned int sk_run_filter(const struct sk_buff *skb,
>                             const struct sock_filter *fentry)
> {
>          void *ptr;
>          u32 A = 0;                      /* Accumulator */
>          u32 X = 0;                      /* Index Register */
> ...
> so it wasn't affected.
>
>> Did the eBPF changes introduce this problem either directly or
>> indirectly?
>
> this bug is an oversight on my side.
> I believe in the net-next it should be fixed by making verifier smarter
> instead of wasting run time cycles to initialize regs.
> For now the same fix is needed for both net and net-next.
> We can remove extra assignments when verifier becomes smarter.
> ebpf verifier that I posted earlier had checks for uninitialized regs and stack.
> I missed lack of uninit regs part in classic verifier when ebpf verifier and jit
> were dropped from the patch set.
> Sorry about this slip.

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 23 Apr 2014 09:02:54 +0200

> On 04/23/2014 06:59 AM, Alexei Starovoitov wrote:
>> On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@davemloft.net>
>> wrote:
>>> From: Alexei Starovoitov <ast@plumgrid.com>
>>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>>
>>>> exisiting BPF verifier allows uninitialized access to registers,
>>>> 'ret A' is considered to be a valid filter.
>>>> So initialize A and X to zero to prevent leaking kernel memory
>>>> In the future BPF verifier will be rejecting such filters
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>
>>> Has the code always been like this?
> 
> I think it would be much cleaner to just prevent such filters that
> only contain a 'ret A', or 'ret X' w/o a load into X from attaching.

That choice had to be done two decades ago, not now.

We have to accept such filters, we always have.

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/23/2014 06:52 PM, David Miller wrote:
...
>> I think it would be much cleaner to just prevent such filters that
>> only contain a 'ret A', or 'ret X' w/o a load into X from attaching.
>
> That choice had to be done two decades ago, not now.
>
> We have to accept such filters, we always have.

Yep, very true. :/

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 22 Apr 2014 21:59:32 -0700

> On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@davemloft.net> wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>
>>> exisiting BPF verifier allows uninitialized access to registers,
>>> 'ret A' is considered to be a valid filter.
>>> So initialize A and X to zero to prevent leaking kernel memory
>>> In the future BPF verifier will be rejecting such filters
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> Has the code always been like this?
> 
> the previous interpreter had:
> unsigned int sk_run_filter(const struct sk_buff *skb,
>                            const struct sock_filter *fentry)
> {
>         void *ptr;
>         u32 A = 0;                      /* Accumulator */
>         u32 X = 0;                      /* Index Register */
> ...
> so it wasn't affected.
> 
>> Did the eBPF changes introduce this problem either directly or
>> indirectly?
> 
> this bug is an oversight on my side.
> I believe in the net-next it should be fixed by making verifier smarter
> instead of wasting run time cycles to initialize regs.
> For now the same fix is needed for both net and net-next.
> We can remove extra assignments when verifier becomes smarter.
> ebpf verifier that I posted earlier had checks for uninitialized regs and stack.
> I missed lack of uninit regs part in classic verifier when ebpf verifier and jit
> were dropped from the patch set.

I think you will need to add the initializations during validation
rather than fail because existing BPF filters would be accepted and
run.  You can't just make them fail unexpectedly after all of these
years.

Re: [PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

On Wed, Apr 23, 2014 at 9:50 AM, David Miller <davem@davemloft.net> wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Tue, 22 Apr 2014 21:59:32 -0700
>
>> On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Alexei Starovoitov <ast@plumgrid.com>
>>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>>
>>>> exisiting BPF verifier allows uninitialized access to registers,
>>>> 'ret A' is considered to be a valid filter.
>>>> So initialize A and X to zero to prevent leaking kernel memory
>>>> In the future BPF verifier will be rejecting such filters
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>
>>> Has the code always been like this?
>>
>> the previous interpreter had:
>> unsigned int sk_run_filter(const struct sk_buff *skb,
>>                            const struct sock_filter *fentry)
>> {
>>         void *ptr;
>>         u32 A = 0;                      /* Accumulator */
>>         u32 X = 0;                      /* Index Register */
>> ...
>> so it wasn't affected.
>>
>>> Did the eBPF changes introduce this problem either directly or
>>> indirectly?
>>
>> this bug is an oversight on my side.
>> I believe in the net-next it should be fixed by making verifier smarter
>> instead of wasting run time cycles to initialize regs.
>> For now the same fix is needed for both net and net-next.
>> We can remove extra assignments when verifier becomes smarter.
>> ebpf verifier that I posted earlier had checks for uninitialized regs and stack.
>> I missed lack of uninit regs part in classic verifier when ebpf verifier and jit
>> were dropped from the patch set.
>
> I think you will need to add the initializations during validation
> rather than fail because existing BPF filters would be accepted and
> run.  You can't just make them fail unexpectedly after all of these
> years.

In this particular case we can indeed make verifier smarter to the point
that once it detects uninitialized A/X in a filter, it will add explicit init
after conversion, so we don't have to pay the penalty at run time.
It's a very good suggestion.

At the same time I think 'ret A' falls into category of the 20 year old bug.
The filters that using undocumented behavior are destined to be
broken sooner or later. tcpdump doesn't generate such filters.
Valid filters will not be affected by strict checking, but I think users
will be actually happy to see kernel rejecting their buggy filters.
I think we can have a global flag for bpf verifier whether to reject
such filters or accept them by adding explicit inits under
'bpf_permissive_loading' flag.
Thoughts?

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Wed, 2014-04-23 at 13:38 -0700, Alexei Starovoitov wrote:

> In this particular case we can indeed make verifier smarter to the point
> that once it detects uninitialized A/X in a filter, it will add explicit init
> after conversion, so we don't have to pay the penalty at run time.
> It's a very good suggestion.
> 
> At the same time I think 'ret A' falls into category of the 20 year old bug.
> The filters that using undocumented behavior are destined to be
> broken sooner or later. tcpdump doesn't generate such filters.
> Valid filters will not be affected by strict checking, but I think users
> will be actually happy to see kernel rejecting their buggy filters.
> I think we can have a global flag for bpf verifier whether to reject
> such filters or accept them by adding explicit inits under
> 'bpf_permissive_loading' flag.
> Thoughts?


I do not really understand the concerns and this discussion over this
bug.

BPF JIT does basic checks and forces A=0 if first instruction doesn't
set A.

( arch/x86/net/bpf_jit_comp.c line 263 )

X is forced to 0 anyway if X is ever used in the JITed filter.
( line 227, and we have a typo in comment : s/leek/leak/ )

This is not a big deal. If you want to 'optimize', fine, but don't break
programs.

There is no way you add a 'flag' to break a user program, even
if _you_ believe its broken.

Maybe it assumed initial X / A contents were 0.

Even if not documented, we cannot break this 20 years after the facts.

Re: [PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

On Wed, Apr 23, 2014 at 2:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2014-04-23 at 13:38 -0700, Alexei Starovoitov wrote:
>
>> In this particular case we can indeed make verifier smarter to the point
>> that once it detects uninitialized A/X in a filter, it will add explicit init
>> after conversion, so we don't have to pay the penalty at run time.
>> It's a very good suggestion.
>>
>> At the same time I think 'ret A' falls into category of the 20 year old bug.
>> The filters that using undocumented behavior are destined to be
>> broken sooner or later. tcpdump doesn't generate such filters.
>> Valid filters will not be affected by strict checking, but I think users
>> will be actually happy to see kernel rejecting their buggy filters.
>> I think we can have a global flag for bpf verifier whether to reject
>> such filters or accept them by adding explicit inits under
>> 'bpf_permissive_loading' flag.
>> Thoughts?
>
>
> I do not really understand the concerns and this discussion over this
> bug.
>
> BPF JIT does basic checks and forces A=0 if first instruction doesn't
> set A.
>
> ( arch/x86/net/bpf_jit_comp.c line 263 )
>
> X is forced to 0 anyway if X is ever used in the JITed filter.
> ( line 227, and we have a typo in comment : s/leek/leak/ )

yes, x86 jit is smart to deal around this problem in an efficient way.
sparc/arm jits copy pasted the idea correctly.
but s390 jit seems to have a bug, since it's not clearing A
when 1st insn is BPF_S_LDX_B_MSH
That shouldn't be a jit job, especially since it can be done by verifier.
classic bpf has 2 regs, ebpf has 10. Checking 1st insn won't work
for ebpf.

> This is not a big deal. If you want to 'optimize', fine, but don't break
> programs.

Fine. I'm not saying to break them by default. I'm suggesting
to give users a flag to detect invalid programs.

> There is no way you add a 'flag' to break a user program, even
> if _you_ believe its broken.
>
> Maybe it assumed initial X / A contents were 0.
>
> Even if not documented, we cannot break this 20 years after the facts.

I'm not suggesting to have this flag by default, but don't you want
kernel to say that bpf program is using uninitialized register?
imo the more checking can be done the better user experience will be.
Just like compiler warnings. Better to have an option to know
what's wrong with the filter than not having it at all.

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Wed, 2014-04-23 at 15:19 -0700, Alexei Starovoitov wrote:

> yes, x86 jit is smart to deal around this problem in an efficient way.
> sparc/arm jits copy pasted the idea correctly.

Well... not really smart, but sufficient enough (even if I am sure we
probably have some bugs)

> but s390 jit seems to have a bug, since it's not clearing A
> when 1st insn is BPF_S_LDX_B_MSH

Sure, JIT is hard and quite risky (You forgot to CC Heiko & Martin, they
probably can send the needed patch)

Its not yet enabled by default for this reason.

I am worried that all recent bpf changes raised the bar so high that
nobody but you can really understand what is going on, unless spending
a _lot_ of time on it.

net/core/filter.c was about 880 lines in previous release, its now 1800+
lines.

netdev became a list discussing compiler technology, 
assembly code and ABI level stuff lately.

Say I want to check why we have this code, potentially modifying
arbitrary kernel memory :

        BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
                atomic_add((u32) X, (atomic_t *)(unsigned long)
                           (A + insn->off));
                CONT;
        BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
                atomic64_add((u64) X, (atomic64_t *)(unsigned long)
                             (A + insn->off));
                CONT;



No comments on this scary code, and I couldn't find why it is there.

$ git grep -n2 BPF_XADD
include/linux/filter.h-19-/* ld/ldx fields */
include/linux/filter.h-20-#define BPF_DW                0x18    /* double word */
include/linux/filter.h:21:#define BPF_XADD      0xc0    /* exclusive add */
include/linux/filter.h-22-
include/linux/filter.h-23-/* alu/jmp fields */
--
net/core/filter.c-222-          DL(BPF_STX, BPF_MEM, BPF_W),
net/core/filter.c-223-          DL(BPF_STX, BPF_MEM, BPF_DW),
net/core/filter.c:224:          DL(BPF_STX, BPF_XADD, BPF_W),
net/core/filter.c:225:          DL(BPF_STX, BPF_XADD, BPF_DW),
net/core/filter.c-226-          DL(BPF_ST, BPF_MEM, BPF_B),
net/core/filter.c-227-          DL(BPF_ST, BPF_MEM, BPF_H),
--
net/core/filter.c-479-  LDST(BPF_DW, u64)
net/core/filter.c-480-#undef LDST
net/core/filter.c:481:  BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
net/core/filter.c-482-          atomic_add((u32) X, (atomic_t *)(unsigned long)
net/core/filter.c-483-                     (A + insn->off));
net/core/filter.c-484-          CONT;
net/core/filter.c:485:  BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
net/core/filter.c-486-          atomic64_add((u64) X, (atomic64_t *)(unsigned long)
net/core/filter.c-487-                       (A + insn->off));

This looks like premature inclusion, at very minimum.

Sure, I probably can find answer reading past months netdev/lkml traffic ?

Sorry if this looks like ranting, but I would rather complain before thousand
of other lines of code are merged.

Re: [PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

On Wed, Apr 23, 2014 at 7:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2014-04-23 at 15:19 -0700, Alexei Starovoitov wrote:
>
>> yes, x86 jit is smart to deal around this problem in an efficient way.
>> sparc/arm jits copy pasted the idea correctly.
>
> Well... not really smart, but sufficient enough (even if I am sure we
> probably have some bugs)
>
>> but s390 jit seems to have a bug, since it's not clearing A
>> when 1st insn is BPF_S_LDX_B_MSH
>
> Sure, JIT is hard and quite risky (You forgot to CC Heiko & Martin, they
> probably can send the needed patch)
>
> Its not yet enabled by default for this reason.
>
> I am worried that all recent bpf changes raised the bar so high that
> nobody but you can really understand what is going on, unless spending
> a _lot_ of time on it.

I'm trying to explain it as much as I can. If you're willing to listen,
I'm happy to answer any questions.

> net/core/filter.c was about 880 lines in previous release, its now 1800+
> lines.

but the whole sk_encode/decode functions are obsolete.
Now we can remove some code too.

> netdev became a list discussing compiler technology,
> assembly code and ABI level stuff lately.
>
> Say I want to check why we have this code, potentially modifying
> arbitrary kernel memory :
>
>         BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
>                 atomic_add((u32) X, (atomic_t *)(unsigned long)
>                            (A + insn->off));
>                 CONT;
>         BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
>                 atomic64_add((u64) X, (atomic64_t *)(unsigned long)
>                              (A + insn->off));
>                 CONT;
>
>
>
> No comments on this scary code, and I couldn't find why it is there.

I'm sorry but this very list forced me to drop all the comments and docs
around ebpf verifier that go into details how this is verified and why
it is safe.
I can repost the whole thing as one big series of patches
or send the links to previous patches where approach to safety is explained.
I thought the preference was to phase it in gradually.
Smallest patch at a time. Don't break anything. That's what I'm trying to do.

> $ git grep -n2 BPF_XADD
> include/linux/filter.h-19-/* ld/ldx fields */
> include/linux/filter.h-20-#define BPF_DW                0x18    /* double word */
> include/linux/filter.h:21:#define BPF_XADD      0xc0    /* exclusive add */
> include/linux/filter.h-22-
> include/linux/filter.h-23-/* alu/jmp fields */
> --
> net/core/filter.c-222-          DL(BPF_STX, BPF_MEM, BPF_W),
> net/core/filter.c-223-          DL(BPF_STX, BPF_MEM, BPF_DW),
> net/core/filter.c:224:          DL(BPF_STX, BPF_XADD, BPF_W),
> net/core/filter.c:225:          DL(BPF_STX, BPF_XADD, BPF_DW),
> net/core/filter.c-226-          DL(BPF_ST, BPF_MEM, BPF_B),
> net/core/filter.c-227-          DL(BPF_ST, BPF_MEM, BPF_H),
> --
> net/core/filter.c-479-  LDST(BPF_DW, u64)
> net/core/filter.c-480-#undef LDST
> net/core/filter.c:481:  BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
> net/core/filter.c-482-          atomic_add((u32) X, (atomic_t *)(unsigned long)
> net/core/filter.c-483-                     (A + insn->off));
> net/core/filter.c-484-          CONT;
> net/core/filter.c:485:  BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
> net/core/filter.c-486-          atomic64_add((u64) X, (atomic64_t *)(unsigned long)
> net/core/filter.c-487-                       (A + insn->off));
>
> This looks like premature inclusion, at very minimum.
>
> Sure, I probably can find answer reading past months netdev/lkml traffic ?

yes. it's more than one month. It was posted first back in September.
ebpf architecture didn't change since then.
Just new use cases were found.
I thought to apply ebpf to ovs initially, but now it looks like tracing filters
will benefit it more, so they took a priority. seccomp, nids, etc

> Sorry if this looks like ranting, but I would rather complain before thousand
> of other lines of code are merged.

It's a valid concern. I need to share and explain more. Point taken.

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/24/2014 05:22 AM, Alexei Starovoitov wrote:
...
>> net/core/filter.c was about 880 lines in previous release, its now 1800+
>> lines.
>
> but the whole sk_encode/decode functions are obsolete.
> Now we can remove some code too.

Indeed, these can eventually be removed; naturally, a bigger portion
of these changes go into the conversion functions into the new layout.
The interpreter part itself is conceptually the same, seccomp as you
might have noticed actually got a lot easier thanks to the BPF
changes, and the new instruction layout is much easier to handle for
JITs (i.e. the BPF extensions), which is ongoing work. Clearly, there
are successive integration steps required. As such, also the
documentation will get further improved for all that in the next days.

Thanks,

Daniel

Re: [PATCH net] net: filter: initialize A and X registers

[Martin Schwidefsky]

On Wed, 23 Apr 2014 19:55:09 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2014-04-23 at 15:19 -0700, Alexei Starovoitov wrote:
> 
> > yes, x86 jit is smart to deal around this problem in an efficient way.
> > sparc/arm jits copy pasted the idea correctly.
> 
> Well... not really smart, but sufficient enough (even if I am sure we
> probably have some bugs)
> 
> > but s390 jit seems to have a bug, since it's not clearing A
> > when 1st insn is BPF_S_LDX_B_MSH
> 
> Sure, JIT is hard and quite risky (You forgot to CC Heiko & Martin, they
> probably can send the needed patch)

That should be a one-line patch which removes the BPF_S_LDX_B_MSH opcode
from the switch statement in bpf_jit_noleaks.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Tue, 2014-04-22 at 23:57 -0400, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Tue, 22 Apr 2014 20:18:57 -0700
> 
> > exisiting BPF verifier allows uninitialized access to registers,
> > 'ret A' is considered to be a valid filter.
> > So initialize A and X to zero to prevent leaking kernel memory
> > In the future BPF verifier will be rejecting such filters
> > 
> > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> 
> Has the code always been like this?
> 
> Did the eBPF changes introduce this problem either directly or
> indirectly?

Original code was fine AFAIK

Fixes: bd4cf0ed331a2 ("net: filter: rework/optimize internal BPF interpreter's instruction set")

David, is it possible for you to push net-next tree ?

Thank !

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/23/2014 07:13 AM, Eric Dumazet wrote:
> On Tue, 2014-04-22 at 23:57 -0400, David Miller wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>
>>> exisiting BPF verifier allows uninitialized access to registers,
>>> 'ret A' is considered to be a valid filter.
>>> So initialize A and X to zero to prevent leaking kernel memory
>>> In the future BPF verifier will be rejecting such filters
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>
>> Has the code always been like this?
>>
>> Did the eBPF changes introduce this problem either directly or
>> indirectly?
>
> Original code was fine AFAIK

Yep.

> Fixes: bd4cf0ed331a2 ("net: filter: rework/optimize internal BPF interpreter's instruction set")
>
> David, is it possible for you to push net-next tree ?

I think this would need to go into net tree first and then
for the upcoming changes, net would need to be merged into
net-next. This will create a minor merge conflict with the
BPF prandom extension; fix is to remove the defines near
__get_random_u32(), as they will be above __sk_run_filter()
already.

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Wed, 2014-04-23 at 13:45 +0200, Daniel Borkmann wrote:
> On 04/23/2014 07:13 AM, Eric Dumazet wrote:
> >
> > David, is it possible for you to push net-next tree ?
> 
> I think this would need to go into net tree first and then
> for the upcoming changes, net would need to be merged into
> net-next. This will create a minor merge conflict with the
> BPF prandom extension; fix is to remove the defines near
> __get_random_u32(), as they will be above __sk_run_filter()
> already.

My suggestion to push net-next was not related to the bpf patch.

I needed net-next being fresh for a backport into Google kernels.

Thanks

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Apr 2014 06:39:04 -0700

> I needed net-next being fresh for a backport into Google kernels.

I try to have this merge taken care of by the end of today.

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 22 Apr 2014 22:13:00 -0700

> On Tue, 2014-04-22 at 23:57 -0400, David Miller wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>> 
>> > exisiting BPF verifier allows uninitialized access to registers,
>> > 'ret A' is considered to be a valid filter.
>> > So initialize A and X to zero to prevent leaking kernel memory
>> > In the future BPF verifier will be rejecting such filters
>> > 
>> > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>> 
>> Has the code always been like this?
>> 
>> Did the eBPF changes introduce this problem either directly or
>> indirectly?
> 
> Original code was fine AFAIK
> 
> Fixes: bd4cf0ed331a2 ("net: filter: rework/optimize internal BPF interpreter's instruction set")
> 
> David, is it possible for you to push net-next tree ?

What exactly are you asking me to do?  Put this patch in the net-next tree?
Or are you asking me to merge net into net-next after I apply it?

It's definitely a 'net' patch.

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/23/2014 06:51 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 22 Apr 2014 22:13:00 -0700
>
>> On Tue, 2014-04-22 at 23:57 -0400, David Miller wrote:
>>> From: Alexei Starovoitov <ast@plumgrid.com>
>>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>>
>>>> exisiting BPF verifier allows uninitialized access to registers,
>>>> 'ret A' is considered to be a valid filter.
>>>> So initialize A and X to zero to prevent leaking kernel memory
>>>> In the future BPF verifier will be rejecting such filters
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>>>
>>> Has the code always been like this?
>>>
>>> Did the eBPF changes introduce this problem either directly or
>>> indirectly?
>>
>> Original code was fine AFAIK
>>
>> Fixes: bd4cf0ed331a2 ("net: filter: rework/optimize internal BPF interpreter's instruction set")
>>
>> David, is it possible for you to push net-next tree ?
>
> What exactly are you asking me to do?  Put this patch in the net-next tree?
> Or are you asking me to merge net into net-next after I apply it?
>
> It's definitely a 'net' patch.

I think Eric already clarified it in [1].

It's definitely against net tree.

 From my side, it would be awesome, if you could put this into net and
then merge net into net-next as I have some pending stuff on top of
this fix. That would at least avoid a bigger merge conflict later on.

Thanks a lot, Dave.

  [1] http://patchwork.ozlabs.org/patch/341693/, April 23, 2014, 1:39 p.m.

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Wed, 2014-04-23 at 12:51 -0400, David Miller wrote:

> > David, is it possible for you to push net-next tree ?
> 
> What exactly are you asking me to do?  Put this patch in the net-next tree?
> Or are you asking me to merge net into net-next after I apply it?
> 
> It's definitely a 'net' patch.

This net-next push request had nothing to do with this bpf patch, sorry
for the confusion.

I even missed a s to the 'Thanks', you see ;)

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Apr 2014 10:14:44 -0700

> This net-next push request had nothing to do with this bpf patch, sorry
> for the confusion.

I just did the merge, sorry for taking so long.

Re: [PATCH net] net: filter: initialize A and X registers

[Eric Dumazet]

On Thu, 2014-04-24 at 13:24 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 23 Apr 2014 10:14:44 -0700
> 
> > This net-next push request had nothing to do with this bpf patch, sorry
> > for the confusion.
> 
> I just did the merge, sorry for taking so long.

No problem, thanks David !

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/24/2014 07:24 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 23 Apr 2014 10:14:44 -0700
>
>> This net-next push request had nothing to do with this bpf patch, sorry
>> for the confusion.
>
> I just did the merge, sorry for taking so long.

Great, thanks Dave!

Re: [PATCH net] net: filter: initialize A and X registers

[Daniel Borkmann]

On 04/23/2014 05:18 AM, Alexei Starovoitov wrote:
> exisiting BPF verifier allows uninitialized access to registers,
> 'ret A' is considered to be a valid filter.
> So initialize A and X to zero to prevent leaking kernel memory
> In the future BPF verifier will be rejecting such filters
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Daniel Borkmann <dborkman@redhat.com>

I gave it some more thought, it's actually more than just 'ret A'
from your description, user programs could be used to generate code
like 'add X' or 'add #42' instead of a txa or load instruction [where
A is previously uninitialized], as it was always initialized to 0
before. We prevent to do a 'ret X', but X could be uninitialized and
then transferred over to taint A etc. So yeah, this patch is the most
simple way to prevent that w/o huge complexity. Therefore, it's
better this way:

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH net] net: filter: initialize A and X registers

[Alexei Starovoitov]

On Wed, Apr 23, 2014 at 12:53 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 04/23/2014 05:18 AM, Alexei Starovoitov wrote:
>>
>> exisiting BPF verifier allows uninitialized access to registers,
>> 'ret A' is considered to be a valid filter.
>> So initialize A and X to zero to prevent leaking kernel memory
>> In the future BPF verifier will be rejecting such filters
>>
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Daniel Borkmann <dborkman@redhat.com>
>
>
> I gave it some more thought, it's actually more than just 'ret A'
> from your description, user programs could be used to generate code
> like 'add X' or 'add #42' instead of a txa or load instruction [where
> A is previously uninitialized], as it was always initialized to 0
> before. We prevent to do a 'ret X', but X could be uninitialized and
> then transferred over to taint A etc. So yeah, this patch is the most
> simple way to prevent that w/o huge complexity. Therefore, it's
> better this way:
>
> Acked-by: Daniel Borkmann <dborkman@redhat.com>

Thanks.

the uninitialized checking is indeed not as simple as rejecting 'ret A'.
verifier needs to consider data flow between registers based
on control graph. Just linear scan won't be correct.
Also for ebpf there are 10+1 registers to consider, so ebpf verifier
will take care of both (classic and extended) at once.
classic verifier can stay as-is and eventually removed.
For example after 'bpf_call' insn the registers r1-r5 are scratched,
so they cannot be read, r10 is frame pointer and is read only, etc.
will prepare ebpf verifier patch asap.
the steps will be:
- classic verifier (as-is today)
- bpf->ebpf converter (as-is today)
- ebpf verifier
after insane amount of testing we will be able to remove classic verifier,
since ebpf verifier will be doing it more thorough.

Re: [PATCH net] net: filter: initialize A and X registers

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 22 Apr 2014 20:18:57 -0700

> exisiting BPF verifier allows uninitialized access to registers,
> 'ret A' is considered to be a valid filter.
> So initialize A and X to zero to prevent leaking kernel memory
> In the future BPF verifier will be rejecting such filters
> 
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

Applied, thanks.