lkml.org 
[lkml]   [2014]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 net] x86: bpf_jit: fix two bugs in eBPF JIT compiler
On Fri, Oct 10, 2014 at 08:30:23PM -0700, Alexei Starovoitov wrote:
> 1.
> JIT compiler using multi-pass approach to converge to final image size,
> since x86 instructions are variable length. It starts with large
> gaps between instructions (so some jumps may use imm32 instead of imm8)
> and iterates until total program size is the same as in previous pass.
> This algorithm works only if program size is strictly decreasing.
> Programs that use LD_ABS insn need additional code in prologue, but it
> was not emitted during 1st pass, so there was a chance that 2nd pass would
> adjust imm32->imm8 jump offsets to the same number of bytes as increase in
> prologue, which may cause algorithm to erroneously decide that size converged.
> Fix it by always emitting largest prologue in the first pass which
> is detected by oldproglen==0 check.
> Also change error check condition 'proglen != oldproglen' to fail gracefully.
>
> 2.
> while staring at the code realized that 64-byte buffer may not be enough
> when 1st insn is large, so increase it to 128 to avoid buffer overflow
> (theoretical maximum size of prologue+div is 109) and add runtime check.
>
> Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT")
> Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

This fixes the crash, thank you!

Tested-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> v1->v2: reduce chances of stack corruption in case of future bugs (suggested by Eric)
>
> note in classic BPF programs 1st insn is always short move, but native eBPF
> programs may trigger buffer overflow. I couldn't force the crash with overflow,
> since there are no further calls while this part of stack is used.
> Both are ugly bugs regardless.
> When net-next opens I will add narrowed down testcase from 'nmap' to testsuite.
>
> arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index d56cd1f..3f62734 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -182,12 +182,17 @@ struct jit_context {
> bool seen_ld_abs;
> };
>
> +/* maximum number of bytes emitted while JITing one eBPF insn */
> +#define BPF_MAX_INSN_SIZE 128
> +#define BPF_INSN_SAFETY 64
> +
> static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> int oldproglen, struct jit_context *ctx)
> {
> struct bpf_insn *insn = bpf_prog->insnsi;
> int insn_cnt = bpf_prog->len;
> - u8 temp[64];
> + bool seen_ld_abs = ctx->seen_ld_abs | (oldproglen == 0);
> + u8 temp[BPF_MAX_INSN_SIZE + BPF_INSN_SAFETY];
> int i;
> int proglen = 0;
> u8 *prog = temp;
> @@ -225,7 +230,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> EMIT2(0x31, 0xc0); /* xor eax, eax */
> EMIT3(0x4D, 0x31, 0xED); /* xor r13, r13 */
>
> - if (ctx->seen_ld_abs) {
> + if (seen_ld_abs) {
> /* r9d : skb->len - skb->data_len (headlen)
> * r10 : skb->data
> */
> @@ -685,7 +690,7 @@ xadd: if (is_imm8(insn->off))
> case BPF_JMP | BPF_CALL:
> func = (u8 *) __bpf_call_base + imm32;
> jmp_offset = func - (image + addrs[i]);
> - if (ctx->seen_ld_abs) {
> + if (seen_ld_abs) {
> EMIT2(0x41, 0x52); /* push %r10 */
> EMIT2(0x41, 0x51); /* push %r9 */
> /* need to adjust jmp offset, since
> @@ -699,7 +704,7 @@ xadd: if (is_imm8(insn->off))
> return -EINVAL;
> }
> EMIT1_off32(0xE8, jmp_offset);
> - if (ctx->seen_ld_abs) {
> + if (seen_ld_abs) {
> EMIT2(0x41, 0x59); /* pop %r9 */
> EMIT2(0x41, 0x5A); /* pop %r10 */
> }
> @@ -804,7 +809,8 @@ emit_jmp:
> goto common_load;
> case BPF_LD | BPF_ABS | BPF_W:
> func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> -common_load: ctx->seen_ld_abs = true;
> +common_load:
> + ctx->seen_ld_abs = seen_ld_abs = true;
> jmp_offset = func - (image + addrs[i]);
> if (!func || !is_simm32(jmp_offset)) {
> pr_err("unsupported bpf func %d addr %p image %p\n",
> @@ -878,6 +884,11 @@ common_load: ctx->seen_ld_abs = true;
> }
>
> ilen = prog - temp;
> + if (ilen > BPF_MAX_INSN_SIZE) {
> + pr_err("bpf_jit_compile fatal insn size error\n");
> + return -EFAULT;
> + }
> +
> if (image) {
> if (unlikely(proglen + ilen > oldproglen)) {
> pr_err("bpf_jit_compile fatal error\n");
> @@ -934,9 +945,11 @@ void bpf_int_jit_compile(struct bpf_prog *prog)
> goto out;
> }
> if (image) {
> - if (proglen != oldproglen)
> + if (proglen != oldproglen) {
> pr_err("bpf_jit: proglen=%d != oldproglen=%d\n",
> proglen, oldproglen);
> + goto out;
> + }
> break;
> }
> if (proglen == oldproglen) {
> --
> 1.7.9.5
>


\
 
 \ /
  Last update: 2014-10-13 20:01    [W:0.036 / U:1.520 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site