LinuxLists.cc - [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted memory

[permalink] [raw]

Subject: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

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.

But, this approach does not work for unaccepted memory. For TDX, a load
from unaccepted memory will not lead to a recoverable exception within
the guest. The guest will exit to the VMM where the only recourse is to
terminate the guest.

There are two parts to fix this issue and comprehensively avoid access
to unaccepted memory. Together these ensure that an extra "guard" page
is accepted in addition to the memory that needs to be used.

1. Implicitly extend the range_contains_unaccepted_memory(start, end)
checks up to end+unit_size if 'end' is aligned on a unit_size
boundary.
2. Implicitly extend accept_memory(start, end) to end+unit_size if 'end'
is aligned on a unit_size boundary.

Side note: This leads to something strange. Pages which were accepted
at boot, marked by the firmware as accepted and will never
_need_ to be accepted might be on unaccepted_pages list
This is a cue to ensure that the next page is accepted
before 'page' can be used.

This is an actual, real-world problem which was discovered during TDX
testing.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
---
drivers/firmware/efi/unaccepted_memory.c | 35 ++++++++++++++++++++++++
1 file changed, 35 insertions(+)

diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index bb91c41f76fb..3d1ca60916dd 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -37,6 +37,34 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
start -= unaccepted->phys_base;
end -= unaccepted->phys_base;

+ /*
+ * 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.
+ *
+ * But, this approach does not work for unaccepted memory. For TDX, a
+ * load from unaccepted memory will not lead to a recoverable exception
+ * within the guest. The guest will exit to the VMM where the only
+ * recourse is to terminate the guest.
+ *
+ * There are two parts to fix this issue and comprehensively avoid
+ * access to unaccepted memory. Together these ensure that an extra
+ * "guard" page is accepted in addition to the memory that needs to be
+ * used:
+ *
+ * 1. Implicitly extend the range_contains_unaccepted_memory(start, end)
+ * checks up to end+unit_size if 'end' is aligned on a unit_size
+ * boundary.
+ *
+ * 2. Implicitly extend accept_memory(start, end) to end+unit_size if
+ * 'end' is aligned on a unit_size boundary. (immediately following
+ * this comment)
+ */
+ if (!(end % unit_size))
+ end += unit_size;
+
/* Make sure not to overrun the bitmap */
if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
end = unaccepted->size * unit_size * BITS_PER_BYTE;
@@ -84,6 +112,13 @@ bool range_contains_unaccepted_memory(phys_addr_t start, phys_addr_t end)
start -= unaccepted->phys_base;
end -= unaccepted->phys_base;

+ /*
+ * Also consider the unaccepted state of the *next* page. See fix #1 in
+ * the comment on load_unaligned_zeropad() in accept_memory().
+ */
+ if (!(end % unit_size))
+ end += unit_size;
+
/* Make sure not to overrun the bitmap */
if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
end = unaccepted->size * unit_size * BITS_PER_BYTE;
--
2.39.3

2023-05-13 22:21:43

[permalink] [raw]

Subject: [PATCHv11 7/9] x86/tdx: Make _tdx_hypercall() and __tdx_module_call() available in boot stub

Memory acceptance requires a hypercall and one or multiple module calls.

Make helpers for the calls available in boot stub. It has to accept
memory where kernel image and initrd are placed.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 32 -------------------
arch/x86/include/asm/shared/tdx.h | 51 +++++++++++++++++++++++++++++++
arch/x86/include/asm/tdx.h | 19 ------------
3 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index e146b599260f..e6f4c2758a68 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -14,20 +14,6 @@
#include <asm/insn-eval.h>
#include <asm/pgtable.h>

-/* TDX module Call Leaf IDs */
-#define TDX_GET_INFO 1
-#define TDX_GET_VEINFO 3
-#define TDX_GET_REPORT 4
-#define TDX_ACCEPT_PAGE 6
-#define TDX_WR 8
-
-/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
-#define TDCS_NOTIFY_ENABLES 0x9100000000000010
-
-/* TDX hypercall Leaf IDs */
-#define TDVMCALL_MAP_GPA 0x10001
-#define TDVMCALL_REPORT_FATAL_ERROR 0x10003
-
/* MMIO direction */
#define EPT_READ 0
#define EPT_WRITE 1
@@ -51,24 +37,6 @@

#define TDREPORT_SUBTYPE_0 0

-/*
- * Wrapper for standard use of __tdx_hypercall with no output aside from
- * return code.
- */
-static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
-{
- struct tdx_hypercall_args args = {
- .r10 = TDX_HYPERCALL_STANDARD,
- .r11 = fn,
- .r12 = r12,
- .r13 = r13,
- .r14 = r14,
- .r15 = r15,
- };
-
- return __tdx_hypercall(&args);
-}
-
/* Called from __tdx_hypercall() for unrecoverable failure */
noinstr void __tdx_hypercall_failed(void)
{
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 2631e01f6e0f..1ff0ee822961 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -10,6 +10,20 @@
#define TDX_CPUID_LEAF_ID 0x21
#define TDX_IDENT "IntelTDX "

+/* TDX module Call Leaf IDs */
+#define TDX_GET_INFO 1
+#define TDX_GET_VEINFO 3
+#define TDX_GET_REPORT 4
+#define TDX_ACCEPT_PAGE 6
+#define TDX_WR 8
+
+/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
+#define TDCS_NOTIFY_ENABLES 0x9100000000000010
+
+/* TDX hypercall Leaf IDs */
+#define TDVMCALL_MAP_GPA 0x10001
+#define TDVMCALL_REPORT_FATAL_ERROR 0x10003
+
#ifndef __ASSEMBLY__

/*
@@ -37,8 +51,45 @@ struct tdx_hypercall_args {
u64 __tdx_hypercall(struct tdx_hypercall_args *args);
u64 __tdx_hypercall_ret(struct tdx_hypercall_args *args);

+/*
+ * Wrapper for standard use of __tdx_hypercall with no output aside from
+ * return code.
+ */
+static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = fn,
+ .r12 = r12,
+ .r13 = r13,
+ .r14 = r14,
+ .r15 = r15,
+ };
+
+ return __tdx_hypercall(&args);
+}
+
+
/* Called from __tdx_hypercall() for unrecoverable failure */
void __tdx_hypercall_failed(void);

+/*
+ * Used in __tdx_module_call() to gather the output registers' values of the
+ * TDCALL instruction when requesting services from the TDX module. This is a
+ * software only structure and not part of the TDX module/VMM ABI
+ */
+struct tdx_module_output {
+ u64 rcx;
+ u64 rdx;
+ u64 r8;
+ u64 r9;
+ u64 r10;
+ u64 r11;
+};
+
+/* Used to communicate with the TDX module */
+u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out);
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_SHARED_TDX_H */
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 28d889c9aa16..234197ec17e4 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -20,21 +20,6 @@

#ifndef __ASSEMBLY__

-/*
- * Used to gather the output registers values of the TDCALL and SEAMCALL
- * instructions when requesting services from the TDX module.
- *
- * This is a software only structure and not part of the TDX module/VMM ABI.
- */
-struct tdx_module_output {
- u64 rcx;
- u64 rdx;
- u64 r8;
- u64 r9;
- u64 r10;
- u64 r11;
-};
-
/*
* Used by the #VE exception handler to gather the #VE exception
* info from the TDX module. This is a software only structure
@@ -55,10 +40,6 @@ struct ve_info {

void __init tdx_early_init(void);

-/* Used to communicate with the TDX module */
-u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
- struct tdx_module_output *out);
-
void tdx_get_ve_info(struct ve_info *ve);

bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);
--
2.39.3

2023-05-16 18:26:29

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Sun, 14 May 2023 at 00:04, Kirill A. Shutemov
<[email protected]> wrote:
>
> 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.
>
> But, this approach does not work for unaccepted memory. For TDX, a load
> from unaccepted memory will not lead to a recoverable exception within
> the guest. The guest will exit to the VMM where the only recourse is to
> terminate the guest.
>

Does this mean that the kernel maps memory before accepting it? As
otherwise, I would assume that such an access would page fault inside
the guest before triggering an exception related to the unaccepted
state.

> There are two parts to fix this issue and comprehensively avoid access
> to unaccepted memory. Together these ensure that an extra "guard" page
> is accepted in addition to the memory that needs to be used.
>
> 1. Implicitly extend the range_contains_unaccepted_memory(start, end)
> checks up to end+unit_size if 'end' is aligned on a unit_size
> boundary.
> 2. Implicitly extend accept_memory(start, end) to end+unit_size if 'end'
> is aligned on a unit_size boundary.
>
> Side note: This leads to something strange. Pages which were accepted
> at boot, marked by the firmware as accepted and will never
> _need_ to be accepted might be on unaccepted_pages list
> This is a cue to ensure that the next page is accepted
> before 'page' can be used.
>
> This is an actual, real-world problem which was discovered during TDX
> testing.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Reviewed-by: Dave Hansen <[email protected]>
> ---
> drivers/firmware/efi/unaccepted_memory.c | 35 ++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
> index bb91c41f76fb..3d1ca60916dd 100644
> --- a/drivers/firmware/efi/unaccepted_memory.c
> +++ b/drivers/firmware/efi/unaccepted_memory.c
> @@ -37,6 +37,34 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
> start -= unaccepted->phys_base;
> end -= unaccepted->phys_base;
>
> + /*
> + * 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.
> + *
> + * But, this approach does not work for unaccepted memory. For TDX, a
> + * load from unaccepted memory will not lead to a recoverable exception
> + * within the guest. The guest will exit to the VMM where the only
> + * recourse is to terminate the guest.
> + *
> + * There are two parts to fix this issue and comprehensively avoid
> + * access to unaccepted memory. Together these ensure that an extra
> + * "guard" page is accepted in addition to the memory that needs to be
> + * used:
> + *
> + * 1. Implicitly extend the range_contains_unaccepted_memory(start, end)
> + * checks up to end+unit_size if 'end' is aligned on a unit_size
> + * boundary.
> + *
> + * 2. Implicitly extend accept_memory(start, end) to end+unit_size if
> + * 'end' is aligned on a unit_size boundary. (immediately following
> + * this comment)
> + */
> + if (!(end % unit_size))
> + end += unit_size;
> +
> /* Make sure not to overrun the bitmap */
> if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> end = unaccepted->size * unit_size * BITS_PER_BYTE;
> @@ -84,6 +112,13 @@ bool range_contains_unaccepted_memory(phys_addr_t start, phys_addr_t end)
> start -= unaccepted->phys_base;
> end -= unaccepted->phys_base;
>
> + /*
> + * Also consider the unaccepted state of the *next* page. See fix #1 in
> + * the comment on load_unaligned_zeropad() in accept_memory().
> + */
> + if (!(end % unit_size))
> + end += unit_size;
> +
> /* Make sure not to overrun the bitmap */
> if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> end = unaccepted->size * unit_size * BITS_PER_BYTE;
> --
> 2.39.3
>

2023-05-16 18:43:43

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Tue, 16 May 2023 at 20:27, Dave Hansen <[email protected]> wrote:
>
> On 5/16/23 11:08, Ard Biesheuvel wrote:
> >> But, this approach does not work for unaccepted memory. For TDX, a load
> >> from unaccepted memory will not lead to a recoverable exception within
> >> the guest. The guest will exit to the VMM where the only recourse is to
> >> terminate the guest.
> >>
> > Does this mean that the kernel maps memory before accepting it? As
> > otherwise, I would assume that such an access would page fault inside
> > the guest before triggering an exception related to the unaccepted
> > state.
>
> Yes, the kernel maps memory before accepting it (modulo things like
> DEBUG_PAGEALLOC).
>

OK, and so the architecture stipulates that prefetching or other
speculative accesses must never deliver exceptions to the host
regarding such ranges?

If this all works as it should, then I'm ok with leaving this here,
but I imagine we may want to factor out some arch specific policy here
in the future, as I don't think this would work the same on ARM.

2023-05-16 18:45:33

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Tue, May 16, 2023 at 08:08:37PM +0200, Ard Biesheuvel wrote:
> On Sun, 14 May 2023 at 00:04, Kirill A. Shutemov
> <[email protected]> wrote:
> >
> > 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.
> >
> > But, this approach does not work for unaccepted memory. For TDX, a load
> > from unaccepted memory will not lead to a recoverable exception within
> > the guest. The guest will exit to the VMM where the only recourse is to
> > terminate the guest.
> >
>
> Does this mean that the kernel maps memory before accepting it? As
> otherwise, I would assume that such an access would page fault inside
> the guest before triggering an exception related to the unaccepted
> state.

Yes, kernel maps all memory into direct mapping whether it is accepted or
not [yet].

The problem is that access of unaccepted memory is not page fault on TDX.
It causes unrecoverable exit to the host so it must not happen to
legitimate accesses, including load_unaligned_zeropad() overshoot.

For context: there's a way configure TDX environment to trigger #VE on
such accesses and it is default. But Linux requires such #VEs to be
disabled as it opens attack vector from the host to the guest: host can
pull any private page from under kernel at any point and trigger such #VE.
If it happens in just a right time in syscall gap or NMI entry code it can
be exploitable.

See also commits 9a22bf6debbf and 373e715e31bf.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-16 18:47:28

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On 5/16/23 11:08, Ard Biesheuvel wrote:
>> But, this approach does not work for unaccepted memory. For TDX, a load
>> from unaccepted memory will not lead to a recoverable exception within
>> the guest. The guest will exit to the VMM where the only recourse is to
>> terminate the guest.
>>
> Does this mean that the kernel maps memory before accepting it? As
> otherwise, I would assume that such an access would page fault inside
> the guest before triggering an exception related to the unaccepted
> state.

Yes, the kernel maps memory before accepting it (modulo things like
DEBUG_PAGEALLOC).

2023-05-16 19:31:34

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Tue, May 16, 2023 at 08:35:27PM +0200, Ard Biesheuvel wrote:
> On Tue, 16 May 2023 at 20:27, Dave Hansen <[email protected]> wrote:
> >
> > On 5/16/23 11:08, Ard Biesheuvel wrote:
> > >> But, this approach does not work for unaccepted memory. For TDX, a load
> > >> from unaccepted memory will not lead to a recoverable exception within
> > >> the guest. The guest will exit to the VMM where the only recourse is to
> > >> terminate the guest.
> > >>
> > > Does this mean that the kernel maps memory before accepting it? As
> > > otherwise, I would assume that such an access would page fault inside
> > > the guest before triggering an exception related to the unaccepted
> > > state.
> >
> > Yes, the kernel maps memory before accepting it (modulo things like
> > DEBUG_PAGEALLOC).
> >
>
> OK, and so the architecture stipulates that prefetching or other
> speculative accesses must never deliver exceptions to the host
> regarding such ranges?
>
> If this all works as it should, then I'm ok with leaving this here,
> but I imagine we may want to factor out some arch specific policy here
> in the future, as I don't think this would work the same on ARM.

