[PATCH v2 0/3] objtool: Support the stack swizzle

[PATCH v2 0/3] objtool: Support the stack swizzle

[Peter Zijlstra]

Hi!

Implement objtool support for the x86_64 stack swizzle pattern.

This means we can use the minial stack swizzle:

  mov %rsp, (%[tos])
  mov %[tos], %rsp
  ...
  pop %rsp

from inline asm, with arbitrary stack setup. The ORC data for the Top-of-Stack
will use the SP_INDIRECT CFA base. In order for this to work, SP_INDIRECT needs
to first dereference and then add the offset to find the next frame.

Therefore we need to change SP_INDIRECT (which is currently unused) to mean:
(%rsp) + offset.

Changes since v1 include:

 - removed the !callee saved reg restriction by using the vals[] array
   over the regs[] array.

 - per the above, removed the patches creating the regs[] scratch space.

 - more comments.

 - rebased to tip/objtool/core

[PATCH v2 1/3] x86/unwind/orc: Change REG_SP_INDIRECT

[Peter Zijlstra]

Currently REG_SP_INDIRECT is unused but means (%rsp + offset),
change it to mean (%rsp) + offset.

The reason is that we're going to swizzle stack in the middle of a C
function with non-trivial stack footprint. This means that when the
unwinder finds the ToS, it needs to dereference it (%rsp) and then add
the offset to the next frame, resulting in: (%rsp) + offset

