Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1271977 > unrolled thread

[PATCH] arm64: bpf: fix buffer pointer

Started byZi Shen Lim <zlim.lnx@gmail.com>
First post2015-11-18 10:00 +0100
Last post2015-11-19 04:40 +0100
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] arm64: bpf: fix buffer pointer Zi Shen Lim <zlim.lnx@gmail.com> - 2015-11-18 10:00 +0100
    Re: [PATCH] arm64: bpf: fix buffer pointer David Miller <davem@davemloft.net> - 2015-11-18 21:40 +0100
    Re: [PATCH] arm64: bpf: fix buffer pointer "Shi, Yang" <yang.shi@linaro.org> - 2015-11-18 22:10 +0100
      Re: [PATCH] arm64: bpf: fix buffer pointer Z Lim <zlim.lnx@gmail.com> - 2015-11-18 22:50 +0100
        Re: [PATCH] arm64: bpf: fix buffer pointer "Shi, Yang" <yang.shi@linaro.org> - 2015-11-19 00:00 +0100
    Re: [PATCH] arm64: bpf: fix buffer pointer David Miller <davem@davemloft.net> - 2015-11-19 04:40 +0100

#1271977 — [PATCH] arm64: bpf: fix buffer pointer

FromZi Shen Lim <zlim.lnx@gmail.com>
Date2015-11-18 10:00 +0100
Subject[PATCH] arm64: bpf: fix buffer pointer
Message-ID<qw6KU-Af-53@gated-at.bofh.it>
During code review, I noticed we were passing a bad buffer pointer
to bpf_load_pointer helper function called by jitted code.

Point to the buffer allocated by JIT, so we don't silently corrupt
other parts of the stack.

Signed-off-by: Zi Shen Lim <zlim.lnx@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index d6a53ef..7cf032b 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -139,6 +139,12 @@ static inline int epilogue_offset(const struct jit_ctx *ctx)
 /* Stack must be multiples of 16B */
 #define STACK_ALIGN(sz) (((sz) + 15) & ~15)
 
+#define _STACK_SIZE \
+	(MAX_BPF_STACK \
+	 + 4 /* extra for skb_copy_bits buffer */)
+
+#define STACK_SIZE STACK_ALIGN(_STACK_SIZE)
+
 static void build_prologue(struct jit_ctx *ctx)
 {
 	const u8 r6 = bpf2a64[BPF_REG_6];
@@ -150,10 +156,6 @@ static void build_prologue(struct jit_ctx *ctx)
 	const u8 rx = bpf2a64[BPF_REG_X];
 	const u8 tmp1 = bpf2a64[TMP_REG_1];
 	const u8 tmp2 = bpf2a64[TMP_REG_2];
-	int stack_size = MAX_BPF_STACK;
-
-	stack_size += 4; /* extra for skb_copy_bits buffer */
-	stack_size = STACK_ALIGN(stack_size);
 
 	/*
 	 * BPF prog stack layout
@@ -165,12 +167,13 @@ static void build_prologue(struct jit_ctx *ctx)
 	 *                        | ... | callee saved registers
 	 *                        +-----+
 	 *                        |     | x25/x26
-	 * BPF fp register => -80:+-----+
+	 * BPF fp register => -80:+-----+ <= (BPF_FP)
 	 *                        |     |
 	 *                        | ... | BPF prog stack
 	 *                        |     |
-	 *                        |     |
-	 * current A64_SP =>      +-----+
+	 *                        +-----+ <= (BPF_FP - MAX_BPF_STACK)
+	 *                        |RSVD | JIT scratchpad
+	 * current A64_SP =>      +-----+ <= (BPF_FP - STACK_SIZE)
 	 *                        |     |
 	 *                        | ... | Function call stack
 	 *                        |     |
@@ -196,7 +199,7 @@ static void build_prologue(struct jit_ctx *ctx)
 	emit(A64_MOV(1, fp, A64_SP), ctx);
 
 	/* Set up function call stack */
-	emit(A64_SUB_I(1, A64_SP, A64_SP, stack_size), ctx);
+	emit(A64_SUB_I(1, A64_SP, A64_SP, STACK_SIZE), ctx);
 
 	/* Clear registers A and X */
 	emit_a64_mov_i64(ra, 0, ctx);
@@ -213,13 +216,9 @@ static void build_epilogue(struct jit_ctx *ctx)
 	const u8 fp = bpf2a64[BPF_REG_FP];
 	const u8 tmp1 = bpf2a64[TMP_REG_1];
 	const u8 tmp2 = bpf2a64[TMP_REG_2];
-	int stack_size = MAX_BPF_STACK;
-
-	stack_size += 4; /* extra for skb_copy_bits buffer */
-	stack_size = STACK_ALIGN(stack_size);
 
 	/* We're done with BPF stack */
-	emit(A64_ADD_I(1, A64_SP, A64_SP, stack_size), ctx);
+	emit(A64_ADD_I(1, A64_SP, A64_SP, STACK_SIZE), ctx);
 
 	/* Restore fs (x25) and x26 */
 	emit(A64_POP(fp, A64_R(26), A64_SP), ctx);
@@ -658,7 +657,7 @@ emit_cond_jmp:
 			return -EINVAL;
 		}
 		emit_a64_mov_i64(r3, size, ctx);
