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
State New
Headers
Series bpf, x86: allow function arguments up to 12 for TRACING |

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,