| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 07 Apr 2012 00:28:09 +0200
From: Mircea Gherzan <mgherzan@...il.com>
To: Jan Seiffert <kaffeemonster@...glemail.com>
CC: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Eric Dumazet <eric.dumazet@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Russell King <linux@....linux.org.uk>
Subject: Re: [REGRESSION][PATCH v1] bpf jit: Let the arm jit handle negative
memory references
Hi,
Am 06.04.2012 20:57, schrieb Jan Seiffert:
> The arm jit has the same problem as the other two jits.
> It only tests for negative absolute memory references, and if it sees
> one bails out to let the interpreter handle the filter program.
>
> But this fails if the X register contains a negative memory reference
> and is used for an indirect load.
I don't think that there's any use case for negative values in X. In
both the original BPF design and in the LSF interpreter, A and X are
considered unsigned. The role of X is mainly to allow variable length
headers (load the length -> unsigned).
"Negative" K values are permitted for ancillary data loads based on the
fact that we're not going to see 2GB packets any time soon.
Mircea
> This is only caught at runtime, where the filter will always drop
> the packet.
> Filter which work fine with the interpreter do not work with the jit.
>
> So this patch tries to fix it for the arm jit.
>
> First we add the prototype of bpf_internal_load_pointer_neg_helper to
> filter.h, since the arm jit has no assembler component, instead it has
> to be used from C.
>
> Then the helper functions are prepared to either handle known positive
> offsets, known negative offsets, or any offset and test at runtime.
>
> Finally the compiler is modified to emit calls to the right function,
> depending if either the offset is known, or not.
>
> This fixes the case of a negative X register and allows to lift
> the restriction that bpf programs with negative offsets can't
> be jited.
>
> Signed-off-by: Jan Seiffert <kaffeemonster@...glemail.com>
> ---
> The arm jit structure is a little bit different then the other jits, esp.
> it does not use assembler load helper. So i had to put the prototype for
> bpf_internal_load_pointer_neg_helper somewhere.
>
> This is a v1 to keep the ball rolling.
>
> Testing would also be cool, -ENOHARDWARE.
>
> arch/arm/net/bpf_jit_32.c | 149 ++++++++++++++++++++++++++++++++-------------
> include/linux/filter.h | 6 ++
> net/core/filter.c | 4 +
> 3 files changed, 117 insertions(+), 42 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index 62135849..19d60af 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -18,6 +18,7 @@
> #include <linux/slab.h>
> #include <asm/cacheflush.h>
> #include <asm/hwcap.h>
> +#include <asm/unaligned.h>
>
> #include "bpf_jit_32.h"
>
> @@ -71,7 +72,7 @@ struct jit_ctx {
>
> int bpf_jit_enable __read_mostly;
>
> -static u64 jit_get_skb_b(struct sk_buff *skb, unsigned offset)
> +static u64 jit_get_skb_b_pos(struct sk_buff *skb, unsigned offset)
> {
> u8 ret;
> int err;
> @@ -81,7 +82,7 @@ static u64 jit_get_skb_b(struct sk_buff *skb, unsigned offset)
> return (u64)err << 32 | ret;
> }
>
> -static u64 jit_get_skb_h(struct sk_buff *skb, unsigned offset)
> +static u64 jit_get_skb_h_pos(struct sk_buff *skb, unsigned offset)
> {
> u16 ret;
> int err;
> @@ -91,7 +92,7 @@ static u64 jit_get_skb_h(struct sk_buff *skb, unsigned offset)
> return (u64)err << 32 | ntohs(ret);
> }
>
> -static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
> +static u64 jit_get_skb_w_pos(struct sk_buff *skb, unsigned offset)
> {
> u32 ret;
> int err;
> @@ -101,6 +102,60 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
> return (u64)err << 32 | ntohl(ret);
> }
>
> +static u64 jit_get_skb_b_neg(struct sk_buff *skb, unsigned offset)
> +{
> + u8 *ptr;
> +
> + ptr = bpf_internal_load_pointer_neg_helper(skb, offset, 1);
> + if (!ptr)
> + return (u64)1 << 32;
> + return *ptr;
> +}
> +
> +static u64 jit_get_skb_h_neg(struct sk_buff *skb, unsigned offset)
> +{
> + u16 *ptr;
> +
> + ptr = bpf_internal_load_pointer_neg_helper(skb, offset, 2);
> + if (!ptr)
> + return (u64)1 << 32;
> + return get_unaligned_be16(ptr);
> +}
> +
> +static u64 jit_get_skb_w_neg(struct sk_buff *skb, unsigned offset)
> +{
> + u32 *ptr;
> +
> + ptr = bpf_internal_load_pointer_neg_helper(skb, offset, 4);
> + if (!ptr)
> + return (u64)1 << 32;
> + return get_unaligned_be32(ptr);
> +}
> +
> +static u64 jit_get_skb_b_any(struct sk_buff *skb, unsigned offset)
> +{
> + if ((int)offset >= 0)
> + return jit_get_skb_b_pos(skb, offset);
> + else
> + return jit_get_skb_b_neg(skb, offset);
> +}
> +
> +static u64 jit_get_skb_h_any(struct sk_buff *skb, unsigned offset)
> +{
> + if ((int)offset >= 0)
> + return jit_get_skb_h_pos(skb, offset);
> + else
> + return jit_get_skb_h_neg(skb, offset);
> +}
> +
> +static u64 jit_get_skb_w_any(struct sk_buff *skb, unsigned offset)
> +{
> + if ((int)offset >= 0)
> + return jit_get_skb_w_pos(skb, offset);
> + else
> + return jit_get_skb_w_neg(skb, offset);
> +}
> +
> /*
> * Wrapper that handles both OABI and EABI and assures Thumb2 interworking
> * (where the assembly routines like __aeabi_uidiv could cause problems).
> @@ -458,7 +513,10 @@ static inline void update_on_xread(struct jit_ctx *ctx)
>
> static int build_body(struct jit_ctx *ctx)
> {
> - void *load_func[] = {jit_get_skb_b, jit_get_skb_h, jit_get_skb_w};
> + void *load_func_any[] = {jit_get_skb_b_any, jit_get_skb_h_any, jit_get_skb_w_any};
> + void *load_func_pos[] = {jit_get_skb_b_pos, jit_get_skb_h_pos, jit_get_skb_w_pos};
> + void *load_func_neg[] = {jit_get_skb_b_neg, jit_get_skb_h_neg, jit_get_skb_w_neg};
> + void **load_func;
> const struct sk_filter *prog = ctx->skf;
> const struct sock_filter *inst;
> unsigned i, load_order, off, condt;
> @@ -498,36 +556,38 @@ static int build_body(struct jit_ctx *ctx)
> case BPF_S_LD_B_ABS:
> load_order = 0;
> load:
> - /* the interpreter will deal with the negative K */
> - if ((int)k < 0)
> - return -ENOTSUPP;
> emit_mov_i(r_off, k, ctx);
> -load_common:
> - ctx->seen |= SEEN_DATA | SEEN_CALL;
> -
> - if (load_order > 0) {
> - emit(ARM_SUB_I(r_scratch, r_skb_hl,
> - 1 << load_order), ctx);
> - emit(ARM_CMP_R(r_scratch, r_off), ctx);
> - condt = ARM_COND_HS;
> - } else {
> - emit(ARM_CMP_R(r_skb_hl, r_off), ctx);
> - condt = ARM_COND_HI;
> - }
>
> - _emit(condt, ARM_ADD_R(r_scratch, r_off, r_skb_data),
> - ctx);
> -
> - if (load_order == 0)
> - _emit(condt, ARM_LDRB_I(r_A, r_scratch, 0),
> + /* deal with negative K */
> + if (k >= 0) {
> + load_func = load_func_pos;
> + if (load_order > 0) {
> + emit(ARM_SUB_I(r_scratch, r_skb_hl,
> + 1 << load_order), ctx);
> + emit(ARM_CMP_R(r_scratch, r_off), ctx);
> + condt = ARM_COND_HS;
> + } else {
> + emit(ARM_CMP_R(r_skb_hl, r_off), ctx);
> + condt = ARM_COND_HI;
> + }
> +
> + _emit(condt, ARM_ADD_R(r_scratch, r_off, r_skb_data),
> ctx);
> - else if (load_order == 1)
> - emit_load_be16(condt, r_A, r_scratch, ctx);
> - else if (load_order == 2)
> - emit_load_be32(condt, r_A, r_scratch, ctx);
>
> - _emit(condt, ARM_B(b_imm(i + 1, ctx)), ctx);
> + if (load_order == 0)
> + _emit(condt, ARM_LDRB_I(r_A, r_scratch, 0),
> + ctx);
> + else if (load_order == 1)
> + emit_load_be16(condt, r_A, r_scratch, ctx);
> + else if (load_order == 2)
> + emit_load_be32(condt, r_A, r_scratch, ctx);
>
> + _emit(condt, ARM_B(b_imm(i + 1, ctx)), ctx);
> + } else {
> + load_func = load_func_neg;
> + }
> +load_common:
> + ctx->seen |= SEEN_DATA | SEEN_CALL;
> /* the slowpath */
> emit_mov_i(ARM_R3, (u32)load_func[load_order], ctx);
> emit(ARM_MOV_R(ARM_R0, r_skb), ctx);
> @@ -547,7 +607,9 @@ load_common:
> case BPF_S_LD_B_IND:
> load_order = 0;
> load_ind:
> + load_func = load_func_any;
> OP_IMM3(ARM_ADD, r_off, r_X, k, ctx);
> + load_func = load_func_any;
> goto load_common;
> case BPF_S_LDX_IMM:
> ctx->seen |= SEEN_X;
> @@ -565,25 +627,28 @@ load_ind:
> case BPF_S_LDX_B_MSH:
> /* x = ((*(frame + k)) & 0xf) << 2; */
> ctx->seen |= SEEN_X | SEEN_DATA | SEEN_CALL;
> - /* the interpreter should deal with the negative K */
> - if (k < 0)
> - return -1;
> /* offset in r1: we might have to take the slow path */
> emit_mov_i(r_off, k, ctx);
> - emit(ARM_CMP_R(r_skb_hl, r_off), ctx);
> + /* deal with negative K */
> + if (k >= 0) {
> + load_func = load_func_pos;
> + emit(ARM_CMP_R(r_skb_hl, r_off), ctx);
>
> - /* load in r0: common with the slowpath */
> - _emit(ARM_COND_HI, ARM_LDRB_R(ARM_R0, r_skb_data,
> - ARM_R1), ctx);
> - /*
> - * emit_mov_i() might generate one or two instructions,
> - * the same holds for emit_blx_r()
> - */
> - _emit(ARM_COND_HI, ARM_B(b_imm(i + 1, ctx) - 2), ctx);
> + /* load in r0: common with the slowpath */
> + _emit(ARM_COND_HI, ARM_LDRB_R(ARM_R0, r_skb_data,
> + ARM_R1), ctx);
> + /*
> + * emit_mov_i() might generate one or two instructions,
> + * the same holds for emit_blx_r()
> + */
> + _emit(ARM_COND_HI, ARM_B(b_imm(i + 1, ctx) - 2), ctx);
> + } else {
> + load_func = load_func_neg;
> + }
>
> emit(ARM_MOV_R(ARM_R0, r_skb), ctx);
> /* r_off is r1 */
> - emit_mov_i(ARM_R3, (u32)jit_get_skb_b, ctx);
> + emit_mov_i(ARM_R3, (u32)load_func[0], ctx);
> emit_blx_r(ARM_R3, ctx);
> /* check the return value of skb_copy_bits */
> emit(ARM_CMP_I(ARM_R1, 0), ctx);
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 8eeb205..78cd56d 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -161,6 +161,12 @@ extern int sk_chk_filter(struct sock_filter *filter, unsigned int flen);
> extern void bpf_jit_compile(struct sk_filter *fp);
> extern void bpf_jit_free(struct sk_filter *fp);
> #define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
> +/*
> + * Only Exported for the bpf jit load helper.
> + * Do not call from anywhere else!
> + */
> +extern void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb,
> + int k, unsigned int size);
> #else
> static inline void bpf_jit_compile(struct sk_filter *fp)
> {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6f755cc..9cbaecb 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -42,6 +42,10 @@
> /* No hurry in this branch
> *
> * Exported for the bpf jit load helper.
> + *
> + * CAUTION ! :
> + * If its prototype is ever changed, check arch/{*}/net/{*}.S files,
> + * since it is called from BPF assembly code.
> */
> void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
> {
>
--
Mircea
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists