* [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
@ 2016-01-05 15:23 Rabin Vincent
2016-01-05 16:00 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Rabin Vincent @ 2016-01-05 15:23 UTC (permalink / raw)
To: davem
Cc: netdev, daniel, ast, linux-arm-kernel, linux-mips, linuxppc-dev,
sparclinux, linux-kernel, Rabin Vincent
The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
instructions since it XORs A with X while all the others replace A with
some loaded value. All the BPF JITs fail to clear A if this is used as
the first instruction in a filter. This was found using american fuzzy
lop.
Add a helper to determine if A needs to be cleared given the first
instruction in a filter, and use this in the JITs. Except for ARM, the
rest have only been compile-tested.
Fixes: 3480593131e0 ("net: filter: get rid of BPF_S_* enum")
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
arch/arm/net/bpf_jit_32.c | 16 +---------------
arch/mips/net/bpf_jit.c | 16 +---------------
arch/powerpc/net/bpf_jit_comp.c | 13 ++-----------
arch/sparc/net/bpf_jit_comp.c | 17 ++---------------
include/linux/filter.h | 19 +++++++++++++++++++
5 files changed, 25 insertions(+), 56 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 591f9db3bf40..e153eb065fe4 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -187,19 +187,6 @@ static inline int mem_words_used(struct jit_ctx *ctx)
return fls(ctx->seen & SEEN_MEM);
}
-static inline bool is_load_to_a(u16 inst)
-{
- switch (inst) {
- case BPF_LD | BPF_W | BPF_LEN:
- case BPF_LD | BPF_W | BPF_ABS:
- case BPF_LD | BPF_H | BPF_ABS:
- case BPF_LD | BPF_B | BPF_ABS:
- return true;
- default:
- return false;
- }
-}
-
static void jit_fill_hole(void *area, unsigned int size)
{
u32 *ptr;
@@ -211,7 +198,6 @@ static void jit_fill_hole(void *area, unsigned int size)
static void build_prologue(struct jit_ctx *ctx)
{
u16 reg_set = saved_regs(ctx);
- u16 first_inst = ctx->skf->insns[0].code;
u16 off;
#ifdef CONFIG_FRAME_POINTER
@@ -241,7 +227,7 @@ static void build_prologue(struct jit_ctx *ctx)
emit(ARM_MOV_I(r_X, 0), ctx);
/* do not leak kernel data to userspace */
- if ((first_inst != (BPF_RET | BPF_K)) && !(is_load_to_a(first_inst)))
+ if (bpf_needs_clear_a(&ctx->skf->insns[0]))
emit(ARM_MOV_I(r_A, 0), ctx);
/* stack space for the BPF_MEM words */
diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index 77cb27309db2..1a8c96035716 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -521,19 +521,6 @@ static inline u16 align_sp(unsigned int num)
return num;
}
-static bool is_load_to_a(u16 inst)
-{
- switch (inst) {
- case BPF_LD | BPF_W | BPF_LEN:
- case BPF_LD | BPF_W | BPF_ABS:
- case BPF_LD | BPF_H | BPF_ABS:
- case BPF_LD | BPF_B | BPF_ABS:
- return true;
- default:
- return false;
- }
-}
-
static void save_bpf_jit_regs(struct jit_ctx *ctx, unsigned offset)
{
int i = 0, real_off = 0;
@@ -614,7 +601,6 @@ static unsigned int get_stack_depth(struct jit_ctx *ctx)
static void build_prologue(struct jit_ctx *ctx)
{
- u16 first_inst = ctx->skf->insns[0].code;
int sp_off;
/* Calculate the total offset for the stack pointer */
@@ -641,7 +627,7 @@ static void build_prologue(struct jit_ctx *ctx)
emit_jit_reg_move(r_X, r_zero, ctx);
/* Do not leak kernel data to userspace */
- if ((first_inst != (BPF_RET | BPF_K)) && !(is_load_to_a(first_inst)))
+ if (bpf_needs_clear_a(&ctx->skf->insns[0]))
emit_jit_reg_move(r_A, r_zero, ctx);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 04782164ee67..2d66a8446198 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -78,18 +78,9 @@ static void bpf_jit_build_prologue(struct bpf_prog *fp, u32 *image,
PPC_LI(r_X, 0);
}
- switch (filter[0].code) {
- case BPF_RET | BPF_K:
- case BPF_LD | BPF_W | BPF_LEN:
- case BPF_LD | BPF_W | BPF_ABS:
- case BPF_LD | BPF_H | BPF_ABS:
- case BPF_LD | BPF_B | BPF_ABS:
- /* first instruction sets A register (or is RET 'constant') */
- break;
- default:
- /* make sure we dont leak kernel information to user */
+ /* make sure we dont leak kernel information to user */
+ if (bpf_needs_clear_a(&filter[0]))
PPC_LI(r_A, 0);
- }
}
static void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 22564f5f2364..3e6e05a7c4c2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -420,22 +420,9 @@ void bpf_jit_compile(struct bpf_prog *fp)
}
emit_reg_move(O7, r_saved_O7);
- switch (filter[0].code) {
- case BPF_RET | BPF_K:
- case BPF_LD | BPF_W | BPF_LEN:
- case BPF_LD | BPF_W | BPF_ABS:
- case BPF_LD | BPF_H | BPF_ABS:
- case BPF_LD | BPF_B | BPF_ABS:
- /* The first instruction sets the A register (or is
- * a "RET 'constant'")
- */
- break;
- default:
- /* Make sure we dont leak kernel information to the
- * user.
- */
+ /* Make sure we dont leak kernel information to the user. */
+ if (bpf_needs_clear_a(&filter[0]))
emit_clear(r_A); /* A = 0 */
- }
for (i = 0; i < flen; i++) {
unsigned int K = filter[i].k;
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 4165e9ac9e36..5972ffe5719a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -493,6 +493,25 @@ static inline void bpf_jit_free(struct bpf_prog *fp)
#define BPF_ANC BIT(15)
+static inline bool bpf_needs_clear_a(const struct sock_filter *first)
+{
+ switch (first->code) {
+ case BPF_RET | BPF_K:
+ case BPF_LD | BPF_W | BPF_LEN:
+ return false;
+
+ case BPF_LD | BPF_W | BPF_ABS:
+ case BPF_LD | BPF_H | BPF_ABS:
+ case BPF_LD | BPF_B | BPF_ABS:
+ if (first->k == SKF_AD_OFF + SKF_AD_ALU_XOR_X)
+ return true;
+ return false;
+
+ default:
+ return true;
+ }
+}
+
static inline u16 bpf_anc_helper(const struct sock_filter *ftest)
{
BUG_ON(ftest->code & BPF_ANC);
--
2.6.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 15:23 [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X Rabin Vincent
@ 2016-01-05 16:00 ` Eric Dumazet
2016-01-05 16:03 ` Rabin Vincent
2016-01-05 16:36 ` Daniel Borkmann
2016-01-06 5:44 ` David Miller
2 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2016-01-05 16:00 UTC (permalink / raw)
To: Rabin Vincent
Cc: davem, netdev, daniel, ast, linux-arm-kernel, linux-mips,
linuxppc-dev, sparclinux, linux-kernel
On Tue, 2016-01-05 at 16:23 +0100, Rabin Vincent wrote:
> The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
> instructions since it XORs A with X while all the others replace A with
> some loaded value. All the BPF JITs fail to clear A if this is used as
> the first instruction in a filter.
Is x86_64 part of this 'All' subset ? ;)
> This was found using american fuzzy
> lop.
>
> Add a helper to determine if A needs to be cleared given the first
> instruction in a filter, and use this in the JITs. Except for ARM, the
> rest have only been compile-tested.
>
> Fixes: 3480593131e0 ("net: filter: get rid of BPF_S_* enum")
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 16:00 ` Eric Dumazet
@ 2016-01-05 16:03 ` Rabin Vincent
2016-01-05 16:37 ` Daniel Borkmann
0 siblings, 1 reply; 7+ messages in thread
From: Rabin Vincent @ 2016-01-05 16:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, netdev, daniel, ast, linux-arm-kernel, linux-mips,
linuxppc-dev, sparclinux, linux-kernel
On Tue, Jan 05, 2016 at 08:00:45AM -0800, Eric Dumazet wrote:
> On Tue, 2016-01-05 at 16:23 +0100, Rabin Vincent wrote:
> > The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
> > instructions since it XORs A with X while all the others replace A with
> > some loaded value. All the BPF JITs fail to clear A if this is used as
> > the first instruction in a filter.
>
> Is x86_64 part of this 'All' subset ? ;)
No, because it's an eBPF JIT.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 15:23 [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X Rabin Vincent
2016-01-05 16:00 ` Eric Dumazet
@ 2016-01-05 16:36 ` Daniel Borkmann
2016-01-05 17:43 ` Alexei Starovoitov
2016-01-06 5:44 ` David Miller
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2016-01-05 16:36 UTC (permalink / raw)
To: Rabin Vincent, davem
Cc: netdev, ast, linux-arm-kernel, linux-mips, linuxppc-dev,
sparclinux, linux-kernel
On 01/05/2016 04:23 PM, Rabin Vincent wrote:
> The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
> instructions since it XORs A with X while all the others replace A with
> some loaded value. All the BPF JITs fail to clear A if this is used as
> the first instruction in a filter. This was found using american fuzzy
> lop.
>
> Add a helper to determine if A needs to be cleared given the first
> instruction in a filter, and use this in the JITs. Except for ARM, the
> rest have only been compile-tested.
>
> Fixes: 3480593131e0 ("net: filter: get rid of BPF_S_* enum")
> Signed-off-by: Rabin Vincent <rabin@rab.in>
Excellent catch, thanks a lot! The fix looks good to me and should
go to -net tree.
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
If you're interested, feel free to add a small test case for the
SKF_AD_ALU_XOR_X issue to lib/test_bpf.c for -net-next tree. Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 16:03 ` Rabin Vincent
@ 2016-01-05 16:37 ` Daniel Borkmann
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2016-01-05 16:37 UTC (permalink / raw)
To: Rabin Vincent, Eric Dumazet
Cc: davem, netdev, ast, linux-arm-kernel, linux-mips, linuxppc-dev,
sparclinux, linux-kernel
On 01/05/2016 05:03 PM, Rabin Vincent wrote:
> On Tue, Jan 05, 2016 at 08:00:45AM -0800, Eric Dumazet wrote:
>> On Tue, 2016-01-05 at 16:23 +0100, Rabin Vincent wrote:
>>> The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
>>> instructions since it XORs A with X while all the others replace A with
>>> some loaded value. All the BPF JITs fail to clear A if this is used as
>>> the first instruction in a filter.
>>
>> Is x86_64 part of this 'All' subset ? ;)
>
> No, because it's an eBPF JIT.
Correct, filter conversion to eBPF clears it already.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 16:36 ` Daniel Borkmann
@ 2016-01-05 17:43 ` Alexei Starovoitov
0 siblings, 0 replies; 7+ messages in thread
From: Alexei Starovoitov @ 2016-01-05 17:43 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Rabin Vincent, davem, netdev, ast, linux-arm-kernel, linux-mips,
linuxppc-dev, sparclinux, linux-kernel
On Tue, Jan 05, 2016 at 05:36:47PM +0100, Daniel Borkmann wrote:
> On 01/05/2016 04:23 PM, Rabin Vincent wrote:
> >The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
> >instructions since it XORs A with X while all the others replace A with
> >some loaded value. All the BPF JITs fail to clear A if this is used as
> >the first instruction in a filter. This was found using american fuzzy
> >lop.
> >
> >Add a helper to determine if A needs to be cleared given the first
> >instruction in a filter, and use this in the JITs. Except for ARM, the
> >rest have only been compile-tested.
> >
> >Fixes: 3480593131e0 ("net: filter: get rid of BPF_S_* enum")
> >Signed-off-by: Rabin Vincent <rabin@rab.in>
>
> Excellent catch, thanks a lot! The fix looks good to me and should
> go to -net tree.
>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
good catch indeed.
Classic bpf jits didn't have much love. Great to see this work.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X
2016-01-05 15:23 [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X Rabin Vincent
2016-01-05 16:00 ` Eric Dumazet
2016-01-05 16:36 ` Daniel Borkmann
@ 2016-01-06 5:44 ` David Miller
2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2016-01-06 5:44 UTC (permalink / raw)
To: rabin
Cc: netdev, daniel, ast, linux-arm-kernel, linux-mips, linuxppc-dev,
sparclinux, linux-kernel
From: Rabin Vincent <rabin@rab.in>
Date: Tue, 5 Jan 2016 16:23:07 +0100
> The SKF_AD_ALU_XOR_X ancillary is not like the other ancillary data
> instructions since it XORs A with X while all the others replace A with
> some loaded value. All the BPF JITs fail to clear A if this is used as
> the first instruction in a filter. This was found using american fuzzy
> lop.
>
> Add a helper to determine if A needs to be cleared given the first
> instruction in a filter, and use this in the JITs. Except for ARM, the
> rest have only been compile-tested.
>
> Fixes: 3480593131e0 ("net: filter: get rid of BPF_S_* enum")
> Signed-off-by: Rabin Vincent <rabin@rab.in>
Applied and queued up for -stable, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-06 5:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-05 15:23 [PATCH] net: filter: make JITs zero A for SKF_AD_ALU_XOR_X Rabin Vincent
2016-01-05 16:00 ` Eric Dumazet
2016-01-05 16:03 ` Rabin Vincent
2016-01-05 16:37 ` Daniel Borkmann
2016-01-05 16:36 ` Daniel Borkmann
2016-01-05 17:43 ` Alexei Starovoitov
2016-01-06 5:44 ` David Miller
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).