Even if other architectures don't need this, it is harmless: we just
accept one unit ahead of time.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-16 20:15:32

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On 5/16/23 11:35, Ard Biesheuvel wrote:
>>> Does this mean that the kernel maps memory before accepting it? As
>>> otherwise, I would assume that such an access would page fault inside
>>> the guest before triggering an exception related to the unaccepted
>>> state.
>> Yes, the kernel maps memory before accepting it (modulo things like
>> DEBUG_PAGEALLOC).
>>
> OK, and so the architecture stipulates that prefetching or other
> speculative accesses must never deliver exceptions to the host
> regarding such ranges?

I don't know of anywhere that this is explicitly written. It's probably
implicit _somewhere_ in the reams of VMX/TDX and base SDM docs, but heck
if I know where it is. :)

If this is something anyone wants to see added to the SEPT_VE_DISABLE
documentation, please speak up. I don't think it would be hard to get
it added and provide an explicit guarantee.

2023-05-16 22:12:39

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Tue, May 16, 2023 at 01:03:32PM -0700, Dave Hansen wrote:
> On 5/16/23 11:35, Ard Biesheuvel wrote:
> >>> Does this mean that the kernel maps memory before accepting it? As
> >>> otherwise, I would assume that such an access would page fault inside
> >>> the guest before triggering an exception related to the unaccepted
> >>> state.
> >> Yes, the kernel maps memory before accepting it (modulo things like
> >> DEBUG_PAGEALLOC).
> >>
> > OK, and so the architecture stipulates that prefetching or other
> > speculative accesses must never deliver exceptions to the host
> > regarding such ranges?
>
> I don't know of anywhere that this is explicitly written. It's probably
> implicit _somewhere_ in the reams of VMX/TDX and base SDM docs, but heck
> if I know where it is. :)

It is not specific to TDX: on x86 (and all architectures with precise
exceptions) exception handling is delayed until instruction retirement and
will not happen if speculation turned out to be wrong. And prefetching
never generates exceptions.

But I failed to find right away in 5000+ pages of Intel Software
Developer’s Manual. :/

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-16 22:15:28

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On 5/16/23 14:52, Kirill A. Shutemov wrote:
> On Tue, May 16, 2023 at 01:03:32PM -0700, Dave Hansen wrote:
>> On 5/16/23 11:35, Ard Biesheuvel wrote:
>>>>> Does this mean that the kernel maps memory before accepting it? As
>>>>> otherwise, I would assume that such an access would page fault inside
>>>>> the guest before triggering an exception related to the unaccepted
>>>>> state.
>>>> Yes, the kernel maps memory before accepting it (modulo things like
>>>> DEBUG_PAGEALLOC).
>>>>
>>> OK, and so the architecture stipulates that prefetching or other
>>> speculative accesses must never deliver exceptions to the host
>>> regarding such ranges?
>> I don't know of anywhere that this is explicitly written. It's probably
>> implicit _somewhere_ in the reams of VMX/TDX and base SDM docs, but heck
>> if I know where it is. ????
> It is not specific to TDX: on x86 (and all architectures with precise
> exceptions) exception handling is delayed until instruction retirement and
> will not happen if speculation turned out to be wrong. And prefetching
> never generates exceptions.

Not to be Debbie Downer too much here, but it's *totally* possible for
speculative execution to go read memory that causes you to machine
check. We've had such bugs in Linux.

We just happen to be lucky in this case that the unaccepted memory
exceptions don't generate machine checks *AND* TDX hardware does not
machine check on speculative accesses that would _just_ violate TDX
security properties.

You're right for normal, sane exceptions, though.

2023-05-16 22:24:54

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On Wed, 17 May 2023 at 00:00, Dave Hansen <[email protected]> wrote:
>
> On 5/16/23 14:52, Kirill A. Shutemov wrote:
> > On Tue, May 16, 2023 at 01:03:32PM -0700, Dave Hansen wrote:
> >> On 5/16/23 11:35, Ard Biesheuvel wrote:
> >>>>> Does this mean that the kernel maps memory before accepting it? As
> >>>>> otherwise, I would assume that such an access would page fault inside
> >>>>> the guest before triggering an exception related to the unaccepted
> >>>>> state.
> >>>> Yes, the kernel maps memory before accepting it (modulo things like
> >>>> DEBUG_PAGEALLOC).
> >>>>
> >>> OK, and so the architecture stipulates that prefetching or other
> >>> speculative accesses must never deliver exceptions to the host
> >>> regarding such ranges?
> >> I don't know of anywhere that this is explicitly written. It's probably
> >> implicit _somewhere_ in the reams of VMX/TDX and base SDM docs, but heck
> >> if I know where it is. ????
> > It is not specific to TDX: on x86 (and all architectures with precise
> > exceptions) exception handling is delayed until instruction retirement and
> > will not happen if speculation turned out to be wrong. And prefetching
> > never generates exceptions.
>
> Not to be Debbie Downer too much here, but it's *totally* possible for
> speculative execution to go read memory that causes you to machine
> check. We've had such bugs in Linux.
>
> We just happen to be lucky in this case that the unaccepted memory
> exceptions don't generate machine checks *AND* TDX hardware does not
> machine check on speculative accesses that would _just_ violate TDX
> security properties.
>
> You're right for normal, sane exceptions, though.

Same thing on ARM, although I'd have to check their RME stuff in more
detail to see how it behaves in this particular case.

But Kyrill is right that it doesn't really matter for the logic in
this patch - it just accepts some additional pages. The relevant
difference between implementations will likely be whether unaccepted
memory gets mapped beforehand in the first place, but we'll deal with
that once we have to.

As long as we only accept memory that appears in the bitmap as
'unaccepted', this kind of rounding seems safe and reasonable to me.

Reviewed-by: Ard Biesheuvel <[email protected]>

2023-05-16 22:48:02

[permalink] [raw]

