[net] x86: bpf_jit: fix two bugs in eBPF JIT compiler

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>
---
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 |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

On Fri, 2014-10-10 at 19:44 -0700, Alexei Starovoitov wrote:

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


> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index d56cd1f..8266896 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -187,7 +187,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>  {
>  	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[128];

Hmmm. I would use some guard like :

#define BPF_MAX_INSN_SIZE 128
#define BPF_INSN_SAFETY   64

	u8 temp[MAX_INSN_SIZE + BPF_INSN_SAFETY];
 

> +		if (ilen >= sizeof(temp)) {

	if (ilen > BPF_MAX_INSN_SIZE) {
...

> +			pr_err("bpf_jit_compile fatal insn size error\n");
> +			return -EFAULT;
> +		}
> +

Otherwise, we might have corrupted stack and panic anyway.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 10, 2014 at 8:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-10-10 at 19:44 -0700, Alexei Starovoitov wrote:
>
>> 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.
>>
>
>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index d56cd1f..8266896 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -187,7 +187,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>>  {
>>       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[128];
>
> Hmmm. I would use some guard like :
>
> #define BPF_MAX_INSN_SIZE 128
> #define BPF_INSN_SAFETY   64
>
>         u8 temp[MAX_INSN_SIZE + BPF_INSN_SAFETY];
>
>
>> +             if (ilen >= sizeof(temp)) {
>
>         if (ilen > BPF_MAX_INSN_SIZE) {
> ...
>
>> +                     pr_err("bpf_jit_compile fatal insn size error\n");
>> +                     return -EFAULT;
>> +             }
>> +
>
> Otherwise, we might have corrupted stack and panic anyway.

well, it only reduces the chances of stack corruption.. but yeah,
let's reduce them. will respin.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html