[PATCH 0/4] Seccomp filter JIT support on ARM.

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Nicolas Schichan]

Greetings,

The following patches allow the use of the existing JIT code under
arch/arm for seccomp filters.

The first patch makes bpf_migrate_filter() available so that seccomp
code can use it.

The second patch invokes the classic BPF JIT code and if this fails
reverts to the internal BPF. The open coded double call to
bpf_convert_filter() and bpf_prog_select_runtime() is replaced by
bpf_migrate_filter()

The third patch adds support for loads from the seccomp_data structure in
the ARM BPF JIT code.

The fourth and final patch fixes a bug in the emit_udiv() function
when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
ARM BPF JIT code.

Regards,

Nicolas Schichan (4):
  net: filter: make bpf_migrate_filter available outside of
    net/core/filter.c
  seccomp: rework seccomp_prepare_filter().
  ARM: net: add JIT support for loads from struct seccomp_data.
  ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

 arch/arm/net/bpf_jit_32.c | 14 ++++++++++++--
 include/linux/filter.h    |  1 +
 kernel/seccomp.c          | 47 ++++++++++++++++++++---------------------------
 net/core/filter.c         |  2 +-
 4 files changed, 34 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH 3/4] ARM: net: add JIT support for loads from struct seccomp_data.

[Nicolas Schichan]

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index e1268f9..b5f470d 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -852,6 +852,16 @@ b_epilogue:
 			off = offsetof(struct sk_buff, queue_mapping);
 			emit(ARM_LDRH_I(r_A, r_skb, off), ctx);
 			break;
+		case BPF_LDX | BPF_W | BPF_ABS:
+			/*
+			 * load a 32bit word from struct seccomp_data.
+			 * seccomp_check_filter() will already have checked
+			 * that k is 32bit aligned and lies within the
+			 * struct seccomp_data.
+			 */
+			ctx->seen |= SEEN_SKB;
+			emit(ARM_LDR_I(r_A, r_skb, k), ctx);
+			break;
 		default:
 			return -1;
 		}
-- 
1.9.1

[PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Nicolas Schichan]

In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch)
and loading rm first into ARM_R0 will result in jit_udiv() function
being called the same dividend and divisor. Fix that by loading rn
first into ARM_R1 and then rm into ARM_R0.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index b5f470d..ffaf311 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
 		return;
 	}
 #endif
-	if (rm != ARM_R0)
-		emit(ARM_MOV_R(ARM_R0, rm), ctx);
 	if (rn != ARM_R1)
 		emit(ARM_MOV_R(ARM_R1, rn), ctx);
+	if (rm != ARM_R0)
+		emit(ARM_MOV_R(ARM_R0, rm), ctx);
 
 	ctx->seen |= SEEN_CALL;
 	emit_mov_i(ARM_R3, (u32)jit_udiv, ctx);
-- 
1.9.1

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Daniel Borkmann]

On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
...
> The fourth and final patch fixes a bug in the emit_udiv() function
> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
> ARM BPF JIT code.

Shouldn't that fix go separately, so it can be included into 4.1
resp. -stable?

Would be good if you also Cc Mircea, who wrote the code. Was that
caught by lib/test_bpf.c suite (if not, would be good to add a test
case for it) ?

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Alexei Starovoitov]

On Wed, Apr 29, 2015 at 03:37:33PM +0200, Nicolas Schichan wrote:
> Greetings,
> 
> The following patches allow the use of the existing JIT code under
> arch/arm for seccomp filters.
> 
> The first patch makes bpf_migrate_filter() available so that seccomp
> code can use it.
> 
> The second patch invokes the classic BPF JIT code and if this fails
> reverts to the internal BPF. The open coded double call to
> bpf_convert_filter() and bpf_prog_select_runtime() is replaced by
> bpf_migrate_filter()
> 
> The third patch adds support for loads from the seccomp_data structure in
> the ARM BPF JIT code.

the patches 1,2,3 look fine.

> The fourth and final patch fixes a bug in the emit_udiv() function
> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
> ARM BPF JIT code.

this one looks independent from first three and probably should
be sent to stable?

Your cc list is different for every patch, why? What tree are you
thinking to go through? 1st patch is trivial, but 2nd belongs in
seccomp, 3rd and 4th are in arm32... I'd would say arm tree if Kees
is ok with it.

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Nicolas Schichan]

On 04/29/2015 06:37 PM, Daniel Borkmann wrote:
> On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
> ...
>> The fourth and final patch fixes a bug in the emit_udiv() function
>> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
>> ARM BPF JIT code.
> 
> Shouldn't that fix go separately, so it can be included into 4.1
> resp. -stable?

Sure, shall I resend that separately from the v2 of the serie or is it fine in
its current form ?

> Would be good if you also Cc Mircea, who wrote the code. Was that
> caught by lib/test_bpf.c suite (if not, would be good to add a test
> case for it) ?

It was detected by an internal test suite we have here. I see that there are
some BPF_ALU | BPF_DIV | BPF_K instructions so it might also be caught by
lib/test_bpf.c as well.

Regards,

-- 
Nicolas Schichan
Freebox SAS

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Daniel Borkmann]

