All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12  7:11 ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12  7:11 UTC (permalink / raw)
  To: will.deacon
  Cc: catalin.marinas, davem, zlim.lnx, ast, dborkman,
	linux-arm-kernel, linux-kernel

This is the ARM64 variant for 314beb9bcab ("x86: bpf_jit_comp: secure bpf
jit against spraying attacks").

Thanks to commit 11d91a770f1f ("arm64: Add CONFIG_DEBUG_SET_MODULE_RONX
support") which added necessary infrastructure, we can now implement
RO marking of eBPF generated JIT image pages and randomize start offset
for the JIT code, so that it does not reside directly on a page boundary
anymore. Likewise, the holes are filled with illegal instructions.

This is basically the ARM64 variant of what we already have in ARM via
commit 55309dd3d4cd ("net: bpf: arm: address randomize and write protect
JIT code"). Moreover, this commit also presents a merge resolution due to
conflicts with commit 60a3b2253c41 ("net: bpf: make eBPF interpreter images
read-only") as we don't use kfree() in bpf_jit_free() anymore to release
the locked bpf_prog structure, but instead bpf_prog_unlock_free() through
a different allocator.

JIT tested on aarch64 with BPF test suite.

Reference: http://mainisusuallyafunction.blogspot.com/2012/11/attacking-hardened-linux-systems-with.html
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Reviewed-by: Zi Shen Lim <zlim.lnx@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Alexei Starovoitov <ast@plumgrid.com>
---
 README:

 Will, Catalin, Dave, this is more or less a heads-up: when net-next and
 arm64-next tree will get both merged into Linus' tree, we will run into
 a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
 and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
 assume nobody actually _runs_ linux-next, but not sure about that though.

 The reason is that in net-next tree we did some BPF-wide (incl. JIT)
 changes regarding the allocator which are currently _not_ present here
 while eBPF JIT currently is _only_ available here.

 Zi offered to alternatively only have this one-liner replacement and
 would rebase this one on top of it. Both is fine by me, I think this one
 would be important to have as well since we have already migrated all
 other archs that support DEBUG_SET_MODULE_RONX to this (ARM32 with
 Mircea's consent pending in net-next).

 This patch here is on top of the module memory leak patch from yesterday
 but requires some of the dependencies from net-next below.

 If you want to look it up, on the top of my head, revelant commits from
 Dave's net-next tree are:

  60a3b2253c413cf601783b070507d7dd6620c954
  738cbe72adc5c8f2016c4c68aa5162631d4f27e1
  55309dd3d4cd7420376a3de0526d6ed24ff8fa76
  b954d83421d51d822c42e5ab7b65069b25ad3005

 How do we handle this? Would I need to resend this patch when the time
 comes or would you ARM64 guys take care of it automagically? ;)

 Thanks a lot!

 arch/arm64/net/bpf_jit_comp.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 7ae3354..4b71779 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -19,7 +19,6 @@
 #define pr_fmt(fmt) "bpf_jit: " fmt
 
 #include <linux/filter.h>
-#include <linux/moduleloader.h>
 #include <linux/printk.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
@@ -119,6 +118,15 @@ static inline int bpf2a64_offset(int bpf_to, int bpf_from,
 	return to - from;
 }
 
+static void jit_fill_hole(void *area, unsigned int size)
+{
+	/* Insert illegal UND instructions. */
+	u32 *ptr, fill_ins = 0xe7ffffff;
+	/* We are guaranteed to have aligned memory. */
+	for (ptr = area; size >= sizeof(u32); size -= sizeof(u32))
+		*ptr++ = fill_ins;
+}
+
 static inline int epilogue_offset(const struct jit_ctx *ctx)
 {
 	int to = ctx->offset[ctx->prog->len - 1];
@@ -613,8 +621,10 @@ void bpf_jit_compile(struct bpf_prog *prog)
 
 void bpf_int_jit_compile(struct bpf_prog *prog)
 {
+	struct bpf_binary_header *header;
 	struct jit_ctx ctx;
 	int image_size;
+	u8 *image_ptr;
 
 	if (!bpf_jit_enable)
 		return;
@@ -636,23 +646,25 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
 		goto out;
 
 	build_prologue(&ctx);
-
 	build_epilogue(&ctx);
 
 	/* Now we know the actual image size. */
 	image_size = sizeof(u32) * ctx.idx;
-	ctx.image = module_alloc(image_size);
-	if (unlikely(ctx.image == NULL))
+	header = bpf_jit_binary_alloc(image_size, &image_ptr,
+				      sizeof(u32), jit_fill_hole);
+	if (header == NULL)
 		goto out;
 
 	/* 2. Now, the actual pass. */
 
+	ctx.image = (u32 *)image_ptr;
 	ctx.idx = 0;
+
 	build_prologue(&ctx);
 
 	ctx.body_offset = ctx.idx;
 	if (build_body(&ctx)) {
-		module_free(NULL, ctx.image);
+		bpf_jit_binary_free(header);
 		goto out;
 	}
 
@@ -663,17 +675,25 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
 		bpf_jit_dump(prog->len, image_size, 2, ctx.image);
 
 	bpf_flush_icache(ctx.image, ctx.image + ctx.idx);
+
+	set_memory_ro((unsigned long)header, header->pages);
 	prog->bpf_func = (void *)ctx.image;
 	prog->jited = 1;
-
 out:
 	kfree(ctx.offset);
 }
 
 void bpf_jit_free(struct bpf_prog *prog)
 {
-	if (prog->jited)
-		module_free(NULL, prog->bpf_func);
+	unsigned long addr = (unsigned long)prog->bpf_func & PAGE_MASK;
+	struct bpf_binary_header *header = (void *)addr;
+
+	if (!prog->jited)
+		goto free_filter;
+
+	set_memory_rw(addr, header->pages);
+	bpf_jit_binary_free(header);
 
-	kfree(prog);
+free_filter:
+	bpf_prog_unlock_free(prog);
 }
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12  7:11 ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12  7:11 UTC (permalink / raw)
  To: linux-arm-kernel

This is the ARM64 variant for 314beb9bcab ("x86: bpf_jit_comp: secure bpf
jit against spraying attacks").

Thanks to commit 11d91a770f1f ("arm64: Add CONFIG_DEBUG_SET_MODULE_RONX
support") which added necessary infrastructure, we can now implement
RO marking of eBPF generated JIT image pages and randomize start offset
for the JIT code, so that it does not reside directly on a page boundary
anymore. Likewise, the holes are filled with illegal instructions.

This is basically the ARM64 variant of what we already have in ARM via
commit 55309dd3d4cd ("net: bpf: arm: address randomize and write protect
JIT code"). Moreover, this commit also presents a merge resolution due to
conflicts with commit 60a3b2253c41 ("net: bpf: make eBPF interpreter images
read-only") as we don't use kfree() in bpf_jit_free() anymore to release
the locked bpf_prog structure, but instead bpf_prog_unlock_free() through
a different allocator.

JIT tested on aarch64 with BPF test suite.

Reference: http://mainisusuallyafunction.blogspot.com/2012/11/attacking-hardened-linux-systems-with.html
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Reviewed-by: Zi Shen Lim <zlim.lnx@gmail.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Alexei Starovoitov <ast@plumgrid.com>
---
 README:

 Will, Catalin, Dave, this is more or less a heads-up: when net-next and
 arm64-next tree will get both merged into Linus' tree, we will run into
 a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
 and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
 assume nobody actually _runs_ linux-next, but not sure about that though.

 The reason is that in net-next tree we did some BPF-wide (incl. JIT)
 changes regarding the allocator which are currently _not_ present here
 while eBPF JIT currently is _only_ available here.

 Zi offered to alternatively only have this one-liner replacement and
 would rebase this one on top of it. Both is fine by me, I think this one
 would be important to have as well since we have already migrated all
 other archs that support DEBUG_SET_MODULE_RONX to this (ARM32 with
 Mircea's consent pending in net-next).

 This patch here is on top of the module memory leak patch from yesterday
 but requires some of the dependencies from net-next below.

 If you want to look it up, on the top of my head, revelant commits from
 Dave's net-next tree are:

  60a3b2253c413cf601783b070507d7dd6620c954
  738cbe72adc5c8f2016c4c68aa5162631d4f27e1
  55309dd3d4cd7420376a3de0526d6ed24ff8fa76
  b954d83421d51d822c42e5ab7b65069b25ad3005

 How do we handle this? Would I need to resend this patch when the time
 comes or would you ARM64 guys take care of it automagically? ;)

 Thanks a lot!

 arch/arm64/net/bpf_jit_comp.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 7ae3354..4b71779 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -19,7 +19,6 @@
 #define pr_fmt(fmt) "bpf_jit: " fmt
 
 #include <linux/filter.h>
-#include <linux/moduleloader.h>
 #include <linux/printk.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
@@ -119,6 +118,15 @@ static inline int bpf2a64_offset(int bpf_to, int bpf_from,
 	return to - from;
 }
 
+static void jit_fill_hole(void *area, unsigned int size)
+{
+	/* Insert illegal UND instructions. */
+	u32 *ptr, fill_ins = 0xe7ffffff;
+	/* We are guaranteed to have aligned memory. */
+	for (ptr = area; size >= sizeof(u32); size -= sizeof(u32))
+		*ptr++ = fill_ins;
+}
+
 static inline int epilogue_offset(const struct jit_ctx *ctx)
 {
 	int to = ctx->offset[ctx->prog->len - 1];
@@ -613,8 +621,10 @@ void bpf_jit_compile(struct bpf_prog *prog)
 
 void bpf_int_jit_compile(struct bpf_prog *prog)
 {
+	struct bpf_binary_header *header;
 	struct jit_ctx ctx;
 	int image_size;
+	u8 *image_ptr;
 
 	if (!bpf_jit_enable)
 		return;
@@ -636,23 +646,25 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
 		goto out;
 
 	build_prologue(&ctx);
-
 	build_epilogue(&ctx);
 
 	/* Now we know the actual image size. */
 	image_size = sizeof(u32) * ctx.idx;
-	ctx.image = module_alloc(image_size);
-	if (unlikely(ctx.image == NULL))
+	header = bpf_jit_binary_alloc(image_size, &image_ptr,
+				      sizeof(u32), jit_fill_hole);
+	if (header == NULL)
 		goto out;
 
 	/* 2. Now, the actual pass. */
 
+	ctx.image = (u32 *)image_ptr;
 	ctx.idx = 0;
+
 	build_prologue(&ctx);
 
 	ctx.body_offset = ctx.idx;
 	if (build_body(&ctx)) {
-		module_free(NULL, ctx.image);
+		bpf_jit_binary_free(header);
 		goto out;
 	}
 
@@ -663,17 +675,25 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
 		bpf_jit_dump(prog->len, image_size, 2, ctx.image);
 
 	bpf_flush_icache(ctx.image, ctx.image + ctx.idx);
+
+	set_memory_ro((unsigned long)header, header->pages);
 	prog->bpf_func = (void *)ctx.image;
 	prog->jited = 1;
-
 out:
 	kfree(ctx.offset);
 }
 
 void bpf_jit_free(struct bpf_prog *prog)
 {
-	if (prog->jited)
-		module_free(NULL, prog->bpf_func);
+	unsigned long addr = (unsigned long)prog->bpf_func & PAGE_MASK;
+	struct bpf_binary_header *header = (void *)addr;
+
+	if (!prog->jited)
+		goto free_filter;
+
+	set_memory_rw(addr, header->pages);
+	bpf_jit_binary_free(header);
 
-	kfree(prog);
+free_filter:
+	bpf_prog_unlock_free(prog);
 }
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12  7:11 ` Daniel Borkmann
@ 2014-09-12 16:03   ` Catalin Marinas
  -1 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 16:03 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

Daniel,

On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>  Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>  arm64-next tree will get both merged into Linus' tree, we will run into
>  a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>  and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>  assume nobody actually _runs_ linux-next, but not sure about that though.

Some people do.

>  How do we handle this? Would I need to resend this patch when the time
>  comes or would you ARM64 guys take care of it automagically? ;)

I think we could disable BPF for arm64 until -rc1 and re-enable it
together with this patch.

One comment below:

> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
[...]
> +static void jit_fill_hole(void *area, unsigned int size)
> +{
> +	/* Insert illegal UND instructions. */
> +	u32 *ptr, fill_ins = 0xe7ffffff;

On arm64 we don't have a guaranteed undefined instruction space (and
Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
valid instruction, it seems that you used the same value).

I think the only guaranteed way is to use the BRK #imm instruction but
it requires some changes to the handling code as it is currently used
for kgdb (unless you can use two instructions for filling in which could
generate a NULL pointer access).

-- 
Catalin


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 16:03   ` Catalin Marinas
  0 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 16:03 UTC (permalink / raw)
  To: linux-arm-kernel

Daniel,

On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>  Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>  arm64-next tree will get both merged into Linus' tree, we will run into
>  a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>  and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>  assume nobody actually _runs_ linux-next, but not sure about that though.

Some people do.

>  How do we handle this? Would I need to resend this patch when the time
>  comes or would you ARM64 guys take care of it automagically? ;)

I think we could disable BPF for arm64 until -rc1 and re-enable it
together with this patch.

One comment below:

> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
[...]
> +static void jit_fill_hole(void *area, unsigned int size)
> +{
> +	/* Insert illegal UND instructions. */
> +	u32 *ptr, fill_ins = 0xe7ffffff;

On arm64 we don't have a guaranteed undefined instruction space (and
Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
valid instruction, it seems that you used the same value).

I think the only guaranteed way is to use the BRK #imm instruction but
it requires some changes to the handling code as it is currently used
for kgdb (unless you can use two instructions for filling in which could
generate a NULL pointer access).

-- 
Catalin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 16:03   ` Catalin Marinas
@ 2014-09-12 16:21     ` Daniel Borkmann
  -1 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 16:21 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> Daniel,
>
> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>>   arm64-next tree will get both merged into Linus' tree, we will run into
>>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>>   assume nobody actually _runs_ linux-next, but not sure about that though.
>
> Some people do.
>
>>   How do we handle this? Would I need to resend this patch when the time
>>   comes or would you ARM64 guys take care of it automagically? ;)
>
> I think we could disable BPF for arm64 until -rc1 and re-enable it
> together with this patch.

Ok, yes, that would mitigate it a bit. Sounds fine to me.

> One comment below:
>
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
> [...]
>> +static void jit_fill_hole(void *area, unsigned int size)
>> +{
>> +	/* Insert illegal UND instructions. */
>> +	u32 *ptr, fill_ins = 0xe7ffffff;
>
> On arm64 we don't have a guaranteed undefined instruction space (and
> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> valid instruction, it seems that you used the same value).

Hm, ok, the boards we've tried out and where Zi tested it too, it worked.

> I think the only guaranteed way is to use the BRK #imm instruction but
> it requires some changes to the handling code as it is currently used
> for kgdb (unless you can use two instructions for filling in which could
> generate a NULL pointer access).

The trade-off would be that if we align on 8, it would certainly increase
the probability to jump to the right offset. Note, on x86_64 we have no
alignment requirements, hence 1, and on s390x only alignment of 2.

So, on that few (?) boards where UND would be a valid instruction [ as
opposed to crash the kernel ], would it translate into a NOP and just
'walk' from there into the JIT image?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 16:21     ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 16:21 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> Daniel,
>
> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>>   arm64-next tree will get both merged into Linus' tree, we will run into
>>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>>   assume nobody actually _runs_ linux-next, but not sure about that though.
>
> Some people do.
>
>>   How do we handle this? Would I need to resend this patch when the time
>>   comes or would you ARM64 guys take care of it automagically? ;)
>
> I think we could disable BPF for arm64 until -rc1 and re-enable it
> together with this patch.

Ok, yes, that would mitigate it a bit. Sounds fine to me.

> One comment below:
>
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
> [...]
>> +static void jit_fill_hole(void *area, unsigned int size)
>> +{
>> +	/* Insert illegal UND instructions. */
>> +	u32 *ptr, fill_ins = 0xe7ffffff;
>
> On arm64 we don't have a guaranteed undefined instruction space (and
> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> valid instruction, it seems that you used the same value).

Hm, ok, the boards we've tried out and where Zi tested it too, it worked.

> I think the only guaranteed way is to use the BRK #imm instruction but
> it requires some changes to the handling code as it is currently used
> for kgdb (unless you can use two instructions for filling in which could
> generate a NULL pointer access).

The trade-off would be that if we align on 8, it would certainly increase
the probability to jump to the right offset. Note, on x86_64 we have no
alignment requirements, hence 1, and on s390x only alignment of 2.

So, on that few (?) boards where UND would be a valid instruction [ as
opposed to crash the kernel ], would it translate into a NOP and just
'walk' from there into the JIT image?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 16:21     ` Daniel Borkmann
@ 2014-09-12 16:46       ` Catalin Marinas
  -1 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 16:46 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> >>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
> >>   arm64-next tree will get both merged into Linus' tree, we will run into
> >>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
> >>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
> >>   assume nobody actually _runs_ linux-next, but not sure about that though.
> >
> > Some people do.
> >
> >>   How do we handle this? Would I need to resend this patch when the time
> >>   comes or would you ARM64 guys take care of it automagically? ;)
> >
> > I think we could disable BPF for arm64 until -rc1 and re-enable it
> > together with this patch.
> 
> Ok, yes, that would mitigate it a bit. Sounds fine to me.
> 
> > One comment below:
> >
> >> --- a/arch/arm64/net/bpf_jit_comp.c
> >> +++ b/arch/arm64/net/bpf_jit_comp.c
> > [...]
> >> +static void jit_fill_hole(void *area, unsigned int size)
> >> +{
> >> +	/* Insert illegal UND instructions. */
> >> +	u32 *ptr, fill_ins = 0xe7ffffff;
> >
> > On arm64 we don't have a guaranteed undefined instruction space (and
> > Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> > valid instruction, it seems that you used the same value).
> 
> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.

So, if I try this:

$ echo 0xffffffe7 | xxd -r > test.bin
$ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
...
   0:   e7ffffff        udf     #65535  ; 0xffff

Do you use the same constant on arm32?

> > I think the only guaranteed way is to use the BRK #imm instruction but
> > it requires some changes to the handling code as it is currently used
> > for kgdb (unless you can use two instructions for filling in which could
> > generate a NULL pointer access).
> 
> The trade-off would be that if we align on 8, it would certainly increase
> the probability to jump to the right offset. Note, on x86_64 we have no
> alignment requirements, hence 1, and on s390x only alignment of 2.
> 
> So, on that few (?) boards where UND would be a valid instruction [ as
> opposed to crash the kernel ], would it translate into a NOP and just
> 'walk' from there into the JIT image?

On current ARMv8 CPU implementations, the above constant is unallocated
in the A64 instruction space. But you never know, it may be allocated in
the future.

I think it's easier if you just use something like BRK #0x100 (opcode
0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
0x400 and 0x401).

An unallocated BRK would trigger a fault via do_debug_exception ->
brk_handler and panic the kernel.

-- 
Catalin


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 16:46       ` Catalin Marinas
  0 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 16:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> >>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
> >>   arm64-next tree will get both merged into Linus' tree, we will run into
> >>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
> >>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
> >>   assume nobody actually _runs_ linux-next, but not sure about that though.
> >
> > Some people do.
> >
> >>   How do we handle this? Would I need to resend this patch when the time
> >>   comes or would you ARM64 guys take care of it automagically? ;)
> >
> > I think we could disable BPF for arm64 until -rc1 and re-enable it
> > together with this patch.
> 
> Ok, yes, that would mitigate it a bit. Sounds fine to me.
> 
> > One comment below:
> >
> >> --- a/arch/arm64/net/bpf_jit_comp.c
> >> +++ b/arch/arm64/net/bpf_jit_comp.c
> > [...]
> >> +static void jit_fill_hole(void *area, unsigned int size)
> >> +{
> >> +	/* Insert illegal UND instructions. */
> >> +	u32 *ptr, fill_ins = 0xe7ffffff;
> >
> > On arm64 we don't have a guaranteed undefined instruction space (and
> > Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> > valid instruction, it seems that you used the same value).
> 
> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.

So, if I try this:

$ echo 0xffffffe7 | xxd -r > test.bin
$ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
...
   0:   e7ffffff        udf     #65535  ; 0xffff

Do you use the same constant on arm32?

> > I think the only guaranteed way is to use the BRK #imm instruction but
> > it requires some changes to the handling code as it is currently used
> > for kgdb (unless you can use two instructions for filling in which could
> > generate a NULL pointer access).
> 
> The trade-off would be that if we align on 8, it would certainly increase
> the probability to jump to the right offset. Note, on x86_64 we have no
> alignment requirements, hence 1, and on s390x only alignment of 2.
> 
> So, on that few (?) boards where UND would be a valid instruction [ as
> opposed to crash the kernel ], would it translate into a NOP and just
> 'walk' from there into the JIT image?

On current ARMv8 CPU implementations, the above constant is unallocated
in the A64 instruction space. But you never know, it may be allocated in
the future.

I think it's easier if you just use something like BRK #0x100 (opcode
0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
0x400 and 0x401).

An unallocated BRK would trigger a fault via do_debug_exception ->
brk_handler and panic the kernel.

-- 
Catalin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 16:46       ` Catalin Marinas
@ 2014-09-12 17:10         ` Will Deacon
  -1 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2014-09-12 17:10 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Daniel Borkmann, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On Fri, Sep 12, 2014 at 05:46:57PM +0100, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> > On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> > > On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> > >>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
> > >>   arm64-next tree will get both merged into Linus' tree, we will run into
> > >>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
> > >>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
> > >>   assume nobody actually _runs_ linux-next, but not sure about that though.
> > >
> > > Some people do.
> > >
> > >>   How do we handle this? Would I need to resend this patch when the time
> > >>   comes or would you ARM64 guys take care of it automagically? ;)
> > >
> > > I think we could disable BPF for arm64 until -rc1 and re-enable it
> > > together with this patch.
> > 
> > Ok, yes, that would mitigate it a bit. Sounds fine to me.
> > 
> > > One comment below:
> > >
> > >> --- a/arch/arm64/net/bpf_jit_comp.c
> > >> +++ b/arch/arm64/net/bpf_jit_comp.c
> > > [...]
> > >> +static void jit_fill_hole(void *area, unsigned int size)
> > >> +{
> > >> +	/* Insert illegal UND instructions. */
> > >> +	u32 *ptr, fill_ins = 0xe7ffffff;
> > >
> > > On arm64 we don't have a guaranteed undefined instruction space (and
> > > Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> > > valid instruction, it seems that you used the same value).
> > 
> > Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
> 
> So, if I try this:
> 
> $ echo 0xffffffe7 | xxd -r > test.bin
> $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> ...
>    0:   e7ffffff        udf     #65535  ; 0xffff


...and for Thumb, it ends up as:

0:	ffff e7ff	vqshl.u64	q15, <illegal reg q15.5>, #63

which does happen to be undefined, but it feels fragile to rely on that
particular instruction form always having UNDEF behaviour.

> Do you use the same constant on arm32?
> 
> > > I think the only guaranteed way is to use the BRK #imm instruction but
> > > it requires some changes to the handling code as it is currently used
> > > for kgdb (unless you can use two instructions for filling in which could
> > > generate a NULL pointer access).
> > 
> > The trade-off would be that if we align on 8, it would certainly increase
> > the probability to jump to the right offset. Note, on x86_64 we have no
> > alignment requirements, hence 1, and on s390x only alignment of 2.
> > 
> > So, on that few (?) boards where UND would be a valid instruction [ as
> > opposed to crash the kernel ], would it translate into a NOP and just
> > 'walk' from there into the JIT image?
> 
> On current ARMv8 CPU implementations, the above constant is unallocated
> in the A64 instruction space. But you never know, it may be allocated in
> the future.
> 
> I think it's easier if you just use something like BRK #0x100 (opcode
> 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> 0x400 and 0x401).
> 
> An unallocated BRK would trigger a fault via do_debug_exception ->
> brk_handler and panic the kernel.

Sounds sensible.

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 17:10         ` Will Deacon
  0 siblings, 0 replies; 16+ messages in thread
From: Will Deacon @ 2014-09-12 17:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 12, 2014 at 05:46:57PM +0100, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> > On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> > > On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> > >>   Will, Catalin, Dave, this is more or less a heads-up: when net-next and
> > >>   arm64-next tree will get both merged into Linus' tree, we will run into
> > >>   a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
> > >>   and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
> > >>   assume nobody actually _runs_ linux-next, but not sure about that though.
> > >
> > > Some people do.
> > >
> > >>   How do we handle this? Would I need to resend this patch when the time
> > >>   comes or would you ARM64 guys take care of it automagically? ;)
> > >
> > > I think we could disable BPF for arm64 until -rc1 and re-enable it
> > > together with this patch.
> > 
> > Ok, yes, that would mitigate it a bit. Sounds fine to me.
> > 
> > > One comment below:
> > >
> > >> --- a/arch/arm64/net/bpf_jit_comp.c
> > >> +++ b/arch/arm64/net/bpf_jit_comp.c
> > > [...]
> > >> +static void jit_fill_hole(void *area, unsigned int size)
> > >> +{
> > >> +	/* Insert illegal UND instructions. */
> > >> +	u32 *ptr, fill_ins = 0xe7ffffff;
> > >
> > > On arm64 we don't have a guaranteed undefined instruction space (and
> > > Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> > > valid instruction, it seems that you used the same value).
> > 
> > Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
> 
> So, if I try this:
> 
> $ echo 0xffffffe7 | xxd -r > test.bin
> $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> ...
>    0:   e7ffffff        udf     #65535  ; 0xffff


...and for Thumb, it ends up as:

0:	ffff e7ff	vqshl.u64	q15, <illegal reg q15.5>, #63

which does happen to be undefined, but it feels fragile to rely on that
particular instruction form always having UNDEF behaviour.

> Do you use the same constant on arm32?
> 
> > > I think the only guaranteed way is to use the BRK #imm instruction but
> > > it requires some changes to the handling code as it is currently used
> > > for kgdb (unless you can use two instructions for filling in which could
> > > generate a NULL pointer access).
> > 
> > The trade-off would be that if we align on 8, it would certainly increase
> > the probability to jump to the right offset. Note, on x86_64 we have no
> > alignment requirements, hence 1, and on s390x only alignment of 2.
> > 
> > So, on that few (?) boards where UND would be a valid instruction [ as
> > opposed to crash the kernel ], would it translate into a NOP and just
> > 'walk' from there into the JIT image?
> 
> On current ARMv8 CPU implementations, the above constant is unallocated
> in the A64 instruction space. But you never know, it may be allocated in
> the future.
> 
> I think it's easier if you just use something like BRK #0x100 (opcode
> 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> 0x400 and 0x401).
> 
> An unallocated BRK would trigger a fault via do_debug_exception ->
> brk_handler and panic the kernel.

Sounds sensible.

Will

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 16:46       ` Catalin Marinas
@ 2014-09-12 17:16         ` Daniel Borkmann
  -1 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 17:16 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On 09/12/2014 06:46 PM, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
>> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
>>> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>>>>    Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>>>>    arm64-next tree will get both merged into Linus' tree, we will run into
>>>>    a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>>>>    and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>>>>    assume nobody actually _runs_ linux-next, but not sure about that though.
>>>
>>> Some people do.
>>>
>>>>    How do we handle this? Would I need to resend this patch when the time
>>>>    comes or would you ARM64 guys take care of it automagically? ;)
>>>
>>> I think we could disable BPF for arm64 until -rc1 and re-enable it
>>> together with this patch.
>>
>> Ok, yes, that would mitigate it a bit. Sounds fine to me.
>>
>>> One comment below:
>>>
>>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> [...]
>>>> +static void jit_fill_hole(void *area, unsigned int size)
>>>> +{
>>>> +	/* Insert illegal UND instructions. */
>>>> +	u32 *ptr, fill_ins = 0xe7ffffff;
>>>
>>> On arm64 we don't have a guaranteed undefined instruction space (and
>>> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
>>> valid instruction, it seems that you used the same value).
>>
>> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
>
> So, if I try this:
>
> $ echo 0xffffffe7 | xxd -r > test.bin
> $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> ...
>     0:   e7ffffff        udf     #65535  ; 0xffff
>
> Do you use the same constant on arm32?

I was using something along that lines, but I guess I missed
something:

# cat foo.S
.globl foobar
foobar:
.word 0xe7ffffff
# cat bar.c
#include <stdio.h>
extern void foobar(void);
int main(void)
{
     foobar();
     printf("!ok\n");
     return 0;
}
# as foo.S -o foo.o
# gcc bar.c foo.o -o bar
# ./bar
Illegal instruction

>>> I think the only guaranteed way is to use the BRK #imm instruction but
>>> it requires some changes to the handling code as it is currently used
>>> for kgdb (unless you can use two instructions for filling in which could
>>> generate a NULL pointer access).
>>
>> The trade-off would be that if we align on 8, it would certainly increase
>> the probability to jump to the right offset. Note, on x86_64 we have no
>> alignment requirements, hence 1, and on s390x only alignment of 2.
>>
>> So, on that few (?) boards where UND would be a valid instruction [ as
>> opposed to crash the kernel ], would it translate into a NOP and just
>> 'walk' from there into the JIT image?
>
> On current ARMv8 CPU implementations, the above constant is unallocated
> in the A64 instruction space. But you never know, it may be allocated in
> the future.
>
> I think it's easier if you just use something like BRK #0x100 (opcode
> 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> 0x400 and 0x401).
>
> An unallocated BRK would trigger a fault via do_debug_exception ->
> brk_handler and panic the kernel.

Okay, that's fine by me, I'll just update s/0xe7ffffff/0xd4202000/.

Do you want me to use the same suggestion for arm32 as well as it
would be less fragile?

Last but not least ;), if I would resend it today, would you queue
it for later on, or do you want to handle it differently?

Thanks again,
Daniel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 17:16         ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 17:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/12/2014 06:46 PM, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
>> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
>>> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
>>>>    Will, Catalin, Dave, this is more or less a heads-up: when net-next and
>>>>    arm64-next tree will get both merged into Linus' tree, we will run into
>>>>    a 'silent' merge conflict until someone actually runs eBPF JIT on ARM64
>>>>    and might notice (I presume) an oops when JIT is freeing bpf_prog. I'd
>>>>    assume nobody actually _runs_ linux-next, but not sure about that though.
>>>
>>> Some people do.
>>>
>>>>    How do we handle this? Would I need to resend this patch when the time
>>>>    comes or would you ARM64 guys take care of it automagically? ;)
>>>
>>> I think we could disable BPF for arm64 until -rc1 and re-enable it
>>> together with this patch.
>>
>> Ok, yes, that would mitigate it a bit. Sounds fine to me.
>>
>>> One comment below:
>>>
>>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> [...]
>>>> +static void jit_fill_hole(void *area, unsigned int size)
>>>> +{
>>>> +	/* Insert illegal UND instructions. */
>>>> +	u32 *ptr, fill_ins = 0xe7ffffff;
>>>
>>> On arm64 we don't have a guaranteed undefined instruction space (and
>>> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
>>> valid instruction, it seems that you used the same value).
>>
>> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
>
> So, if I try this:
>
> $ echo 0xffffffe7 | xxd -r > test.bin
> $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> ...
>     0:   e7ffffff        udf     #65535  ; 0xffff
>
> Do you use the same constant on arm32?

I was using something along that lines, but I guess I missed
something:

# cat foo.S
.globl foobar
foobar:
.word 0xe7ffffff
# cat bar.c
#include <stdio.h>
extern void foobar(void);
int main(void)
{
     foobar();
     printf("!ok\n");
     return 0;
}
# as foo.S -o foo.o
# gcc bar.c foo.o -o bar
# ./bar
Illegal instruction

>>> I think the only guaranteed way is to use the BRK #imm instruction but
>>> it requires some changes to the handling code as it is currently used
>>> for kgdb (unless you can use two instructions for filling in which could
>>> generate a NULL pointer access).
>>
>> The trade-off would be that if we align on 8, it would certainly increase
>> the probability to jump to the right offset. Note, on x86_64 we have no
>> alignment requirements, hence 1, and on s390x only alignment of 2.
>>
>> So, on that few (?) boards where UND would be a valid instruction [ as
>> opposed to crash the kernel ], would it translate into a NOP and just
>> 'walk' from there into the JIT image?
>
> On current ARMv8 CPU implementations, the above constant is unallocated
> in the A64 instruction space. But you never know, it may be allocated in
> the future.
>
> I think it's easier if you just use something like BRK #0x100 (opcode
> 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> 0x400 and 0x401).
>
> An unallocated BRK would trigger a fault via do_debug_exception ->
> brk_handler and panic the kernel.

Okay, that's fine by me, I'll just update s/0xe7ffffff/0xd4202000/.

Do you want me to use the same suggestion for arm32 as well as it
would be less fragile?

Last but not least ;), if I would resend it today, would you queue
it for later on, or do you want to handle it differently?

Thanks again,
Daniel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 17:16         ` Daniel Borkmann
@ 2014-09-12 17:21           ` Catalin Marinas
  -1 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 17:21 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On Fri, Sep 12, 2014 at 06:16:38PM +0100, Daniel Borkmann wrote:
> On 09/12/2014 06:46 PM, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> >> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> >>> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> >>>> --- a/arch/arm64/net/bpf_jit_comp.c
> >>>> +++ b/arch/arm64/net/bpf_jit_comp.c
> >>> [...]
> >>>> +static void jit_fill_hole(void *area, unsigned int size)
> >>>> +{
> >>>> +	/* Insert illegal UND instructions. */
> >>>> +	u32 *ptr, fill_ins = 0xe7ffffff;
> >>>
> >>> On arm64 we don't have a guaranteed undefined instruction space (and
> >>> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> >>> valid instruction, it seems that you used the same value).
> >>
> >> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
> >
> > So, if I try this:
> >
> > $ echo 0xffffffe7 | xxd -r > test.bin
> > $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> > ...
> >     0:   e7ffffff        udf     #65535  ; 0xffff
> >
> > Do you use the same constant on arm32?
> 
> I was using something along that lines, but I guess I missed
> something:
> 
> # cat foo.S
> .globl foobar
> foobar:
> .word 0xe7ffffff

That's missing a mov pc, lr.

> # cat bar.c
> #include <stdio.h>
> extern void foobar(void);
> int main(void)
> {
>      foobar();

So you call it here and ends up in some data section, possibly hitting
some undefined instruction. For ARM it is undefined, for Thumb-2 it is
not as Will pointed out.

> >>> I think the only guaranteed way is to use the BRK #imm instruction but
> >>> it requires some changes to the handling code as it is currently used
> >>> for kgdb (unless you can use two instructions for filling in which could
> >>> generate a NULL pointer access).
> >>
> >> The trade-off would be that if we align on 8, it would certainly increase
> >> the probability to jump to the right offset. Note, on x86_64 we have no
> >> alignment requirements, hence 1, and on s390x only alignment of 2.
> >>
> >> So, on that few (?) boards where UND would be a valid instruction [ as
> >> opposed to crash the kernel ], would it translate into a NOP and just
> >> 'walk' from there into the JIT image?
> >
> > On current ARMv8 CPU implementations, the above constant is unallocated
> > in the A64 instruction space. But you never know, it may be allocated in
> > the future.
> >
> > I think it's easier if you just use something like BRK #0x100 (opcode
> > 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> > 0x400 and 0x401).
> >
> > An unallocated BRK would trigger a fault via do_debug_exception ->
> > brk_handler and panic the kernel.
> 
> Okay, that's fine by me, I'll just update s/0xe7ffffff/0xd4202000/.
> 
> Do you want me to use the same suggestion for arm32 as well as it
> would be less fragile?

We don't have a brk instruction for arm32 but we have guaranteed
undefined space. Have a look at the kgdb support for example (or grep
for register_undef_hook under arch/arm) to get an idea.

> Last but not least ;), if I would resend it today, would you queue
> it for later on, or do you want to handle it differently?

You can send it now, it will be pushed upstream at the right time.

Thanks.

-- 
Catalin


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 17:21           ` Catalin Marinas
  0 siblings, 0 replies; 16+ messages in thread
From: Catalin Marinas @ 2014-09-12 17:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 12, 2014 at 06:16:38PM +0100, Daniel Borkmann wrote:
> On 09/12/2014 06:46 PM, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 05:21:27PM +0100, Daniel Borkmann wrote:
> >> On 09/12/2014 06:03 PM, Catalin Marinas wrote:
> >>> On Fri, Sep 12, 2014 at 08:11:37AM +0100, Daniel Borkmann wrote:
> >>>> --- a/arch/arm64/net/bpf_jit_comp.c
> >>>> +++ b/arch/arm64/net/bpf_jit_comp.c
> >>> [...]
> >>>> +static void jit_fill_hole(void *area, unsigned int size)
> >>>> +{
> >>>> +	/* Insert illegal UND instructions. */
> >>>> +	u32 *ptr, fill_ins = 0xe7ffffff;
> >>>
> >>> On arm64 we don't have a guaranteed undefined instruction space (and
> >>> Will tells me that on Thumb-2 for the 32-bit arm port it actually is a
> >>> valid instruction, it seems that you used the same value).
> >>
> >> Hm, ok, the boards we've tried out and where Zi tested it too, it worked.
> >
> > So, if I try this:
> >
> > $ echo 0xffffffe7 | xxd -r > test.bin
> > $ arm-linux-gnueabihf-objdump -m arm -D -b binary test.bin
> > ...
> >     0:   e7ffffff        udf     #65535  ; 0xffff
> >
> > Do you use the same constant on arm32?
> 
> I was using something along that lines, but I guess I missed
> something:
> 
> # cat foo.S
> .globl foobar
> foobar:
> .word 0xe7ffffff

That's missing a mov pc, lr.

> # cat bar.c
> #include <stdio.h>
> extern void foobar(void);
> int main(void)
> {
>      foobar();

So you call it here and ends up in some data section, possibly hitting
some undefined instruction. For ARM it is undefined, for Thumb-2 it is
not as Will pointed out.

> >>> I think the only guaranteed way is to use the BRK #imm instruction but
> >>> it requires some changes to the handling code as it is currently used
> >>> for kgdb (unless you can use two instructions for filling in which could
> >>> generate a NULL pointer access).
> >>
> >> The trade-off would be that if we align on 8, it would certainly increase
> >> the probability to jump to the right offset. Note, on x86_64 we have no
> >> alignment requirements, hence 1, and on s390x only alignment of 2.
> >>
> >> So, on that few (?) boards where UND would be a valid instruction [ as
> >> opposed to crash the kernel ], would it translate into a NOP and just
> >> 'walk' from there into the JIT image?
> >
> > On current ARMv8 CPU implementations, the above constant is unallocated
> > in the A64 instruction space. But you never know, it may be allocated in
> > the future.
> >
> > I think it's easier if you just use something like BRK #0x100 (opcode
> > 0xd4202000) which would trigger a fault in the kernel (kgdb uses #imm
> > 0x400 and 0x401).
> >
> > An unallocated BRK would trigger a fault via do_debug_exception ->
> > brk_handler and panic the kernel.
> 
> Okay, that's fine by me, I'll just update s/0xe7ffffff/0xd4202000/.
> 
> Do you want me to use the same suggestion for arm32 as well as it
> would be less fragile?

We don't have a brk instruction for arm32 but we have guaranteed
undefined space. Have a look at the kgdb support for example (or grep
for register_undef_hook under arch/arm) to get an idea.

> Last but not least ;), if I would resend it today, would you queue
> it for later on, or do you want to handle it differently?

You can send it now, it will be pushed upstream at the right time.

Thanks.

-- 
Catalin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
  2014-09-12 17:21           ` Catalin Marinas
@ 2014-09-12 17:39             ` Daniel Borkmann
  -1 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 17:39 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, davem, zlim.lnx, ast, linux-arm-kernel, linux-kernel

On 09/12/2014 07:21 PM, Catalin Marinas wrote:
...
> We don't have a brk instruction for arm32 but we have guaranteed
> undefined space. Have a look at the kgdb support for example (or grep
> for register_undef_hook under arch/arm) to get an idea.

Will do, thanks!

>> Last but not least ;), if I would resend it today, would you queue
>> it for later on, or do you want to handle it differently?
>
> You can send it now, it will be pushed upstream at the right time.

Just did, thanks a lot for your help!

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code
@ 2014-09-12 17:39             ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2014-09-12 17:39 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/12/2014 07:21 PM, Catalin Marinas wrote:
...
> We don't have a brk instruction for arm32 but we have guaranteed
> undefined space. Have a look at the kgdb support for example (or grep
> for register_undef_hook under arch/arm) to get an idea.

Will do, thanks!

>> Last but not least ;), if I would resend it today, would you queue
>> it for later on, or do you want to handle it differently?
>
> You can send it now, it will be pushed upstream at the right time.

Just did, thanks a lot for your help!

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2014-09-12 17:40 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-12  7:11 [PATCH arm64-next] net: bpf: arm64: address randomize and write protect JIT code Daniel Borkmann
2014-09-12  7:11 ` Daniel Borkmann
2014-09-12 16:03 ` Catalin Marinas
2014-09-12 16:03   ` Catalin Marinas
2014-09-12 16:21   ` Daniel Borkmann
2014-09-12 16:21     ` Daniel Borkmann
2014-09-12 16:46     ` Catalin Marinas
2014-09-12 16:46       ` Catalin Marinas
2014-09-12 17:10       ` Will Deacon
2014-09-12 17:10         ` Will Deacon
2014-09-12 17:16       ` Daniel Borkmann
2014-09-12 17:16         ` Daniel Borkmann
2014-09-12 17:21         ` Catalin Marinas
2014-09-12 17:21           ` Catalin Marinas
2014-09-12 17:39           ` Daniel Borkmann
2014-09-12 17:39             ` Daniel Borkmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.