This is somewhat unfortunate, since REG_BP_INDIRECT is used (by DRAP)
and thus needs to retain the current (%rbp + offset).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/unwind_orc.c |    5 ++++-
 tools/objtool/orc_dump.c     |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -471,7 +471,7 @@ bool unwind_next_frame(struct unwind_sta
 		break;
 
 	case ORC_REG_SP_INDIRECT:
-		sp = state->sp + orc->sp_offset;
+		sp = state->sp;
 		indirect = true;
 		break;
 
@@ -521,6 +521,9 @@ bool unwind_next_frame(struct unwind_sta
 	if (indirect) {
 		if (!deref_stack_reg(state, sp, &sp))
 			goto err;
+
+		if (orc->sp_reg == ORC_REG_SP_INDIRECT)
+			sp += orc->sp_offset;
 	}
 
 	/* Find IP, SP and possibly regs: */
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -54,7 +54,7 @@ static void print_reg(unsigned int reg,
 	if (reg == ORC_REG_BP_INDIRECT)
 		printf("(bp%+d)", offset);
 	else if (reg == ORC_REG_SP_INDIRECT)
-		printf("(sp%+d)", offset);
+		printf("(sp)%+d", offset);
 	else if (reg == ORC_REG_UNDEFINED)
 		printf("(und)");
 	else

[PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Peter Zijlstra]

Where we already decode: mov %rsp, %reg, also decode mov %rsp, (%reg).

Nothing should match for this new stack-op.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch/x86/decode.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -222,14 +222,24 @@ int arch_decode_instruction(const struct
 		break;
 
 	case 0x89:
-		if (rex_w && !rex_r && modrm_mod == 3 && modrm_reg == 4) {
+		if (rex_w && !rex_r && modrm_reg == 4) {
 
-			/* mov %rsp, reg */
+			/* mov %rsp, */
 			ADD_OP(op) {
 				op->src.type = OP_SRC_REG;
 				op->src.reg = CFI_SP;
-				op->dest.type = OP_DEST_REG;
-				op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+				if (modrm_mod == 3) {
+
+					/* mov %rsp, reg */
+					op->dest.type = OP_DEST_REG;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+
+				} else if (modrm_mod == 0) {
+
+					/* mov %rsp, (%reg) */
+					op->dest.type = OP_DEST_REG_INDIRECT;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+				}
 			}
 			break;
 		}
@@ -259,8 +269,10 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_BP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
+		}
 
-		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
+		if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
 
 			/* mov reg, disp(%rsp) */
 			ADD_OP(op) {
@@ -270,6 +282,7 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_SP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
 		}
 
 		break;

[PATCH v2 3/3] objtool: Support stack-swizzle

[Peter Zijlstra]

Natively support the stack swizzle pattern:

	mov %rsp, (%[tos])
	mov %[tos], %rsp
	...
	pop %rsp

It uses the vals[] array to link the first two stack-ops, and detect
the SP to SP_INDIRECT swizzle.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1945,6 +1945,38 @@ static int update_cfi_state(struct instr
 					cfa->offset = -cfi->vals[op->src.reg].offset;
 					cfi->stack_size = cfa->offset;
 
+				} else if (cfa->base == CFI_SP &&
+					   cfi->vals[op->src.reg].base == CFI_SP_INDIRECT &&
+					   cfi->vals[op->src.reg].offset == cfa->offset) {
+
+					/*
+					 * Stack swizzle:
+					 *
+					 * 1: mov %rsp, (%[tos])
+					 * 2: mov %[tos], %rsp
+					 *    ...
+					 * 3: pop %rsp
+					 *
+					 * Where:
+					 *
+					 * 1 - places a pointer to the previous
+					 *     stack at the Top-of-Stack of the
+					 *     new stack.
+					 *
+					 * 2 - switches to the new stack.
+					 *
+					 * 3 - pops the Top-of-Stack to restore
+					 *     the original stack.
+					 *
+					 * Note: we set base to SP_INDIRECT
+					 * here and preserve offset. Therefore
+					 * when the unwinder reaches ToS it
+					 * will dereference SP and then add the
+					 * offset to find the next frame, IOW:
+					 * (%rsp) + offset.
+					 */
+					cfa->base = CFI_SP_INDIRECT;
+
 				} else {
 					cfa->base = CFI_UNDEFINED;
 					cfa->offset = 0;
@@ -2047,6 +2079,13 @@ static int update_cfi_state(struct instr
 
 		case OP_SRC_POP:
 		case OP_SRC_POPF:
+			if (op->dest.reg == CFI_SP && cfa->base == CFI_SP_INDIRECT) {
+
+				/* pop %rsp; # restore from a stack swizzle */
+				cfa->base = CFI_SP;
+				break;
+			}
+
 			if (!cfi->drap && op->dest.reg == cfa->base) {
 
 				/* pop %rbp */
@@ -2193,6 +2232,12 @@ static int update_cfi_state(struct instr
 			/* mov reg, disp(%rsp) */
 			save_reg(cfi, op->src.reg, CFI_CFA,
 				 op->dest.offset - cfi->stack_size);
+
+		} else if (op->src.reg == CFI_SP && op->dest.offset == 0) {
+
+			/* mov %rsp, (%reg); # setup a stack swizzle. */
+			cfi->vals[op->dest.reg].base = CFI_SP_INDIRECT;
+			cfi->vals[op->dest.reg].offset = cfi->cfa.offset;
 		}
 
 		break;

Re: [PATCH v2 3/3] objtool: Support stack-swizzle

[Peter Zijlstra]

On Tue, Feb 09, 2021 at 10:16:03AM +0100, Peter Zijlstra wrote:
> @@ -2193,6 +2232,12 @@ static int update_cfi_state(struct instr
>  			/* mov reg, disp(%rsp) */
>  			save_reg(cfi, op->src.reg, CFI_CFA,
>  				 op->dest.offset - cfi->stack_size);
> +
> +		} else if (op->src.reg == CFI_SP && op->dest.offset == 0) {
> +
> +			/* mov %rsp, (%reg); # setup a stack swizzle. */
> +			cfi->vals[op->dest.reg].base = CFI_SP_INDIRECT;
> +			cfi->vals[op->dest.reg].offset = cfi->cfa.offset;

I'll change that to:

			cfi->vals[op->dest.reg].offset = cfa->offset;

To be more consistent with the rest of the code.

>  		}
>  
>  		break;
> 
> 

Re: [PATCH v2 0/3] objtool: Support the stack swizzle

[Miroslav Benes]

On Tue, 9 Feb 2021, Peter Zijlstra wrote:

> Hi!
> 
> Implement objtool support for the x86_64 stack swizzle pattern.
> 
> This means we can use the minial stack swizzle:
> 
>   mov %rsp, (%[tos])
>   mov %[tos], %rsp
>   ...
>   pop %rsp
> 
> from inline asm, with arbitrary stack setup. The ORC data for the Top-of-Stack
> will use the SP_INDIRECT CFA base. In order for this to work, SP_INDIRECT needs
> to first dereference and then add the offset to find the next frame.
> 
> Therefore we need to change SP_INDIRECT (which is currently unused) to mean:
> (%rsp) + offset.
> 
> Changes since v1 include:
> 
>  - removed the !callee saved reg restriction by using the vals[] array
>    over the regs[] array.
> 
>  - per the above, removed the patches creating the regs[] scratch space.
> 
>  - more comments.
> 
>  - rebased to tip/objtool/core

I haven't tested it, but it all looks good to me.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Josh Poimboeuf]

On Tue, Feb 09, 2021 at 10:16:02AM +0100, Peter Zijlstra wrote:
> Where we already decode: mov %rsp, %reg, also decode mov %rsp, (%reg).
> 
> Nothing should match for this new stack-op.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/objtool/arch/x86/decode.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -222,14 +222,24 @@ int arch_decode_instruction(const struct
>  		break;
>  
>  	case 0x89:
> -		if (rex_w && !rex_r && modrm_mod == 3 && modrm_reg == 4) {
> +		if (rex_w && !rex_r && modrm_reg == 4) {
>  
> -			/* mov %rsp, reg */
> +			/* mov %rsp, */
>  			ADD_OP(op) {
>  				op->src.type = OP_SRC_REG;
>  				op->src.reg = CFI_SP;
> -				op->dest.type = OP_DEST_REG;
> -				op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> +				if (modrm_mod == 3) {
> +
> +					/* mov %rsp, reg */
> +					op->dest.type = OP_DEST_REG;
> +					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> +
> +				} else if (modrm_mod == 0) {
> +
> +					/* mov %rsp, (%reg) */
> +					op->dest.type = OP_DEST_REG_INDIRECT;
> +					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> +				}

What if modrm_mod is 1 or 2?   Should 'if' below the 'case' make sure
it's 0 or 3?

>  			}
>  			break;
>  		}
> @@ -259,8 +269,10 @@ int arch_decode_instruction(const struct
>  				op->dest.reg = CFI_BP;
>  				op->dest.offset = insn.displacement.value;
>  			}
> +			break;
> +		}
>  
> -		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
> +		if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
>  
>  			/* mov reg, disp(%rsp) */
>  			ADD_OP(op) {
> @@ -270,6 +282,7 @@ int arch_decode_instruction(const struct
>  				op->dest.reg = CFI_SP;
>  				op->dest.offset = insn.displacement.value;
>  			}
> +			break;
>  		}
>  
>  		break;
> 

Did this change have a point?

-- 
Josh

Re: [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Peter Zijlstra]

On Tue, Feb 09, 2021 at 10:32:55AM -0600, Josh Poimboeuf wrote:
> On Tue, Feb 09, 2021 at 10:16:02AM +0100, Peter Zijlstra wrote:
> > Where we already decode: mov %rsp, %reg, also decode mov %rsp, (%reg).
> > 
> > Nothing should match for this new stack-op.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  tools/objtool/arch/x86/decode.c |   23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > --- a/tools/objtool/arch/x86/decode.c
> > +++ b/tools/objtool/arch/x86/decode.c
> > @@ -222,14 +222,24 @@ int arch_decode_instruction(const struct
> >  		break;
> >  
> >  	case 0x89:
> > -		if (rex_w && !rex_r && modrm_mod == 3 && modrm_reg == 4) {
> > +		if (rex_w && !rex_r && modrm_reg == 4) {
> >  
> > -			/* mov %rsp, reg */
> > +			/* mov %rsp, */
> >  			ADD_OP(op) {
> >  				op->src.type = OP_SRC_REG;
> >  				op->src.reg = CFI_SP;
> > -				op->dest.type = OP_DEST_REG;
> > -				op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> > +				if (modrm_mod == 3) {
> > +
> > +					/* mov %rsp, reg */
> > +					op->dest.type = OP_DEST_REG;
> > +					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> > +
> > +				} else if (modrm_mod == 0) {
> > +
> > +					/* mov %rsp, (%reg) */
> > +					op->dest.type = OP_DEST_REG_INDIRECT;
> > +					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
> > +				}
> 
> What if modrm_mod is 1 or 2?   Should 'if' below the 'case' make sure
> it's 0 or 3?

For 1,2 we need to look at the SIB byte or something. IIRC you get that
encoding for stuff like: mov %rsp, off(%reg).

Didn't want to dive too deep into the instruction encoding thing again,
this is all we need.

> >  			}
> >  			break;
> >  		}
> > @@ -259,8 +269,10 @@ int arch_decode_instruction(const struct
> >  				op->dest.reg = CFI_BP;
> >  				op->dest.offset = insn.displacement.value;
> >  			}
> > +			break;
> > +		}
> >  
> > -		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
> > +		if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
> >  
> >  			/* mov reg, disp(%rsp) */
> >  			ADD_OP(op) {
> > @@ -270,6 +282,7 @@ int arch_decode_instruction(const struct
> >  				op->dest.reg = CFI_SP;
> >  				op->dest.offset = insn.displacement.value;
> >  			}
> > +			break;
> >  		}
> >  
> >  		break;
> > 
> 
> Did this change have a point?

Consistency, but yeah, I see what you mean.

[PATCH v2.1 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Peter Zijlstra]

Subject: objtool,x86: Additionally decode: mov %rsp, (%reg)
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Feb 3 12:02:18 CET 2021

Where we already decode: mov %rsp, %reg, also decode mov %rsp, (%reg).

Nothing should match for this new stack-op.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/arch/x86/decode.c |   42 ++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -222,15 +222,38 @@ int arch_decode_instruction(const struct
 		break;
 
 	case 0x89:
-		if (rex_w && !rex_r && modrm_mod == 3 && modrm_reg == 4) {
+		if (rex_w && !rex_r && modrm_reg == 4) {
 
-			/* mov %rsp, reg */
-			ADD_OP(op) {
-				op->src.type = OP_SRC_REG;
-				op->src.reg = CFI_SP;
-				op->dest.type = OP_DEST_REG;
-				op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+			if (modrm_mod == 3) {
+				/* mov %rsp, reg */
+				ADD_OP(op) {
+					op->src.type = OP_SRC_REG;
+					op->src.reg = CFI_SP;
+					op->dest.type = OP_DEST_REG;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+				}
+				break;
+
+			} else {
+				/* skip nontrivial SIB */
+				if (modrm_rm == 4 && sib != 0x24)
+					break;
+
+				/* skip RIP relative displacement */
+				if (modrm_rm == 5 && modrm_mod == 0)
+					break;
+
+				/* mov %rsp, disp(%reg) */
+				ADD_OP(op) {
+					op->src.type = OP_SRC_REG;
+					op->src.reg = CFI_SP;
+					op->dest.type = OP_DEST_REG_INDIRECT;
+					op->dest.reg = op_to_cfi_reg[modrm_rm][rex_b];
+					op->dest.offset = insn.displacement.value;
+				}
+				break;
 			}
+
 			break;
 		}
 
@@ -259,8 +282,10 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_BP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
+		}
 
-		} else if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
+		if (rex_w && !rex_b && modrm_rm == 4 && sib == 0x24) {
 
 			/* mov reg, disp(%rsp) */
 			ADD_OP(op) {
@@ -270,6 +295,7 @@ int arch_decode_instruction(const struct
 				op->dest.reg = CFI_SP;
 				op->dest.offset = insn.displacement.value;
 			}
+			break;
 		}
 
 		break;

Re: [PATCH v2.1 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)

[Peter Zijlstra]

On Wed, Feb 10, 2021 at 10:08:51AM +0100, Peter Zijlstra wrote:
> +				/* skip nontrivial SIB */
> +				if (modrm_rm == 4 && sib != 0x24)
> +					break;

Hmm,, maybe that should be:

	if (modrm_rm == 4 && !(sib == 0x24 && rex_b == rex_x))

Because what we have is that once we have a SIB byte, rex_b is for
sib_base and rex_x is always sib_index, and we need to ensure that
sib_base == sib_index for the trivial case.


/me changes...