diff mbox series

x86/traps: Fix load_unaligned_zeropad() handling for shared TDX memory

Message ID 20230724230329.3970-1-kirill.shutemov@linux.intel.com
State New
Headers
Series x86/traps: Fix load_unaligned_zeropad() handling for shared TDX memory |

Commit Message

Kirill A. Shutemov July 24, 2023, 11:03 p.m. UTC 
  Commit c4e34dd99f2e ("x86: simplify load_unaligned_zeropad()
implementation") changes how exceptions around load_unaligned_zeropad()
handled. Kernel now relies on fault_address in fixup_exception() to
detect that the exception happened due to load_unaligned_zeropad().

It works fine for #PF, but breaks on #VE since no fault address is
passed down to fixup_exception().

Propagating ve_info.gla down to fixup_exception() resolves the issue.

See commit 1e7769653b06 ("x86/tdx: Handle load_unaligned_zeropad()
page-cross to a shared page") for more context.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Michael Kelley <mikelley@microsoft.com>
Fixes: c4e34dd99f2e ("x86: simplify load_unaligned_zeropad() implementation")
---
 arch/x86/kernel/traps.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Linus Torvalds July 25, 2023, 9:57 p.m. UTC | #1 
On Mon, 24 Jul 2023 at 16:03, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> Commit c4e34dd99f2e ("x86: simplify load_unaligned_zeropad()
> implementation") changes how exceptions around load_unaligned_zeropad()
> handled. Kernel now relies on fault_address in fixup_exception() to
> detect that the exception happened due to load_unaligned_zeropad().
>
> It works fine for #PF, but breaks on #VE since no fault address is
> passed down to fixup_exception().
>
> Propagating ve_info.gla down to fixup_exception() resolves the issue.

Ahh.

The faulting address was really only used as a sanity check, so I
guess another option would have been to just remove it.

We do need to get the "real" address anyway just to get the right
offset, and that comes from

        addr = (unsigned long) insn_get_addr_ref(&insn, regs);

and all we do is to then check they "yep, the faulting address is the
start of the next aligned word".

The faulting address in itself was never sufficient, since it has lost
the original offset in the previous page, and typically just points to
the first word in the next page.

That said, your patch looks fine to me, even if it might have been
easier to just remove the fault_addr thing entirely. And the
fault_addr verification is nice in that it validates our instruction
decoder result, so maybe it is all better this way.

I can apply this directly (having written that ex_handler_zeropad())
or wait for it to come from the x86 tree?

                 Linus
Dave Hansen July 25, 2023, 10:23 p.m. UTC | #2 
On 7/25/23 14:57, Linus Torvalds wrote:
> I can apply this directly (having written that ex_handler_zeropad())
> or wait for it to come from the x86 tree?

Feel free to pull it directly.  Looks fine to me as well:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Linus Torvalds July 25, 2023, 10:31 p.m. UTC | #3 
On Tue, 25 Jul 2023 at 15:23, Dave Hansen <dave.hansen@intel.com> wrote:
>
> Feel free to pull it directly.

Ok, done. I edited the commit message a bit to clarify that we don't
actually _rely_ on that fault_address thing, but other than a slight
wording change it's all good.

And for some reason 'b4' didn't pick up your Acked-by: (maybe it
hadn't made it to the lore DB yet since I got the reply directly) so I
added that manually.

         Linus
diff mbox series

Patch

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 58b1f208eff5..4a817d20ce3b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -697,9 +697,10 @@  static bool try_fixup_enqcmd_gp(void)
 }
 
 static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
-				    unsigned long error_code, const char *str)
+				    unsigned long error_code, const char *str,
+				    unsigned long address)
 {
-	if (fixup_exception(regs, trapnr, error_code, 0))
+	if (fixup_exception(regs, trapnr, error_code, address))
 		return true;
 
 	current->thread.error_code = error_code;
@@ -759,7 +760,7 @@  DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
 		goto exit;
 	}
 
-	if (gp_try_fixup_and_notify(regs, X86_TRAP_GP, error_code, desc))
+	if (gp_try_fixup_and_notify(regs, X86_TRAP_GP, error_code, desc, 0))
 		goto exit;
 
 	if (error_code)
@@ -1357,17 +1358,20 @@  DEFINE_IDTENTRY(exc_device_not_available)
 
 #define VE_FAULT_STR "VE fault"
 
-static void ve_raise_fault(struct pt_regs *regs, long error_code)
+static void ve_raise_fault(struct pt_regs *regs, long error_code,
+			   unsigned long address)
 {
 	if (user_mode(regs)) {
 		gp_user_force_sig_segv(regs, X86_TRAP_VE, error_code, VE_FAULT_STR);
 		return;
 	}
 
-	if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code, VE_FAULT_STR))
+	if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code,
+				    VE_FAULT_STR, address)) {
 		return;
+	}
 
-	die_addr(VE_FAULT_STR, regs, error_code, 0);
+	die_addr(VE_FAULT_STR, regs, error_code, address);
 }
 
 /*
@@ -1431,7 +1435,7 @@  DEFINE_IDTENTRY(exc_virtualization_exception)
 	 * it successfully, treat it as #GP(0) and handle it.
 	 */
 	if (!tdx_handle_virt_exception(regs, &ve))
-		ve_raise_fault(regs, 0);
+		ve_raise_fault(regs, 0, ve.gla);
 
 	cond_local_irq_disable(regs);
 }