diff mbox

arm64: bpf: fix buffer pointer

Message ID 1447836962-4086-1-git-send-email-zlim.lnx@gmail.com
State Accepted
Commit f4b16fce7a5a1ec8069b1f577476bdc1d2688cd1
Headers show

Commit Message

Zi Shen Lim Nov. 18, 2015, 8:56 a.m. UTC 
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(-)

-- 
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/

Comments

David Miller Nov. 18, 2015, 8:34 p.m. UTC | #1 
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/
Yang Shi Nov. 18, 2015, 9:07 p.m. UTC | #2 
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/
Zi Shen Lim Nov. 18, 2015, 9:41 p.m. UTC | #3 
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/
Yang Shi Nov. 18, 2015, 10:59 p.m. UTC | #4 
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/
David Miller Nov. 19, 2015, 3:38 a.m. UTC | #5 
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/
diff mbox

Patch

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);