-		emit(A64_ADD_I(1, r4, fp, MAX_BPF_STACK), ctx);
+		emit(A64_SUB_I(1, r4, fp, STACK_SIZE), ctx);
 		emit_a64_mov_i64(r5, (unsigned long)bpf_load_pointer, ctx);
 		emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);
 		emit(A64_MOV(1, A64_FP, A64_SP), ctx);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1272547

FromDavid Miller <davem@davemloft.net>
Date2015-11-18 21:40 +0100
Message-ID<qwhGi-7Ze-9@gated-at.bofh.it>
In reply to#1271977
From: Zi Shen Lim <zlim.lnx@gmail.com>
Date: Wed, 18 Nov 2015 00:56:02 -0800

> During code review, I noticed we were passing a bad buffer pointer
> to bpf_load_pointer helper function called by jitted code.
> 
> Point to the buffer allocated by JIT, so we don't silently corrupt
> other parts of the stack.
> 
> Signed-off-by: Zi Shen Lim <zlim.lnx@gmail.com>

Can I get some review from other ARM folks before I apply this?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272580

From"Shi, Yang" <yang.shi@linaro.org>
Date2015-11-18 22:10 +0100
Message-ID<qwi9k-8vZ-31@gated-at.bofh.it>
In reply to#1271977
On 11/18/2015 12:56 AM, Zi Shen Lim wrote:
> During code review, I noticed we were passing a bad buffer pointer
> to bpf_load_pointer helper function called by jitted code.
>
> Point to the buffer allocated by JIT, so we don't silently corrupt
> other parts of the stack.
>
> Signed-off-by: Zi Shen Lim <zlim.lnx@gmail.com>
> ---
>   arch/arm64/net/bpf_jit_comp.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index d6a53ef..7cf032b 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -139,6 +139,12 @@ static inline int epilogue_offset(const struct jit_ctx *ctx)
>   /* Stack must be multiples of 16B */
>   #define STACK_ALIGN(sz) (((sz) + 15) & ~15)
>
> +#define _STACK_SIZE \
> +	(MAX_BPF_STACK \
> +	 + 4 /* extra for skb_copy_bits buffer */)
> +
> +#define STACK_SIZE STACK_ALIGN(_STACK_SIZE)
> +
>   static void build_prologue(struct jit_ctx *ctx)
>   {
>   	const u8 r6 = bpf2a64[BPF_REG_6];
> @@ -150,10 +156,6 @@ static void build_prologue(struct jit_ctx *ctx)
>   	const u8 rx = bpf2a64[BPF_REG_X];
>   	const u8 tmp1 = bpf2a64[TMP_REG_1];
>   	const u8 tmp2 = bpf2a64[TMP_REG_2];
> -	int stack_size = MAX_BPF_STACK;
> -
> -	stack_size += 4; /* extra for skb_copy_bits buffer */
> -	stack_size = STACK_ALIGN(stack_size);
>
>   	/*
>   	 * BPF prog stack layout
> @@ -165,12 +167,13 @@ static void build_prologue(struct jit_ctx *ctx)
>   	 *                        | ... | callee saved registers
>   	 *                        +-----+
>   	 *                        |     | x25/x26
> -	 * BPF fp register => -80:+-----+
> +	 * BPF fp register => -80:+-----+ <= (BPF_FP)
>   	 *                        |     |
>   	 *                        | ... | BPF prog stack
>   	 *                        |     |
> -	 *                        |     |
> -	 * current A64_SP =>      +-----+
> +	 *                        +-----+ <= (BPF_FP - MAX_BPF_STACK)
> +	 *                        |RSVD | JIT scratchpad
> +	 * current A64_SP =>      +-----+ <= (BPF_FP - STACK_SIZE)
>   	 *                        |     |
>   	 *                        | ... | Function call stack
>   	 *                        |     |
> @@ -196,7 +199,7 @@ static void build_prologue(struct jit_ctx *ctx)
>   	emit(A64_MOV(1, fp, A64_SP), ctx);
>
>   	/* Set up function call stack */
> -	emit(A64_SUB_I(1, A64_SP, A64_SP, stack_size), ctx);
> +	emit(A64_SUB_I(1, A64_SP, A64_SP, STACK_SIZE), ctx);
>
>   	/* Clear registers A and X */
>   	emit_a64_mov_i64(ra, 0, ctx);
> @@ -213,13 +216,9 @@ static void build_epilogue(struct jit_ctx *ctx)
>   	const u8 fp = bpf2a64[BPF_REG_FP];
>   	const u8 tmp1 = bpf2a64[TMP_REG_1];
>   	const u8 tmp2 = bpf2a64[TMP_REG_2];
> -	int stack_size = MAX_BPF_STACK;
> -
> -	stack_size += 4; /* extra for skb_copy_bits buffer */
> -	stack_size = STACK_ALIGN(stack_size);
>
>   	/* We're done with BPF stack */
> -	emit(A64_ADD_I(1, A64_SP, A64_SP, stack_size), ctx);
> +	emit(A64_ADD_I(1, A64_SP, A64_SP, STACK_SIZE), ctx);
>
>   	/* Restore fs (x25) and x26 */
>   	emit(A64_POP(fp, A64_R(26), A64_SP), ctx);
> @@ -658,7 +657,7 @@ emit_cond_jmp:
>   			return -EINVAL;
>   		}
>   		emit_a64_mov_i64(r3, size, ctx);
> -		emit(A64_ADD_I(1, r4, fp, MAX_BPF_STACK), ctx);
> +		emit(A64_SUB_I(1, r4, fp, STACK_SIZE), ctx);

