diff mbox series

x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall()

Message ID 20230130135354.27674-1-kirill.shutemov@linux.intel.com
State New
Headers
Series x86/tdx: Do not corrupt frame-pointer in __tdx_hypercall() |

Commit Message

Kirill A. Shutemov Jan. 30, 2023, 1:53 p.m. UTC 
  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 <kirill.shutemov@linux.intel.com>
Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
Link: https://lore.kernel.org/all/202301290255.buUBs99R-lkp@intel.com
Reported-by: kernel test robot <lkp@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---

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(-)

Comments

Peter Zijlstra Jan. 31, 2023, 8:32 a.m. UTC | #1 
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 <kirill.shutemov@linux.intel.com>
> Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> Link: https://lore.kernel.org/all/202301290255.buUBs99R-lkp@intel.com
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
> 
> 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.
Peter Zijlstra Jan. 31, 2023, 8:34 a.m. UTC | #2 
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.
Kirill A. Shutemov Jan. 31, 2023, 8:39 a.m. UTC | #3 
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.
Kirill A. Shutemov Jan. 31, 2023, 8:50 a.m. UTC | #4 
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 <kirill.shutemov@linux.intel.com>
> > Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> > Link: https://lore.kernel.org/all/202301290255.buUBs99R-lkp@intel.com
> > Reported-by: kernel test robot <lkp@intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > ---
> > 
> > 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.
Peter Zijlstra Jan. 31, 2023, 9:39 a.m. UTC | #5 
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!
diff mbox series

Patch

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