load_unaligned_zeropad() can lead to unwanted loads across page boundaries.
The unwanted loads are typically harmless. But, they might be made to
totally unrelated or even unmapped memory. load_unaligned_zeropad()
relies on exception fixup (#PF, #GP and now #VE) to recover from these
unwanted loads.
In TDX guests, the second page can be shared page and VMM may configure
it to trigger #VE.
Kernel assumes that #VE on a shared page is MMIO access and tries to
decode instruction to handle it. In case of load_unaligned_zeropad() it
may result in confusion as it is not MMIO access.
Fix it by detecting unaligned MMIO accesses (it includes page-crossings)
and fail them. load_unaligned_zeropad() will recover using exception
fixups.
The issue was discovered by analysis. It was not triggered during the
testing.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 94e447e7f103..4e566ed67db8 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -331,6 +331,17 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
return -EINVAL;
}
+ /*
+ * MMIO accesses suppose to be naturally aligned and therefore never
+ * cross a page boundary. Seeing unaligned accesses indicates a bug or
+ * load_unaligned_zeropad() that steps into unmapped shared page.
+ *
+ * In both cases fail the #VE handling. load_unaligned_zeropad() will
+ * recover using exception fixups.
+ */
+ if ((unsigned long)insn_get_addr_ref(&insn, regs) % size)
+ return -EFAULT;
+
/* Handle writes first */
switch (mmio) {
case MMIO_WRITE:
--
2.35.1
On Thu, May 26, 2022 at 09:20:56AM -0700, Dave Hansen wrote:
> On 5/24/22 15:10, Kirill A. Shutemov wrote:
> > + /*
> > + * MMIO accesses suppose to be naturally aligned and therefore never
> > + * cross a page boundary. Seeing unaligned accesses indicates a bug or
> > + * load_unaligned_zeropad() that steps into unmapped shared page.
>
> Wait a sec though...
>
> We've been talking all along about how MMIO accesses are in some cases
> just plain old compiler-generated memory accesses. It's *probably* bad
> code that does this, but it's not necessarily a bug.
Compiler-generated memory accesses tend to be aligned too. You need to do
something make them unalinged, like __packed or pointer trickery.
> It's kinda like the split lock detection patches. Those definitely
> found some stupid stuff, but it wasn't anything that I would have called
> an outright bug. Plus, in those cases, folks had explicitly opted in to
> more crashes on stupid stuff.
>
> That stupid stuff _might_ be rare enough that it's still OK to just punt
> on it and not emulate the instruction (aka. crash). Or, to say that TDX
> guests are opting in to being more fragile, just like with split lock
> detection.
I think it is reasonable to expect that TDX user value its data security
higher than uptime.
And I'm not sure that compare unaligned MMIO access to split-lock is fair.
Split-lock is performance hit, but semantics is defined. In unalgined MMIO
case, I think the behaviour is not defined: it is not clear what memory
reqested should be issued on the memory bus in case of byte-algined 4-byte
access. It can make a difference on device side.
> But, either of those would call for a very different comment.
Fair enough.
--
Kirill A. Shutemov
On 5/24/22 15:10, Kirill A. Shutemov wrote:
> + /*
> + * MMIO accesses suppose to be naturally aligned and therefore never
> + * cross a page boundary. Seeing unaligned accesses indicates a bug or
> + * load_unaligned_zeropad() that steps into unmapped shared page.
Wait a sec though...
We've been talking all along about how MMIO accesses are in some cases
just plain old compiler-generated memory accesses. It's *probably* bad
code that does this, but it's not necessarily a bug.
It's kinda like the split lock detection patches. Those definitely
found some stupid stuff, but it wasn't anything that I would have called
an outright bug. Plus, in those cases, folks had explicitly opted in to
more crashes on stupid stuff.
That stupid stuff _might_ be rare enough that it's still OK to just punt
on it and not emulate the instruction (aka. crash). Or, to say that TDX
guests are opting in to being more fragile, just like with split lock
detection.
But, either of those would call for a very different comment.
On Thu, May 26, 2022, Dave Hansen wrote:
> On 5/26/22 13:36, Kirill A. Shutemov wrote:
> > On Thu, May 26, 2022 at 09:20:56AM -0700, Dave Hansen wrote:
> >> On 5/24/22 15:10, Kirill A. Shutemov wrote:
> >>> + /*
> >>> + * MMIO accesses suppose to be naturally aligned and therefore never
> >>> + * cross a page boundary. Seeing unaligned accesses indicates a bug or
> >>> + * load_unaligned_zeropad() that steps into unmapped shared page.
> >> Wait a sec though...
> >>
> >> We've been talking all along about how MMIO accesses are in some cases
> >> just plain old compiler-generated memory accesses. It's *probably* bad
> >> code that does this, but it's not necessarily a bug.
> > Compiler-generated memory accesses tend to be aligned too. You need to do
> > something make them unalinged, like __packed or pointer trickery.
>
> I totally agree. But, the point is that __packed or pointer trickery is
> goofy, but it's not necessarily a bug. This might crash the kernel on
> goofy stuff, not bugs.
Yeah, I don't think it's worth exploding on unaligned accesses, it's specifically
page splits that are a mess and are an absolutely nightmare to handle. E.g. for
VirtIO kicks, the data and page offset are completely ignored/irrelevant, so a
multi-byte write to any random byte in the page should work, even though it's all
kinds of goofy.
On 5/26/22 13:36, Kirill A. Shutemov wrote:
> On Thu, May 26, 2022 at 09:20:56AM -0700, Dave Hansen wrote:
>> On 5/24/22 15:10, Kirill A. Shutemov wrote:
>>> + /*
>>> + * MMIO accesses suppose to be naturally aligned and therefore never
>>> + * cross a page boundary. Seeing unaligned accesses indicates a bug or
>>> + * load_unaligned_zeropad() that steps into unmapped shared page.
>> Wait a sec though...
>>
>> We've been talking all along about how MMIO accesses are in some cases
>> just plain old compiler-generated memory accesses. It's *probably* bad
>> code that does this, but it's not necessarily a bug.
> Compiler-generated memory accesses tend to be aligned too. You need to do
> something make them unalinged, like __packed or pointer trickery.
I totally agree. But, the point is that __packed or pointer trickery is
goofy, but it's not necessarily a bug. This might crash the kernel on
goofy stuff, not bugs.