Should not it sub MAX_BPF_STACK?

If you sub STACK_SIZE here, the buffer pointer will point to bottom of 
the reserved area.

You stack layout change also shows this:

+	 *                        +-----+ <= (BPF_FP - MAX_BPF_STACK)
+	 *                        |RSVD | JIT scratchpad
+	 * current A64_SP =>      +-----+ <= (BPF_FP - STACK_SIZE)

Thanks,
Yang


>   		emit_a64_mov_i64(r5, (unsigned long)bpf_load_pointer, ctx);
>   		emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);
>   		emit(A64_MOV(1, A64_FP, A64_SP), ctx);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272604

FromZ Lim <zlim.lnx@gmail.com>
Date2015-11-18 22:50 +0100
Message-ID<qwiM1-iJ-7@gated-at.bofh.it>
In reply to#1272580
On Wed, Nov 18, 2015 at 1:07 PM, Shi, Yang <yang.shi@linaro.org> wrote:
> On 11/18/2015 12:56 AM, Zi Shen Lim wrote:
>>                 emit_a64_mov_i64(r3, size, ctx);
>> -               emit(A64_ADD_I(1, r4, fp, MAX_BPF_STACK), ctx);
>> +               emit(A64_SUB_I(1, r4, fp, STACK_SIZE), ctx);
>
>
> Should not it sub MAX_BPF_STACK?

No, if it's at (BPF_FP - MAX_BPF_STACK), we'll be writing into the BPF
stack area, which should only be used by the BPF program.

> If you sub STACK_SIZE here, the buffer pointer will point to bottom of the
> reserved area.

Yes, that's the idea. The buffer is allocated in here. Right now we're
using this "reserved" space for this buffer only.

>
> You stack layout change also shows this:
>
> +        *                        +-----+ <= (BPF_FP - MAX_BPF_STACK)
> +        *                        |RSVD | JIT scratchpad
> +        * current A64_SP =>      +-----+ <= (BPF_FP - STACK_SIZE)

Yes, this diagram reflects the code and intention.


Thanks for reviewing, we definitely need more of these :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272655

From"Shi, Yang" <yang.shi@linaro.org>
Date2015-11-19 00:00 +0100
Message-ID<qwjRM-YV-7@gated-at.bofh.it>
In reply to#1272604
On 11/18/2015 1:41 PM, Z Lim wrote:
> On Wed, Nov 18, 2015 at 1:07 PM, Shi, Yang <yang.shi@linaro.org> wrote:
>> On 11/18/2015 12:56 AM, Zi Shen Lim wrote:
>>>                  emit_a64_mov_i64(r3, size, ctx);
>>> -               emit(A64_ADD_I(1, r4, fp, MAX_BPF_STACK), ctx);
>>> +               emit(A64_SUB_I(1, r4, fp, STACK_SIZE), ctx);
>>
>>
>> Should not it sub MAX_BPF_STACK?
>
> No, if it's at (BPF_FP - MAX_BPF_STACK), we'll be writing into the BPF
> stack area, which should only be used by the BPF program.
>
>> If you sub STACK_SIZE here, the buffer pointer will point to bottom of the
>> reserved area.
>
> Yes, that's the idea. The buffer is allocated in here. Right now we're
> using this "reserved" space for this buffer only.

OK, I see. The buffer grows from low to high.

Thanks for the elaboration.

Acked-by: Yang Shi <yang.shi@linaro.org>

Yang

>
>>
>> You stack layout change also shows this:
>>
>> +        *                        +-----+ <= (BPF_FP - MAX_BPF_STACK)
>> +        *                        |RSVD | JIT scratchpad
>> +        * current A64_SP =>      +-----+ <= (BPF_FP - STACK_SIZE)
>
> Yes, this diagram reflects the code and intention.
>
>
> Thanks for reviewing, we definitely need more of these :)
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272798

FromDavid Miller <davem@davemloft.net>
Date2015-11-19 04:40 +0100
Message-ID<qwoeL-3V4-43@gated-at.bofh.it>
In reply to#1271977
From: Zi Shen Lim <zlim.lnx@gmail.com>
Date: Wed, 18 Nov 2015 00:56:02 -0800

> During code review, I noticed we were passing a bad buffer pointer
> to bpf_load_pointer helper function called by jitted code.
> 
> Point to the buffer allocated by JIT, so we don't silently corrupt
> other parts of the stack.
> 
> Signed-off-by: Zi Shen Lim <zlim.lnx@gmail.com>

Applied, thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web