netdev - Re: [PATCH bpf-next 11/13] bpf, mips64: remove unneeded zero check from div/mod with k

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d5deb3b6-871e-1594-6ac2-ccbc9a71e50c@caviumnetworks.com>
Date:   Fri, 26 Jan 2018 14:36:24 -0800
From:   David Daney <ddaney@...iumnetworks.com>
To:     Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc:     netdev@...r.kernel.org, David Daney <david.daney@...ium.com>
Subject: Re: [PATCH bpf-next 11/13] bpf, mips64: remove unneeded zero check
 from div/mod with k

On 01/26/2018 02:33 PM, Daniel Borkmann wrote:
> The verifier in both cBPF and eBPF reject div/mod by 0 imm,
> so this can never load. Remove emitting such test and reject
> it from being JITed instead (the latter is actually also not
> needed, but given practice in sparc64, ppc64 today, so
> doesn't hurt to add it here either).
> 
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> Cc: David Daney <david.daney@...ium.com>

This looks plausible,

Reviewed-by: David Daney <david.daney@...ium.com>


> ---
>   arch/mips/net/ebpf_jit.c | 19 ++++---------------
>   1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/mips/net/ebpf_jit.c b/arch/mips/net/ebpf_jit.c
> index 296f1410..3e2798b 100644
> --- a/arch/mips/net/ebpf_jit.c
> +++ b/arch/mips/net/ebpf_jit.c
> @@ -741,16 +741,11 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>   		break;
>   	case BPF_ALU | BPF_DIV | BPF_K: /* ALU_IMM */
>   	case BPF_ALU | BPF_MOD | BPF_K: /* ALU_IMM */
> +		if (insn->imm == 0)
> +			return -EINVAL;
>   		dst = ebpf_to_mips_reg(ctx, insn, dst_reg);
>   		if (dst < 0)
>   			return dst;
> -		if (insn->imm == 0) { /* Div by zero */
> -			b_off = b_imm(exit_idx, ctx);
> -			if (is_bad_offset(b_off))
> -				return -E2BIG;
> -			emit_instr(ctx, beq, MIPS_R_ZERO, MIPS_R_ZERO, b_off);
> -			emit_instr(ctx, addu, MIPS_R_V0, MIPS_R_ZERO, MIPS_R_ZERO);
> -		}
>   		td = get_reg_val_type(ctx, this_idx, insn->dst_reg);
>   		if (td == REG_64BIT || td == REG_32BIT_ZERO_EX)
>   			/* sign extend */
> @@ -770,19 +765,13 @@ static int build_one_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>   		break;
>   	case BPF_ALU64 | BPF_DIV | BPF_K: /* ALU_IMM */
>   	case BPF_ALU64 | BPF_MOD | BPF_K: /* ALU_IMM */
> +		if (insn->imm == 0)
> +			return -EINVAL;
>   		dst = ebpf_to_mips_reg(ctx, insn, dst_reg);
>   		if (dst < 0)
>   			return dst;
> -		if (insn->imm == 0) { /* Div by zero */
> -			b_off = b_imm(exit_idx, ctx);
> -			if (is_bad_offset(b_off))
> -				return -E2BIG;
> -			emit_instr(ctx, beq, MIPS_R_ZERO, MIPS_R_ZERO, b_off);
> -			emit_instr(ctx, addu, MIPS_R_V0, MIPS_R_ZERO, MIPS_R_ZERO);
> -		}
>   		if (get_reg_val_type(ctx, this_idx, insn->dst_reg) == REG_32BIT)
>   			emit_instr(ctx, dinsu, dst, MIPS_R_ZERO, 32, 32);
> -
>   		if (insn->imm == 1) {
>   			/* div by 1 is a nop, mod by 1 is zero */
>   			if (bpf_op == BPF_MOD)
>