lkml.org 
[lkml]   [2015]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] arm64: bpf: fix buffer pointer
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);
>



\
 
 \ /
  Last update: 2015-11-18 22:21    [W:0.090 / U:0.768 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site