2023-01-30 13:54:46

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
__tdx_hypercall() messes up RBP.

objtool: __tdx_hypercall+0x7f: return with modified stack frame

Rework the function to store TDX_HCALL_ flags on stack instead of RBP.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
Link: https://lore.kernel.org/all/[email protected]
Reported-by: kernel test robot <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---

The patch is against tip/x86/tdx. tip/sched/core removes
TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
after that.

---
arch/x86/coco/tdx/tdcall.S | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/coco/tdx/tdcall.S b/arch/x86/coco/tdx/tdcall.S
index 5da06d1a9ba3..2bd436a4790d 100644
--- a/arch/x86/coco/tdx/tdcall.S
+++ b/arch/x86/coco/tdx/tdcall.S
@@ -131,11 +131,10 @@ SYM_FUNC_START(__tdx_hypercall)
push %r13
push %r12
push %rbx
- push %rbp

/* Free RDI and RSI to be used as TDVMCALL arguments */
movq %rdi, %rax
- movq %rsi, %rbp
+ push %rsi

/* Copy hypercall registers from arg struct: */
movq TDX_HYPERCALL_r8(%rax), %r8
@@ -168,7 +167,7 @@ SYM_FUNC_START(__tdx_hypercall)
* HLT operation indefinitely. Since this is the not the desired
* result, conditionally call STI before TDCALL.
*/
- testq $TDX_HCALL_ISSUE_STI, %rbp
+ testq $TDX_HCALL_ISSUE_STI, 8(%rsp)
jz .Lskip_sti
sti
.Lskip_sti:
@@ -188,7 +187,7 @@ SYM_FUNC_START(__tdx_hypercall)
pop %rax

/* Copy hypercall result registers to arg struct if needed */
- testq $TDX_HCALL_HAS_OUTPUT, %rbp
+ testq $TDX_HCALL_HAS_OUTPUT, (%rsp)
jz .Lout

movq %r8, TDX_HYPERCALL_r8(%rax)
@@ -218,11 +217,12 @@ SYM_FUNC_START(__tdx_hypercall)
xor %r10d, %r10d
xor %r11d, %r11d
xor %rdi, %rdi
- xor %rsi, %rsi
xor %rdx, %rdx

+ /* Remove TDX_HCALL_* flags from the stack */
+ pop %rsi
+
/* Restore callee-saved GPRs as mandated by the x86_64 ABI */
- pop %rbp
pop %rbx
pop %r12
pop %r13
--
2.39.1



2023-01-31 08:33:08

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> __tdx_hypercall() messes up RBP.
>
> objtool: __tdx_hypercall+0x7f: return with modified stack frame
>
> Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> Link: https://lore.kernel.org/all/[email protected]
> Reported-by: kernel test robot <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> ---
>
> The patch is against tip/x86/tdx. tip/sched/core removes
> TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
> after that.

Right, this should work. But it does leave me wondering, should we
perhaps strive to completely remove the flags thing and move to
__tdx_hypercall() and __tdx_hypercall_ret() or something? That is,
simply have two different functions, one with and one without return
data.

It should be trivial to generate that without actual code duplication.


2023-01-31 08:35:42

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> __tdx_hypercall() messes up RBP.
>
> objtool: __tdx_hypercall+0x7f: return with modified stack frame
>
> Rework the function to store TDX_HCALL_ flags on stack instead of RBP.

Also, on IRC you mentioned that per TDX spec, BP is a valid argument
register too and you were going to raise this and get it fixed, TDX
hypercalls must not use BP to pass data.

2023-01-31 08:39:15

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

On Tue, Jan 31, 2023 at 09:34:12AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> > __tdx_hypercall() messes up RBP.
> >
> > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> >
> > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
>
> Also, on IRC you mentioned that per TDX spec, BP is a valid argument
> register too and you were going to raise this and get it fixed, TDX
> hypercalls must not use BP to pass data.

I've raised the question yesterday. No progress so far. It will take time
to get it into the public spec.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-01-31 08:57:12

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

On Tue, Jan 31, 2023 at 09:32:37AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that

Oops, just noticed a typo. s/in/is/

> > __tdx_hypercall() messes up RBP.
> >
> > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> >
> > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> > Link: https://lore.kernel.org/all/[email protected]
> > Reported-by: kernel test robot <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > ---
> >
> > The patch is against tip/x86/tdx. tip/sched/core removes
> > TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
> > after that.
>
> Right, this should work. But it does leave me wondering, should we
> perhaps strive to completely remove the flags thing and move to
> __tdx_hypercall() and __tdx_hypercall_ret() or something? That is,
> simply have two different functions, one with and one without return
> data.
>
> It should be trivial to generate that without actual code duplication.

Yeah, that's doable. I will give it a try. I guess on top this one (plus
sched/core changes) should be.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-01-31 09:40:06

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

On Tue, Jan 31, 2023 at 11:39:01AM +0300, Kirill A. Shutemov wrote:
> On Tue, Jan 31, 2023 at 09:34:12AM +0100, Peter Zijlstra wrote:
> > On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> > > __tdx_hypercall() messes up RBP.
> > >
> > > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> > >
> > > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
> >
> > Also, on IRC you mentioned that per TDX spec, BP is a valid argument
> > register too and you were going to raise this and get it fixed, TDX
> > hypercalls must not use BP to pass data.
>
> I've raised the question yesterday. No progress so far. It will take time
> to get it into the public spec.

Sure, just making sure it's not forgotten about. Thanks!