diff mbox series

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

Message ID 20230609095653.1406173-3-imagedong@tencent.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf, x86: allow function arguments up to 12 for TRACING | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail merge-conflict
netdev/tree_selection success Clearly marked for bpf-next
netdev/apply fail Patch does not apply to bpf-next
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat

Commit Message

Menglong Dong June 9, 2023, 9:56 a.m. UTC 
From: Menglong Dong <imagedong@tencent.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@tencent.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(-)

Comments

Yonghong Song June 10, 2023, 3:20 a.m. UTC | #1 
On 6/9/23 2:56 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.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@tencent.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,
Menglong Dong June 10, 2023, 6:33 a.m. UTC | #2 
On Sat, Jun 10, 2023 at 11:20 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 6/9/23 2:56 AM, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.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@tencent.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.
>

Yeah, this indeed is a huge assumption. I'll add more
comments here to help others understand this case.

Thanks!
Menglong Dong

> > +      */
> > +     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,
diff mbox series

Patch

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.
+	 */
+	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,