On 04/30/2015 02:35 PM, Nicolas Schichan wrote:
> On 04/29/2015 06:37 PM, Daniel Borkmann wrote:
>> On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
>> ...
>>> The fourth and final patch fixes a bug in the emit_udiv() function
>>> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
>>> ARM BPF JIT code.
>>
>> Shouldn't that fix go separately, so it can be included into 4.1
>> resp. -stable?
>
> Sure, shall I resend that separately from the v2 of the serie or is it fine in
> its current form ?

Would be great if you could send that as an individual patch, since
it's a stand-alone fix and independent from the (feature) patch set.

>> Would be good if you also Cc Mircea, who wrote the code. Was that
>> caught by lib/test_bpf.c suite (if not, would be good to add a test
>> case for it) ?
>
> It was detected by an internal test suite we have here. I see that there are
> some BPF_ALU | BPF_DIV | BPF_K instructions so it might also be caught by
> lib/test_bpf.c as well.

If there are some additional tests that are not yet covered by lib/test_bpf.c,
I'd be happy if you could add them there. This can also be as a follow-up,
but if we can increase coverage for others as well, the better.

Thanks again,
Daniel

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Alexei Starovoitov]

On 4/30/15 5:51 AM, Daniel Borkmann wrote:
>
> If there are some additional tests that are not yet covered by
> lib/test_bpf.c,
> I'd be happy if you could add them there. This can also be as a follow-up,
> but if we can increase coverage for others as well, the better.

btw, Michael have been working on adding 173 new tests to test_bpf.
Michael, could you submit them to net-next ?

[PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Russell King - ARM Linux]

On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
> In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch)
> and loading rm first into ARM_R0 will result in jit_udiv() function
> being called the same dividend and divisor. Fix that by loading rn
> first into ARM_R1 and then rm into ARM_R0.
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> ---
>  arch/arm/net/bpf_jit_32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index b5f470d..ffaf311 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
>  		return;
>  	}
>  #endif
> -	if (rm != ARM_R0)
> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
>  	if (rn != ARM_R1)
>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
> +	if (rm != ARM_R0)
> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);

I don't think you've thought enough about this.  What if rm is ARM_R1?
What if rn = ARM_R0 and rm = ARM_R1?

How about:

	if (rn == ARM_R0 && rm == ARM_R1) {
		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
	} else if (rn == ARM_R0) {
		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
		if (rm != ARM_R0)
			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
	} else {
		if (rm != ARM_R0)
			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
		if (rn != ARM_R1)
			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
	}

-- 
FTTC broadband for 0.8mile line: currently@10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Nicolas Schichan]

On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
[...]
>> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
>> index b5f470d..ffaf311 100644
>> --- a/arch/arm/net/bpf_jit_32.c
>> +++ b/arch/arm/net/bpf_jit_32.c
>> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
>>  		return;
>>  	}
>>  #endif
>> -	if (rm != ARM_R0)
>> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
>>  	if (rn != ARM_R1)
>>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
>> +	if (rm != ARM_R0)
>> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> 
> I don't think you've thought enough about this.  What if rm is ARM_R1?
> What if rn = ARM_R0 and rm = ARM_R1?
>
> How about:
> 
> 	if (rn == ARM_R0 && rm == ARM_R1) {
> 		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
> 		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
> 		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
> 	} else if (rn == ARM_R0) {
> 		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> 		if (rm != ARM_R0)
> 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> 	} else {
> 		if (rm != ARM_R0)
> 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> 		if (rn != ARM_R1)
> 			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> 	}
> 

Hello Russell,

In the current JIT, emit_udiv() is only being called with:

- rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K

- rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X

so it should not cause any issue in the current code state.

But yes, I'll rework the patch to avoid any other nasty surprises should the
code change.

Thanks,

-- 
Nicolas Schichan
Freebox SAS

[PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Russell King - ARM Linux]

On Mon, May 04, 2015 at 06:16:30PM +0200, Nicolas Schichan wrote:
> On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote:
> > On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
> [...]
> >> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> >> index b5f470d..ffaf311 100644
> >> --- a/arch/arm/net/bpf_jit_32.c
> >> +++ b/arch/arm/net/bpf_jit_32.c
> >> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
> >>  		return;
> >>  	}
> >>  #endif
> >> -	if (rm != ARM_R0)
> >> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> >>  	if (rn != ARM_R1)
> >>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
> >> +	if (rm != ARM_R0)
> >> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> > 
> > I don't think you've thought enough about this.  What if rm is ARM_R1?
> > What if rn = ARM_R0 and rm = ARM_R1?
> >
> > How about:
> > 
> > 	if (rn == ARM_R0 && rm == ARM_R1) {
> > 		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
> > 		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
> > 		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
> > 	} else if (rn == ARM_R0) {
> > 		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> > 		if (rm != ARM_R0)
> > 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> > 	} else {
> > 		if (rm != ARM_R0)
> > 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> > 		if (rn != ARM_R1)
> > 			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> > 	}
> > 
> 
> Hello Russell,
> 
> In the current JIT, emit_udiv() is only being called with:
> 
> - rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K
> 
> - rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X
> 
> so it should not cause any issue in the current code state.
> 
> But yes, I'll rework the patch to avoid any other nasty surprises should the
> code change.

Maybe then add a comment detailing the current conditions that this is
coded for so that if you're not around when the code changes, others are
aware of the issue.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.