Subject: Re: [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted memory

On 5/13/23 17:04, Kirill A. Shutemov wrote:
> UEFI Specification version 2.9 introduces the concept of memory
> acceptance: some Virtual Machine platforms, such as Intel TDX or AMD
> SEV-SNP, requiring memory to be accepted before it can be used by the
> guest. Accepting happens via a protocol specific for the Virtual
> Machine platform.
>
> Accepting memory is costly and it makes VMM allocate memory for the
> accepted guest physical address range. It's better to postpone memory
> acceptance until memory is needed. It lowers boot time and reduces
> memory overhead.
>
> The kernel needs to know what memory has been accepted. Firmware
> communicates this information via memory map: a new memory type --
> EFI_UNACCEPTED_MEMORY -- indicates such memory.
>
> Range-based tracking works fine for firmware, but it gets bulky for
> the kernel: e820 has to be modified on every page acceptance. It leads
> to table fragmentation, but there's a limited number of entries in the
> e820 table
>
> Another option is to mark such memory as usable in e820 and track if the
> range has been accepted in a bitmap. One bit in the bitmap represents
> 2MiB in the address space: one 4k page is enough to track 64GiB or
> physical address space.
>
> In the worst-case scenario -- a huge hole in the middle of the
> address space -- It needs 256MiB to handle 4PiB of the address
> space.
>
> Any unaccepted memory that is not aligned to 2M gets accepted upfront.
>
> The approach lowers boot time substantially. Boot to shell is ~2.5x
> faster for 4G TDX VM and ~4x faster for 64G.
>
> TDX-specific code isolated from the core of unaccepted memory support. It
> supposed to help to plug-in different implementation of unaccepted memory
> such as SEV-SNP.
>
> -- Fragmentation study --
>
> Vlastimil and Mel were concern about effect of unaccepted memory on
> fragmentation prevention measures in page allocator. I tried to evaluate
> it, but it is tricky. As suggested I tried to run multiple parallel kernel
> builds and follow how often kmem:mm_page_alloc_extfrag gets hit.
>
> See results in the v9 of the patchset[1][2]
>
> [1] https://lore.kernel.org/all/[email protected]
> [2] https://lore.kernel.org/all/[email protected]
>
> --
>
> The tree can be found here:
>
> https://github.com/intel/tdx.git guest-unaccepted-memory

I get some failures when building without TDX support selected in my
kernel config after adding unaccepted memory support for SNP:

In file included from arch/x86/boot/compressed/../../coco/tdx/tdx-shared.c:1,
from arch/x86/boot/compressed/tdx-shared.c:2:
./arch/x86/include/asm/tdx.h: In function ‘tdx_kvm_hypercall’:
./arch/x86/include/asm/tdx.h:72:17: error: ‘ENODEV’ undeclared (first use in this function)
72 | return -ENODEV;
| ^~~~~~
./arch/x86/include/asm/tdx.h:72:17: note: each undeclared identifier is reported only once for each function it appears in

Adding an include for linux/errno.h gets past that error, but then
I get the following:

ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_enc_status_changed_phys':
tdx-shared.c:(.text+0x42): undefined reference to `__tdx_hypercall'
ld: tdx-shared.c:(.text+0x7f): undefined reference to `__tdx_module_call'
ld: tdx-shared.c:(.text+0xce): undefined reference to `__tdx_module_call'
ld: tdx-shared.c:(.text+0x13b): undefined reference to `__tdx_module_call'
ld: tdx-shared.c:(.text+0x153): undefined reference to `cc_mkdec'
ld: tdx-shared.c:(.text+0x15d): undefined reference to `cc_mkdec'
ld: tdx-shared.c:(.text+0x18e): undefined reference to `__tdx_hypercall'
ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__tdx_hypercall' isn't defined
ld: final link failed: bad value

So it looks like arch/x86/boot/compressed/tdx-shared.c is being
built, while arch/x86/boot/compressed/tdx.c isn't.

After setting TDX in the kernel config, I can build successfully, but
I'm running into an error when trying to accept memory during
decompression.

In drivers/firmware/efi/libstub/unaccepted_memory.c, I can see that the
unaccepted_table is allocated, but when accept_memory() is invoked the
table address is now zero. I thought maybe it had to do with bss, but even
putting it in the .data section didn't help. I'll keep digging, but if you
have any ideas, that would be great.

Thanks,
Tom

>
> The patchset depends on MAX_ORDER changes in MM tree.
>
> v11:
> - Restructure the code to make it less x86-specific (suggested by Ard):
> + use EFI configuration table instead of zero-page to pass down bitmap;
> + do not imply 1bit == 2M in bitmap;
> + move bulk of the code under driver/firmware/efi;
> - The bitmap only covers unaccpeted memory now. All memory that is not covered
> by the bitmap assumed accepted;
> - Reviewed-by from Ard;
> v10:
> - Restructure code around zones_with_unaccepted_pages static brach to avoid
> unnecessary function calls (Suggested by Vlastimil);
> - Drop mentions of PageUnaccepted();
> - Drop patches that add fake unaccepted memory support and sysfs handle to
> accept memory manually;
> - Add Reviewed-by from Vlastimil;
> v9:
> - Accept memory up to high watermark when kernel runs out of free memory;
> - Treat unaccepted memory as unusable in __zone_watermark_unusable_free();
> - Per-zone unaccepted memory accounting;
> - All pages on unaccepted list are MAX_ORDER now;
> - accept_memory=eager in cmdline to pre-accept memory during the boot;
> - Implement fake unaccepted memory;
> - Sysfs handle to accept memory manually;
> - Drop PageUnaccepted();
> - Rename unaccepted_pages static key to zones_with_unaccepted_pages;
> v8:
> - Rewrite core-mm support for unaccepted memory (patch 02/14);
> - s/UnacceptedPages/Unaccepted/ in meminfo;
> - Drop arch/x86/boot/compressed/compiler.h;
> - Fix build errors;
> - Adjust commit messages and comments;
> - Reviewed-bys from Dave and Borislav;
> - Rebased to tip/master.
> v7:
> - Rework meminfo counter to use PageUnaccepted() and move to generic code;
> - Fix range_contains_unaccepted_memory() on machines without unaccepted memory;
> - Add Reviewed-by from David;
> v6:
> - Fix load_unaligned_zeropad() on machine with unaccepted memory;
> - Clear PageUnaccepted() on merged pages, leaving it only on head;
> - Clarify error handling in allocate_e820();
> - Fix build with CONFIG_UNACCEPTED_MEMORY=y, but without TDX;
> - Disable kexec at boottime instead of build conflict;
> - Rebased to tip/master;
> - Spelling fixes;
> - Add Reviewed-by from Mike and David;
> v5:
> - Updates comments and commit messages;
> + Explain options for unaccepted memory handling;
> - Expose amount of unaccepted memory in /proc/meminfo
> - Adjust check in page_expected_state();
> - Fix error code handling in allocate_e820();
> - Centralize __pa()/__va() definitions in the boot stub;
> - Avoid includes from the main kernel in the boot stub;
> - Use an existing hole in boot_param for unaccepted_memory, instead of adding
> to the end of the structure;
> - Extract allocate_unaccepted_memory() form allocate_e820();
> - Complain if there's unaccepted memory, but kernel does not support it;
> - Fix vmstat counter;
> - Split up few preparatory patches;
> - Random readability adjustments;
> v4:
> - PageBuddyUnaccepted() -> PageUnaccepted;
> - Use separate page_type, not shared with offline;
> - Rework interface between core-mm and arch code;
> - Adjust commit messages;
> - Ack from Mike;
>
> Kirill A. Shutemov (9):
> mm: Add support for unaccepted memory
> efi/x86: Get full memory map in allocate_e820()
> efi/libstub: Implement support for unaccepted memory
> x86/boot/compressed: Handle unaccepted memory
> efi: Provide helpers for unaccepted memory
> efi/unaccepted: Avoid load_unaligned_zeropad() stepping into
> unaccepted memory
> x86/tdx: Make _tdx_hypercall() and __tdx_module_call() available in
> boot stub
> x86/tdx: Refactor try_accept_one()
> x86/tdx: Add unaccepted memory support
>
> arch/x86/Kconfig | 2 +
> arch/x86/boot/compressed/Makefile | 1 +
> arch/x86/boot/compressed/efi.h | 1 +
> arch/x86/boot/compressed/error.c | 19 ++
> arch/x86/boot/compressed/error.h | 1 +
> arch/x86/boot/compressed/kaslr.c | 35 ++-
> arch/x86/boot/compressed/mem.c | 42 ++++
> arch/x86/boot/compressed/misc.c | 6 +
> arch/x86/boot/compressed/misc.h | 6 +
> arch/x86/boot/compressed/tdx-shared.c | 2 +
> arch/x86/boot/compressed/tdx.c | 37 +++
> arch/x86/coco/tdx/Makefile | 2 +-
> arch/x86/coco/tdx/tdx-shared.c | 95 ++++++++
> arch/x86/coco/tdx/tdx.c | 118 +---------
> arch/x86/include/asm/efi.h | 2 +
> arch/x86/include/asm/shared/tdx.h | 53 +++++
> arch/x86/include/asm/tdx.h | 21 +-
> arch/x86/include/asm/unaccepted_memory.h | 23 ++
> drivers/base/node.c | 7 +
> drivers/firmware/efi/Kconfig | 14 ++
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/efi.c | 7 +
> drivers/firmware/efi/libstub/Makefile | 2 +
> drivers/firmware/efi/libstub/bitmap.c | 41 ++++
> drivers/firmware/efi/libstub/efistub.h | 6 +
> drivers/firmware/efi/libstub/find.c | 43 ++++
> .../firmware/efi/libstub/unaccepted_memory.c | 222 ++++++++++++++++++
> drivers/firmware/efi/libstub/x86-stub.c | 39 +--
> drivers/firmware/efi/unaccepted_memory.c | 138 +++++++++++
> fs/proc/meminfo.c | 5 +
> include/linux/efi.h | 13 +-
> include/linux/mm.h | 19 ++
> include/linux/mmzone.h | 8 +
> mm/internal.h | 1 +
> mm/memblock.c | 9 +
> mm/mm_init.c | 7 +
> mm/page_alloc.c | 173 ++++++++++++++
> mm/vmstat.c | 3 +
> 38 files changed, 1060 insertions(+), 164 deletions(-)
> create mode 100644 arch/x86/boot/compressed/mem.c
> create mode 100644 arch/x86/boot/compressed/tdx-shared.c
> create mode 100644 arch/x86/coco/tdx/tdx-shared.c
> create mode 100644 arch/x86/include/asm/unaccepted_memory.h
> create mode 100644 drivers/firmware/efi/libstub/bitmap.c
> create mode 100644 drivers/firmware/efi/libstub/find.c
> create mode 100644 drivers/firmware/efi/libstub/unaccepted_memory.c
> create mode 100644 drivers/firmware/efi/unaccepted_memory.c
>

2023-05-16 23:06:02

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On 5/16/23 11:33, Kirill A. Shutemov wrote:
> For context: there's a way configure TDX environment to trigger #VE on
> such accesses and it is default. But Linux requires such #VEs to be
> disabled as it opens attack vector from the host to the guest: host can
> pull any private page from under kernel at any point and trigger such #VE.
> If it happens in just a right time in syscall gap or NMI entry code it can
> be exploitable.

I'm kinda uncomfortable with saying it's exploitable.

It really boils down to not wanting to deal with managing a new IST
exception. While the NMI IST implementation is about as good as we can
get it, I believe there are still holes in it (even if we consider only
how it interacts with #MC). The more IST users we add, the more holes
there are.

You add the fact that an actual adversary can induce the exceptions
instead of (rare and mostly random) radiation that causes #MC, and it
makes me want to either curl up in a little ball or pursue a new career.

So, exploitable? Dunno. Do I want to touch an #VE/IST implementation?
No way, not with a 10 foot pole.

2023-05-16 23:56:43

[permalink] [raw]

Subject: Re: [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted memory

On Tue, May 16, 2023 at 05:41:55PM -0500, Tom Lendacky wrote:
> On 5/13/23 17:04, Kirill A. Shutemov wrote:
> > UEFI Specification version 2.9 introduces the concept of memory
> > acceptance: some Virtual Machine platforms, such as Intel TDX or AMD
> > SEV-SNP, requiring memory to be accepted before it can be used by the
> > guest. Accepting happens via a protocol specific for the Virtual
> > Machine platform.
> >
> > Accepting memory is costly and it makes VMM allocate memory for the
> > accepted guest physical address range. It's better to postpone memory
> > acceptance until memory is needed. It lowers boot time and reduces
> > memory overhead.
> >
> > The kernel needs to know what memory has been accepted. Firmware
> > communicates this information via memory map: a new memory type --
> > EFI_UNACCEPTED_MEMORY -- indicates such memory.
> >
> > Range-based tracking works fine for firmware, but it gets bulky for
> > the kernel: e820 has to be modified on every page acceptance. It leads
> > to table fragmentation, but there's a limited number of entries in the
> > e820 table
> >
> > Another option is to mark such memory as usable in e820 and track if the
> > range has been accepted in a bitmap. One bit in the bitmap represents
> > 2MiB in the address space: one 4k page is enough to track 64GiB or
> > physical address space.
> >
> > In the worst-case scenario -- a huge hole in the middle of the
> > address space -- It needs 256MiB to handle 4PiB of the address
> > space.
> >
> > Any unaccepted memory that is not aligned to 2M gets accepted upfront.
> >
> > The approach lowers boot time substantially. Boot to shell is ~2.5x
> > faster for 4G TDX VM and ~4x faster for 64G.
> >
> > TDX-specific code isolated from the core of unaccepted memory support. It
> > supposed to help to plug-in different implementation of unaccepted memory
> > such as SEV-SNP.
> >
> > -- Fragmentation study --
> >
> > Vlastimil and Mel were concern about effect of unaccepted memory on
> > fragmentation prevention measures in page allocator. I tried to evaluate
> > it, but it is tricky. As suggested I tried to run multiple parallel kernel
> > builds and follow how often kmem:mm_page_alloc_extfrag gets hit.
> >
> > See results in the v9 of the patchset[1][2]
> >
> > [1] https://lore.kernel.org/all/[email protected]
> > [2] https://lore.kernel.org/all/[email protected]
> >
> > --
> >
> > The tree can be found here:
> >
> > https://github.com/intel/tdx.git guest-unaccepted-memory
>
> I get some failures when building without TDX support selected in my
> kernel config after adding unaccepted memory support for SNP:
>
> In file included from arch/x86/boot/compressed/../../coco/tdx/tdx-shared.c:1,
> from arch/x86/boot/compressed/tdx-shared.c:2:
> ./arch/x86/include/asm/tdx.h: In function ‘tdx_kvm_hypercall’:
> ./arch/x86/include/asm/tdx.h:72:17: error: ‘ENODEV’ undeclared (first use in this function)
> 72 | return -ENODEV;
> | ^~~~~~
> ./arch/x86/include/asm/tdx.h:72:17: note: each undeclared identifier is reported only once for each function it appears in
>
> Adding an include for linux/errno.h gets past that error, but then
> I get the following:
>
> ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_enc_status_changed_phys':
> tdx-shared.c:(.text+0x42): undefined reference to `__tdx_hypercall'
> ld: tdx-shared.c:(.text+0x7f): undefined reference to `__tdx_module_call'
> ld: tdx-shared.c:(.text+0xce): undefined reference to `__tdx_module_call'
> ld: tdx-shared.c:(.text+0x13b): undefined reference to `__tdx_module_call'
> ld: tdx-shared.c:(.text+0x153): undefined reference to `cc_mkdec'
> ld: tdx-shared.c:(.text+0x15d): undefined reference to `cc_mkdec'
> ld: tdx-shared.c:(.text+0x18e): undefined reference to `__tdx_hypercall'
> ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__tdx_hypercall' isn't defined
> ld: final link failed: bad value
>
> So it looks like arch/x86/boot/compressed/tdx-shared.c is being
> built, while arch/x86/boot/compressed/tdx.c isn't.

Right. I think this should help:

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 78f67e0a2666..b13a58021086 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -106,8 +106,8 @@ ifdef CONFIG_X86_64
endif

vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
-vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
-vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o $(obj)/tdx-shared.o
+vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o $(obj)/tdx-shared.o
+vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o

vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o

> After setting TDX in the kernel config, I can build successfully, but
> I'm running into an error when trying to accept memory during
> decompression.
>
> In drivers/firmware/efi/libstub/unaccepted_memory.c, I can see that the
> unaccepted_table is allocated, but when accept_memory() is invoked the
> table address is now zero. I thought maybe it had to do with bss, but even
> putting it in the .data section didn't help. I'll keep digging, but if you
> have any ideas, that would be great.

Not right away. But maybe seeing your side of enabling would help.

--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-17 14:44:17

[permalink] [raw]

Subject: Re: [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted memory

On 5/16/23 18:22, Kirill A. Shutemov wrote:
> On Tue, May 16, 2023 at 05:41:55PM -0500, Tom Lendacky wrote:
>> On 5/13/23 17:04, Kirill A. Shutemov wrote:
>>> UEFI Specification version 2.9 introduces the concept of memory
>>> acceptance: some Virtual Machine platforms, such as Intel TDX or AMD
>>> SEV-SNP, requiring memory to be accepted before it can be used by the
>>> guest. Accepting happens via a protocol specific for the Virtual
>>> Machine platform.
>>>
>>> Accepting memory is costly and it makes VMM allocate memory for the
>>> accepted guest physical address range. It's better to postpone memory
>>> acceptance until memory is needed. It lowers boot time and reduces
>>> memory overhead.
>>>
>>> The kernel needs to know what memory has been accepted. Firmware
>>> communicates this information via memory map: a new memory type --
>>> EFI_UNACCEPTED_MEMORY -- indicates such memory.
>>>
>>> Range-based tracking works fine for firmware, but it gets bulky for
>>> the kernel: e820 has to be modified on every page acceptance. It leads
>>> to table fragmentation, but there's a limited number of entries in the
>>> e820 table
>>>
>>> Another option is to mark such memory as usable in e820 and track if the
>>> range has been accepted in a bitmap. One bit in the bitmap represents
>>> 2MiB in the address space: one 4k page is enough to track 64GiB or
>>> physical address space.
>>>
>>> In the worst-case scenario -- a huge hole in the middle of the
>>> address space -- It needs 256MiB to handle 4PiB of the address
>>> space.
>>>
>>> Any unaccepted memory that is not aligned to 2M gets accepted upfront.
>>>
>>> The approach lowers boot time substantially. Boot to shell is ~2.5x
>>> faster for 4G TDX VM and ~4x faster for 64G.
>>>
>>> TDX-specific code isolated from the core of unaccepted memory support. It
>>> supposed to help to plug-in different implementation of unaccepted memory
>>> such as SEV-SNP.
>>>
>>> -- Fragmentation study --
>>>
>>> Vlastimil and Mel were concern about effect of unaccepted memory on
>>> fragmentation prevention measures in page allocator. I tried to evaluate
>>> it, but it is tricky. As suggested I tried to run multiple parallel kernel
>>> builds and follow how often kmem:mm_page_alloc_extfrag gets hit.
>>>
>>> See results in the v9 of the patchset[1][2]
>>>
>>> [1] https://lore.kernel.org/all/[email protected]
>>> [2] https://lore.kernel.org/all/[email protected]
>>>
>>> --
>>>
>>> The tree can be found here:
>>>
>>> https://github.com/intel/tdx.git guest-unaccepted-memory
>>
>> I get some failures when building without TDX support selected in my
>> kernel config after adding unaccepted memory support for SNP:
>>
>> In file included from arch/x86/boot/compressed/../../coco/tdx/tdx-shared.c:1,
>> from arch/x86/boot/compressed/tdx-shared.c:2:
>> ./arch/x86/include/asm/tdx.h: In function ?tdx_kvm_hypercall?:
>> ./arch/x86/include/asm/tdx.h:72:17: error: ?ENODEV? undeclared (first use in this function)
>> 72 | return -ENODEV;
>> | ^~~~~~
>> ./arch/x86/include/asm/tdx.h:72:17: note: each undeclared identifier is reported only once for each function it appears in
>>
>> Adding an include for linux/errno.h gets past that error, but then
>> I get the following:
>>
>> ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_enc_status_changed_phys':
>> tdx-shared.c:(.text+0x42): undefined reference to `__tdx_hypercall'
>> ld: tdx-shared.c:(.text+0x7f): undefined reference to `__tdx_module_call'
>> ld: tdx-shared.c:(.text+0xce): undefined reference to `__tdx_module_call'
>> ld: tdx-shared.c:(.text+0x13b): undefined reference to `__tdx_module_call'
>> ld: tdx-shared.c:(.text+0x153): undefined reference to `cc_mkdec'
>> ld: tdx-shared.c:(.text+0x15d): undefined reference to `cc_mkdec'
>> ld: tdx-shared.c:(.text+0x18e): undefined reference to `__tdx_hypercall'
>> ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__tdx_hypercall' isn't defined
>> ld: final link failed: bad value
>>
>> So it looks like arch/x86/boot/compressed/tdx-shared.c is being
>> built, while arch/x86/boot/compressed/tdx.c isn't.
>
> Right. I think this should help:
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 78f67e0a2666..b13a58021086 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -106,8 +106,8 @@ ifdef CONFIG_X86_64
> endif
>
> vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> -vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
> -vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o $(obj)/tdx-shared.o
> +vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o $(obj)/tdx-shared.o
> +vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o
>
> vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
>
>> After setting TDX in the kernel config, I can build successfully, but
>> I'm running into an error when trying to accept memory during
>> decompression.
>>
>> In drivers/firmware/efi/libstub/unaccepted_memory.c, I can see that the
>> unaccepted_table is allocated, but when accept_memory() is invoked the
>> table address is now zero. I thought maybe it had to do with bss, but even
>> putting it in the .data section didn't help. I'll keep digging, but if you
>> have any ideas, that would be great.
>
> Not right away. But maybe seeing your side of enabling would help.

Let me get something pushed up where you can access it and I'll also send
you my kernel config.

In the mean time I added the following and everything worked. But I'm not
sure how acceptable it is to always be checking for the table when the
value is zero is.

diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index f4642c4f25dd..8c5632ab1208 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -183,8 +183,13 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
unsigned long bitmap_size;
u64 unit_size;

- if (!unaccepted_table)
- return;
+ if (!unaccepted_table) {
+ efi_guid_t unaccepted_table_guid = LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
+
+ unaccepted_table = get_efi_config_table(unaccepted_table_guid);
+ if (!unaccepted_table)
+ return;
+ }

unit_size = unaccepted_table->unit_size;

Thanks,
Tom

>

2023-05-17 16:17:29

[permalink] [raw]

Subject: Re: [PATCHv11 6/9] efi/unaccepted: Avoid load_unaligned_zeropad() stepping into unaccepted memory

On 5/13/23 17:04, Kirill A. Shutemov wrote:
> 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.
>
> But, this approach does not work for unaccepted memory. For TDX, a load
> from unaccepted memory will not lead to a recoverable exception within
> the guest. The guest will exit to the VMM where the only recourse is to
> terminate the guest.
>
> There are two parts to fix this issue and comprehensively avoid access
> to unaccepted memory. Together these ensure that an extra "guard" page
> is accepted in addition to the memory that needs to be used.
>
> 1. Implicitly extend the range_contains_unaccepted_memory(start, end)
> checks up to end+unit_size if 'end' is aligned on a unit_size
> boundary.
> 2. Implicitly extend accept_memory(start, end) to end+unit_size if 'end'
> is aligned on a unit_size boundary.
>
> Side note: This leads to something strange. Pages which were accepted
> at boot, marked by the firmware as accepted and will never
> _need_ to be accepted might be on unaccepted_pages list
> This is a cue to ensure that the next page is accepted
> before 'page' can be used.
>
> This is an actual, real-world problem which was discovered during TDX
> testing.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Reviewed-by: Dave Hansen <[email protected]>

Reviewed-by: Tom Lendacky <[email protected]>

> ---
> drivers/firmware/efi/unaccepted_memory.c | 35 ++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
> index bb91c41f76fb..3d1ca60916dd 100644
> --- a/drivers/firmware/efi/unaccepted_memory.c
> +++ b/drivers/firmware/efi/unaccepted_memory.c
> @@ -37,6 +37,34 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
> start -= unaccepted->phys_base;
> end -= unaccepted->phys_base;
>
> + /*
> + * 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.
> + *
> + * But, this approach does not work for unaccepted memory. For TDX, a
> + * load from unaccepted memory will not lead to a recoverable exception
> + * within the guest. The guest will exit to the VMM where the only
> + * recourse is to terminate the guest.
> + *
> + * There are two parts to fix this issue and comprehensively avoid
> + * access to unaccepted memory. Together these ensure that an extra
> + * "guard" page is accepted in addition to the memory that needs to be
> + * used:
> + *
> + * 1. Implicitly extend the range_contains_unaccepted_memory(start, end)
> + * checks up to end+unit_size if 'end' is aligned on a unit_size
> + * boundary.
> + *
> + * 2. Implicitly extend accept_memory(start, end) to end+unit_size if
> + * 'end' is aligned on a unit_size boundary. (immediately following
> + * this comment)
> + */
> + if (!(end % unit_size))
> + end += unit_size;
> +
> /* Make sure not to overrun the bitmap */
> if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> end = unaccepted->size * unit_size * BITS_PER_BYTE;
> @@ -84,6 +112,13 @@ bool range_contains_unaccepted_memory(phys_addr_t start, phys_addr_t end)
> start -= unaccepted->phys_base;
> end -= unaccepted->phys_base;
>
> + /*
> + * Also consider the unaccepted state of the *next* page. See fix #1 in
> + * the comment on load_unaligned_zeropad() in accept_memory().
> + */
> + if (!(end % unit_size))
> + end += unit_size;
> +
> /* Make sure not to overrun the bitmap */
> if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> end = unaccepted->size * unit_size * BITS_PER_BYTE;

2023-05-17 18:51:16

[permalink] [raw]

Subject: Re: [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted memory

On Wed, May 17, 2023 at 09:32:27AM -0500, Tom Lendacky wrote:
> On 5/16/23 18:22, Kirill A. Shutemov wrote:
> > On Tue, May 16, 2023 at 05:41:55PM -0500, Tom Lendacky wrote:
> > > On 5/13/23 17:04, Kirill A. Shutemov wrote:
> > > > UEFI Specification version 2.9 introduces the concept of memory
> > > > acceptance: some Virtual Machine platforms, such as Intel TDX or AMD
> > > > SEV-SNP, requiring memory to be accepted before it can be used by the
> > > > guest. Accepting happens via a protocol specific for the Virtual
> > > > Machine platform.
> > > >
> > > > Accepting memory is costly and it makes VMM allocate memory for the
> > > > accepted guest physical address range. It's better to postpone memory
> > > > acceptance until memory is needed. It lowers boot time and reduces
> > > > memory overhead.
> > > >
> > > > The kernel needs to know what memory has been accepted. Firmware
> > > > communicates this information via memory map: a new memory type --
> > > > EFI_UNACCEPTED_MEMORY -- indicates such memory.
> > > >
> > > > Range-based tracking works fine for firmware, but it gets bulky for
> > > > the kernel: e820 has to be modified on every page acceptance. It leads
> > > > to table fragmentation, but there's a limited number of entries in the
> > > > e820 table
> > > >
> > > > Another option is to mark such memory as usable in e820 and track if the
> > > > range has been accepted in a bitmap. One bit in the bitmap represents
> > > > 2MiB in the address space: one 4k page is enough to track 64GiB or
> > > > physical address space.
> > > >
> > > > In the worst-case scenario -- a huge hole in the middle of the
> > > > address space -- It needs 256MiB to handle 4PiB of the address
> > > > space.
> > > >
> > > > Any unaccepted memory that is not aligned to 2M gets accepted upfront.
> > > >
> > > > The approach lowers boot time substantially. Boot to shell is ~2.5x
> > > > faster for 4G TDX VM and ~4x faster for 64G.
> > > >
> > > > TDX-specific code isolated from the core of unaccepted memory support. It
> > > > supposed to help to plug-in different implementation of unaccepted memory
> > > > such as SEV-SNP.
> > > >
> > > > -- Fragmentation study --
> > > >
> > > > Vlastimil and Mel were concern about effect of unaccepted memory on
> > > > fragmentation prevention measures in page allocator. I tried to evaluate
> > > > it, but it is tricky. As suggested I tried to run multiple parallel kernel
> > > > builds and follow how often kmem:mm_page_alloc_extfrag gets hit.
> > > >
> > > > See results in the v9 of the patchset[1][2]
> > > >
> > > > [1] https://lore.kernel.org/all/[email protected]
> > > > [2] https://lore.kernel.org/all/[email protected]
> > > >
> > > > --
> > > >
> > > > The tree can be found here:
> > > >
> > > > https://github.com/intel/tdx.git guest-unaccepted-memory
> > >
> > > I get some failures when building without TDX support selected in my
> > > kernel config after adding unaccepted memory support for SNP:
> > >
> > > In file included from arch/x86/boot/compressed/../../coco/tdx/tdx-shared.c:1,
> > > from arch/x86/boot/compressed/tdx-shared.c:2:
> > > ./arch/x86/include/asm/tdx.h: In function ?tdx_kvm_hypercall?:
> > > ./arch/x86/include/asm/tdx.h:72:17: error: ?ENODEV? undeclared (first use in this function)
> > > 72 | return -ENODEV;
> > > | ^~~~~~
> > > ./arch/x86/include/asm/tdx.h:72:17: note: each undeclared identifier is reported only once for each function it appears in
> > >
> > > Adding an include for linux/errno.h gets past that error, but then
> > > I get the following:
> > >
> > > ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_enc_status_changed_phys':
> > > tdx-shared.c:(.text+0x42): undefined reference to `__tdx_hypercall'
> > > ld: tdx-shared.c:(.text+0x7f): undefined reference to `__tdx_module_call'
> > > ld: tdx-shared.c:(.text+0xce): undefined reference to `__tdx_module_call'
> > > ld: tdx-shared.c:(.text+0x13b): undefined reference to `__tdx_module_call'
> > > ld: tdx-shared.c:(.text+0x153): undefined reference to `cc_mkdec'
> > > ld: tdx-shared.c:(.text+0x15d): undefined reference to `cc_mkdec'
> > > ld: tdx-shared.c:(.text+0x18e): undefined reference to `__tdx_hypercall'
> > > ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__tdx_hypercall' isn't defined
> > > ld: final link failed: bad value
> > >
> > > So it looks like arch/x86/boot/compressed/tdx-shared.c is being
> > > built, while arch/x86/boot/compressed/tdx.c isn't.
> >
> > Right. I think this should help:
> >
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 78f67e0a2666..b13a58021086 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -106,8 +106,8 @@ ifdef CONFIG_X86_64
> > endif
> >
> > vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> > -vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
> > -vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o $(obj)/tdx-shared.o
> > +vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o $(obj)/tdx-shared.o
> > +vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o
> >
> > vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
> > vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
> >
> > > After setting TDX in the kernel config, I can build successfully, but
> > > I'm running into an error when trying to accept memory during
> > > decompression.
> > >
> > > In drivers/firmware/efi/libstub/unaccepted_memory.c, I can see that the
> > > unaccepted_table is allocated, but when accept_memory() is invoked the
> > > table address is now zero. I thought maybe it had to do with bss, but even
> > > putting it in the .data section didn't help. I'll keep digging, but if you
> > > have any ideas, that would be great.
> >
> > Not right away. But maybe seeing your side of enabling would help.
>
> Let me get something pushed up where you can access it and I'll also send
> you my kernel config.
>
> In the mean time I added the following and everything worked. But I'm not
> sure how acceptable it is to always be checking for the table when the
> value is zero is.
>
>
> diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
> index f4642c4f25dd..8c5632ab1208 100644
> --- a/drivers/firmware/efi/libstub/unaccepted_memory.c
> +++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
> @@ -183,8 +183,13 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
> unsigned long bitmap_size;
> u64 unit_size;
> - if (!unaccepted_table)
> - return;
> + if (!unaccepted_table) {
> + efi_guid_t unaccepted_table_guid = LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
> +
> + unaccepted_table = get_efi_config_table(unaccepted_table_guid);
> + if (!unaccepted_table)
> + return;
> + }
> unit_size = unaccepted_table->unit_size;
>

Kudos to Ard: if efi_relocate_kernel() triggered, it copies the kernel
image to the new place before the variable gets initialized, so it has to
be initialized explicitly by decompressor.

It also covers the cases when bootloader doesn't use EFI stub, including
kexec cases.

I think this fixup should work.

diff --git a/arch/x86/boot/compressed/efi.h b/arch/x86/boot/compressed/efi.h
index cf475243b6d5..866c0af8b5b9 100644
--- a/arch/x86/boot/compressed/efi.h
+++ b/arch/x86/boot/compressed/efi.h
@@ -16,6 +16,7 @@ typedef guid_t efi_guid_t __aligned(__alignof__(u32));
#define ACPI_TABLE_GUID EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
#define ACPI_20_TABLE_GUID EFI_GUID(0x8868e871, 0xe4f1, 0x11d3, 0xbc, 0x22, 0x00, 0x80, 0xc7, 0x3c, 0x88, 0x81)
#define EFI_CC_BLOB_GUID EFI_GUID(0x067b1f5f, 0xcf26, 0x44c5, 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42)
+#define LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID EFI_GUID(0xd5d1de3c, 0x105c, 0x44f9, 0x9e, 0xa9, 0xbc, 0xef, 0x98, 0x12, 0x00, 0x31)

#define EFI32_LOADER_SIGNATURE "EL32"
#define EFI64_LOADER_SIGNATURE "EL64"
@@ -105,6 +106,14 @@ struct efi_setup_data {
u64 reserved[8];
};

+struct efi_unaccepted_memory {
+ u32 version;
+ u32 unit_size;
+ u64 phys_base;
+ u64 size;
+ unsigned long bitmap[];
+};
+
static inline int efi_guidcmp (efi_guid_t left, efi_guid_t right)
{
return memcmp(&left, &right, sizeof (efi_guid_t));
diff --git a/arch/x86/boot/compressed/mem.c b/arch/x86/boot/compressed/mem.c
index a4308d077885..0108c97399a5 100644
--- a/arch/x86/boot/compressed/mem.c
+++ b/arch/x86/boot/compressed/mem.c
@@ -1,8 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only

-#include "../cpuflags.h"
-#include "../string.h"
#include "error.h"
+#include "misc.h"
#include "tdx.h"
#include <asm/shared/tdx.h>

@@ -40,3 +39,25 @@ void arch_accept_memory(phys_addr_t start, phys_addr_t end)
else
error("Cannot accept memory: unknown platform\n");
}
+
+void init_unaccepted_memory(void)
+{
+ guid_t guid = LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
+ struct efi_unaccepted_memory *unaccepted_table;
+ unsigned long cfg_table_pa;
+ unsigned int cfg_table_len;
+ int ret;
+
+ ret = efi_get_conf_table(boot_params, &cfg_table_pa, &cfg_table_len);
+ if (ret)
+ error("EFI config table not found.");
+
+ unaccepted_table = (void *)efi_find_vendor_table(boot_params,
+ cfg_table_pa,
+ cfg_table_len,
+ guid);
+ if (unaccepted_table->version != 1)
+ error("Unknown version of unaccepted memory table\n");
+
+ set_unaccepted_table(unaccepted_table);
+}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index eb8df0d4ad51..36535a3753f5 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -458,6 +458,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,

if (IS_ENABLED(CONFIG_UNACCEPTED_MEMORY)) {
debug_putstr("Accepting memory... ");
+ init_unaccepted_memory();
accept_memory(__pa(output), __pa(output) + needed_size);
}

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 9663d1839f54..e1a0b49e0ed2 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -247,10 +247,10 @@ static inline unsigned long efi_find_vendor_table(struct boot_params *bp,
}
#endif /* CONFIG_EFI */

-#ifdef CONFIG_UNACCEPTED_MEMORY
+void init_unaccepted_memory(void);
+
+/* Implemented in EFI stub */
+void set_unaccepted_table(struct efi_unaccepted_memory *table);
void accept_memory(phys_addr_t start, phys_addr_t end);
-#else
-static inline void accept_memory(phys_addr_t start, phys_addr_t end) {}
-#endif

#endif /* BOOT_COMPRESSED_MISC_H */
diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index f4642c4f25dd..fd6a3195c68f 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -6,6 +6,18 @@

static struct efi_unaccepted_memory *unaccepted_table;

+/*
+ * Decompressor needs to initialize the variable to cover cases when the table
+ * is not allocated by EFI stub or EFI stub copied the kernel image with
+ * efi_relocate_kernel() before the variable is set.
+ *
+ * It must be call before the first usage of accept_memory() by decompressor.
+ */
+void set_unaccepted_table(struct efi_unaccepted_memory *table)
+{
+ unaccepted_table = table;
+}
+
efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
struct efi_boot_memmap *map)
{
--
Kiryl Shutsemau / Kirill A. Shutemov

2023-05-17 19:05:12