linux-kernel - Re: [PATCH bpf-next v4 2/3] bpf, x86: clean garbage value in the stack of trampoline

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: <6c195897-e2c5-f0b8-45b4-83a0e9b71bf8@meta.com>
Date:   Fri, 9 Jun 2023 20:20:09 -0700
From:   Yonghong Song <yhs@...a.com>
To:     menglong8.dong@...il.com, andrii.nakryiko@...il.com,
        alan.maguire@...cle.com
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, Menglong Dong <imagedong@...cent.com>
Subject: Re: [PATCH bpf-next v4 2/3] bpf, x86: clean garbage value in the
 stack of trampoline



On 6/9/23 2:56 AM, menglong8.dong@...il.com wrote:
> From: Menglong Dong <imagedong@...cent.com>
> 
> There are garbage values in upper bytes when we store the arguments
> into stack in save_regs() if the size of the argument less then 8.
> 
> As we already reserve 8 byte for the arguments in regs and stack,
> it is ok to store/restore the regs in BPF_DW size. Then, the garbage
> values in upper bytes will be cleaned.
> 
> Signed-off-by: Menglong Dong <imagedong@...cent.com>
> ---
> v4:
> - clean grabage value when argument count is 7
> ---
>   arch/x86/net/bpf_jit_comp.c | 45 ++++++++++++++++++++++++++-----------
>   1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a767e13c8c85..f6f51a5d14db 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1857,6 +1857,28 @@ st:			if (is_imm8(insn->off))
>   	return proglen;
>   }
>   
> +static inline void clean_garbage(u8 **pprog, int nr_regs, int stack_size,
> +				 int arg_size)
> +{
> +	u8 *prog;
> +
> +	/* clean potential garbage values in upper 32-bit. 'stack_size'
> +	 * here is the offset of the 7th argument on-stack.

Here, we have a huge assumption that if only 7 registers, compiler might
just allocate a 8-byte stack space and write the value to it. If the
type of the value is <= 32bit, a 32bit store will be on the stack.
So in this case, the upper 32bit needs to be cleared.
If the number of arguments (in terms of number of registers) is more 
than 7, the extra arguments will be 'pushed' to the stack, so there
is no garbage. This could be true. But please add enough details
here so people knows why we special case this particular instance.

> +	 */
> +	if (nr_regs == 7 && arg_size <= 4) {
> +		int off = -(stack_size - 4);
> +
> +		prog = *pprog;
> +		/* mov DWORD PTR [rbp + off], 0 */
> +		if (!is_imm8(off))
> +			EMIT2_off32(0xC7, 0x85, off);
> +		else
> +			EMIT3(0xC7, 0x45, off);
> +		EMIT(0, 4);
> +		*pprog = prog;
> +	}
> +}
> +
>   static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   		      int stack_size)
>   {
> @@ -1878,8 +1900,7 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   
>   		if (i <= 5) {
>   			/* copy function arguments from regs into stack */
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP,
>   				 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>   				 -(stack_size - i * 8));
>   		} else {
> @@ -1893,17 +1914,16 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   			 *   8(return addr of the caller)
>   			 * which means: rbp + 24
>   			 */
> -			emit_ldx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_0, BPF_REG_FP,
> +			emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
>   				 (i - 6) * 8 + 0x18);
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> -				 BPF_REG_0,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
>   				 -(stack_size - i * 8));
>   		}
>   
>   		j = next_same_struct ? j : j + 1;
>   	}
> +
> +	clean_garbage(prog, nr_regs, stack_size - 6 * 8, arg_size);
>   }
>   
>   static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> @@ -1925,7 +1945,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   			next_same_struct = !next_same_struct;
>   		}
>   
> -		emit_ldx(prog, bytes_to_bpf_size(arg_size),
> +		emit_ldx(prog, BPF_DW,
>   			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>   			 BPF_REG_FP,
>   			 -(stack_size - i * 8));
> @@ -1956,17 +1976,16 @@ static void prepare_origin_stack(const struct btf_func_model *m, u8 **prog,
>   		}
>   
>   		if (i > 5) {
> -			emit_ldx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_0, BPF_REG_FP,
> +			emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
>   				 (i - 6) * 8 + 0x18);
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> -				 BPF_REG_0,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
>   				 -(stack_size - (i - 6) * 8));
>   		}
>   
>   		j = next_same_struct ? j : j + 1;
>   	}
> +
> +	clean_garbage(prog, nr_regs, stack_size, arg_size);
>   }
>   
>   static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,