* [PATCH v2 1/3] x86/unwind/orc: Change REG_SP_INDIRECT
2021-02-09 9:16 [PATCH v2 0/3] objtool: Support the stack swizzle Peter Zijlstra
@ 2021-02-09 9:16 ` Peter Zijlstra
2021-02-09 9:16 ` [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg) Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-09 9:16 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel, peterz
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)
2021-02-09 9:16 [PATCH v2 0/3] objtool: Support the stack swizzle Peter Zijlstra
2021-02-09 9:16 ` [PATCH v2 1/3] x86/unwind/orc: Change REG_SP_INDIRECT Peter Zijlstra
@ 2021-02-09 9:16 ` Peter Zijlstra
2021-02-09 16:32 ` Josh Poimboeuf
2021-02-10 9:08 ` [PATCH v2.1 " Peter Zijlstra
2021-02-09 9:16 ` [PATCH v2 3/3] objtool: Support stack-swizzle Peter Zijlstra
2021-02-09 15:45 ` [PATCH v2 0/3] objtool: Support the stack swizzle Miroslav Benes
3 siblings, 2 replies; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-09 9:16 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel, peterz
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;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)
2021-02-09 9:16 ` [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg) Peter Zijlstra
@ 2021-02-09 16:32 ` Josh Poimboeuf
2021-02-09 16:42 ` Peter Zijlstra
2021-02-10 9:08 ` [PATCH v2.1 " Peter Zijlstra
1 sibling, 1 reply; 10+ messages in thread
From: Josh Poimboeuf @ 2021-02-09 16:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Miroslav Benes, Nick Desaulniers,
Julien Thierry, Kees Cook, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)
2021-02-09 16:32 ` Josh Poimboeuf
@ 2021-02-09 16:42 ` Peter Zijlstra
0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-09 16:42 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Miroslav Benes, Nick Desaulniers,
Julien Thierry, Kees Cook, x86, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2.1 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)
2021-02-09 9:16 ` [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg) Peter Zijlstra
2021-02-09 16:32 ` Josh Poimboeuf
@ 2021-02-10 9:08 ` Peter Zijlstra
2021-02-10 9:31 ` Peter Zijlstra
1 sibling, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-10 9:08 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel
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;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2.1 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg)
2021-02-10 9:08 ` [PATCH v2.1 " Peter Zijlstra
@ 2021-02-10 9:31 ` Peter Zijlstra
0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-10 9:31 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel
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...
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] objtool: Support stack-swizzle
2021-02-09 9:16 [PATCH v2 0/3] objtool: Support the stack swizzle Peter Zijlstra
2021-02-09 9:16 ` [PATCH v2 1/3] x86/unwind/orc: Change REG_SP_INDIRECT Peter Zijlstra
2021-02-09 9:16 ` [PATCH v2 2/3] objtool,x86: Additionally decode: mov %rsp, (%reg) Peter Zijlstra
@ 2021-02-09 9:16 ` Peter Zijlstra
2021-02-09 10:35 ` Peter Zijlstra
2021-02-09 15:45 ` [PATCH v2 0/3] objtool: Support the stack swizzle Miroslav Benes
3 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-09 9:16 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel, peterz
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;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 3/3] objtool: Support stack-swizzle
2021-02-09 9:16 ` [PATCH v2 3/3] objtool: Support stack-swizzle Peter Zijlstra
@ 2021-02-09 10:35 ` Peter Zijlstra
0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2021-02-09 10:35 UTC (permalink / raw)
To: Josh Poimboeuf, Thomas Gleixner
Cc: Miroslav Benes, Nick Desaulniers, Julien Thierry, Kees Cook, x86,
linux-kernel
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;
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/3] objtool: Support the stack swizzle
2021-02-09 9:16 [PATCH v2 0/3] objtool: Support the stack swizzle Peter Zijlstra
` (2 preceding siblings ...)
2021-02-09 9:16 ` [PATCH v2 3/3] objtool: Support stack-swizzle Peter Zijlstra
@ 2021-02-09 15:45 ` Miroslav Benes
3 siblings, 0 replies; 10+ messages in thread
From: Miroslav Benes @ 2021-02-09 15:45 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Josh Poimboeuf, Thomas Gleixner, Nick Desaulniers,
Julien Thierry, Kees Cook, x86, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread