[PATCH 04/13] bpf: move tmp variable into ax register in interpreter

Kleber Souza kleber.souza at canonical.com
Thu Mar 7 14:02:26 UTC 2019


Hi Tyler,

On 2/11/19 6:25 AM, Tyler Hicks wrote:
> From: Daniel Borkmann <daniel at iogearbox.net>
> 
> This change moves the on-stack 64 bit tmp variable in ___bpf_prog_run()
> into the hidden ax register. The latter is currently only used in JITs
> for constant blinding as a temporary scratch register, meaning the BPF
> interpreter will never see the use of ax. Therefore it is safe to use
> it for the cases where tmp has been used earlier. This is needed to later
> on allow restricted hidden use of ax in both interpreter and JITs.
> 
> Signed-off-by: Daniel Borkmann <daniel at iogearbox.net>
> Acked-by: Alexei Starovoitov <ast at kernel.org>
> Signed-off-by: Alexei Starovoitov <ast at kernel.org>
> 
> CVE-2019-7308
> 
> (backported from commit 144cd91c4c2bced6eb8a7e25e590f6618a11e854)
> [tyhicks: Backport around context changes and missing commit 1ea47e01ad6e]
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> ---
>  include/linux/filter.h |  3 ++-
>  kernel/bpf/core.c      | 32 ++++++++++++++++----------------
>  2 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index e85292a16467..ec944f15f1a4 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -53,7 +53,8 @@ struct bpf_prog_aux;
>   * constants. See JIT pre-step in bpf_jit_blind_constants().
>   */
>  #define BPF_REG_AX		MAX_BPF_REG
> -#define MAX_BPF_JIT_REG		(MAX_BPF_REG + 1)
> +#define MAX_BPF_EXT_REG		(MAX_BPF_REG + 1)
> +#define MAX_BPF_JIT_REG		MAX_BPF_EXT_REG
>  
>  /* unused opcode to mark special call to bpf_tail_call() helper */
>  #define BPF_TAIL_CALL	0xf0
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 7949e8b8f94e..c8b57f2afbd6 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -51,6 +51,7 @@
>  #define DST	regs[insn->dst_reg]
>  #define SRC	regs[insn->src_reg]
>  #define FP	regs[BPF_REG_FP]
> +#define AX	regs[BPF_REG_AX]
>  #define ARG1	regs[BPF_REG_ARG1]
>  #define CTX	regs[BPF_REG_CTX]
>  #define IMM	insn->imm
> @@ -778,7 +779,6 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
>  static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
>  				    u64 *stack)
>  {
> -	u64 tmp;
>  	static const void *jumptable[256] = {
>  		[0 ... 255] = &&default_label,
>  		/* Now overwrite non-defaults ... */
> @@ -952,22 +952,22 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
>  	ALU64_MOD_X:
>  		if (unlikely(SRC == 0))
>  			return 0;
> -		div64_u64_rem(DST, SRC, &tmp);
> -		DST = tmp;
> +		div64_u64_rem(DST, SRC, &AX);
> +		DST = AX;
>  		CONT;
>  	ALU_MOD_X:
>  		if (unlikely((u32)SRC == 0))
>  			return 0;
> -		tmp = (u32) DST;
> -		DST = do_div(tmp, (u32) SRC);
> +		AX = (u32) DST;
> +		DST = do_div(AX, (u32) SRC);
>  		CONT;
>  	ALU64_MOD_K:
> -		div64_u64_rem(DST, IMM, &tmp);
> -		DST = tmp;
> +		div64_u64_rem(DST, IMM, &AX);
> +		DST = AX;
>  		CONT;
>  	ALU_MOD_K:
> -		tmp = (u32) DST;
> -		DST = do_div(tmp, (u32) IMM);
> +		AX = (u32) DST;
> +		DST = do_div(AX, (u32) IMM);
>  		CONT;
>  	ALU64_DIV_X:
>  		if (unlikely(SRC == 0))
> @@ -977,17 +977,17 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
>  	ALU_DIV_X:
>  		if (unlikely((u32)SRC == 0))
>  			return 0;
> -		tmp = (u32) DST;
> -		do_div(tmp, (u32) SRC);
> -		DST = (u32) tmp;
> +		AX = (u32) DST;
> +		do_div(AX, (u32) SRC);
> +		DST = (u32) AX;
>  		CONT;
>  	ALU64_DIV_K:
>  		DST = div64_u64(DST, IMM);
>  		CONT;
>  	ALU_DIV_K:
> -		tmp = (u32) DST;
> -		do_div(tmp, (u32) IMM);
> -		DST = (u32) tmp;
> +		AX = (u32) DST;
> +		do_div(AX, (u32) IMM);
> +		DST = (u32) AX;
>  		CONT;
>  	ALU_END_TO_BE:
>  		switch (IMM) {
> @@ -1291,7 +1291,7 @@ STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */
>  static unsigned int PROG_NAME(stack_size)(const void *ctx, const struct bpf_insn *insn) \
>  { \
>  	u64 stack[stack_size / sizeof(u64)]; \
> -	u64 regs[MAX_BPF_REG]; \
> +	u64 regs[MAX_BPF_EXT_REG]; \
>  \
>  	FP = (u64) (unsigned long) &stack[ARRAY_SIZE(stack)]; \
>  	ARG1 = (u64) (unsigned long) ctx; \
> 

This code doesn't compile when CONFIG_BPF_JIT_ALWAYS_ON is not set. The
Bionic kernel doesn't have e0cea7ce988c (bpf: implement ld_abs/ld_ind in
native bpf), so the tmp variable is still used in other places. It seems
that we also need the additional fixes:

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 3cf79f0eac6c..f20df89589ed 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1262,7 +1262,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 		 *   BPF_R0 - 8/16/32-bit skb data converted to cpu endianness
 		 */
 
-		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 4, &tmp);
+		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 4, &AX);
 		if (likely(ptr != NULL)) {
 			BPF_R0 = get_unaligned_be32(ptr);
 			CONT;
@@ -1272,7 +1272,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 	LD_ABS_H: /* BPF_R0 = ntohs(*(u16 *) (skb->data + imm32)) */
 		off = IMM;
 load_half:
-		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 2, &tmp);
+		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 2, &AX);
 		if (likely(ptr != NULL)) {
 			BPF_R0 = get_unaligned_be16(ptr);
 			CONT;
@@ -1282,7 +1282,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 	LD_ABS_B: /* BPF_R0 = *(u8 *) (skb->data + imm32) */
 		off = IMM;
 load_byte:
-		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 1, &tmp);
+		ptr = bpf_load_pointer((struct sk_buff *) (unsigned long) CTX, off, 1, &AX);
 		if (likely(ptr != NULL)) {
 			BPF_R0 = *(u8 *)ptr;
 			CONT;


Does it make sense?

If it does, could you please send a v2 of this patch? We can replace the
original backport applied to the tree so we don't break bisectability.


Thanks,
Kleber 



More information about the kernel-team mailing list