Move memory acceptance x86 arch code into EFI stub

arch/x86/coco/tdx/tdx-shared.c +6 -0

+void __noreturn tdx_panic(const char *msg)
+{
+	struct tdx_module_args args = {
+		.r10 = TDX_HYPERCALL_STANDARD,
+		.r11 = TDVMCALL_REPORT_FATAL_ERROR,
+		.r12 = 0, /* Error code: 0 is Panic */

Move the implementation of tdx_panic() into the source file that is shared with the decompressor and the EFI stub.

Use memcpy() and strnlen() instead of strtomem_pad(), as the latter does not exist in the early boot code.

Note that __tdx_hypercall() may call __tdx_hypercall_failed() if the hypercall returns with an error (while it should never return to begin with). __tdx_hypercall_failed() calls the decompressor's error() routine, which prints a message and then loops forever.

When called from the EFI stub, this error() call may attempt to use port I/O to the default serial port rather than the TDX hypercalls which the decompressor uses normally to print diagnostics to the console, but this is fine: given that this situation only occurs after a catastrophic error, and a subsequent spurious return from tdx_panic(), whether error() uses port I/O or not is rather moot at that point, as long as it never returns.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/coco/tdx/tdx-shared.c    | 35 ++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c           | 35 --------------------
 arch/x86/include/asm/shared/tdx.h |  1 +
 3 files changed, 36 insertions(+), 35 deletions(-)

Quoted 19 lines · Ard Biesheuvel

Move the implementation of tdx_panic() into the source file that is shared with the decompressor and the EFI stub.

Use memcpy() and strnlen() instead of strtomem_pad(), as the latter does not exist in the early boot code.

Note that __tdx_hypercall() may call __tdx_hypercall_failed() if the hypercall returns with an error (while it should never return to begin with). __tdx_hypercall_failed() calls the decompressor's error() routine, which prints a message and then loops forever.

When called from the EFI stub, this error() call may attempt to use port I/O to the default serial port rather than the TDX hypercalls which the decompressor uses normally to print diagnostics to the console, but this is fine: given that this situation only occurs after a catastrophic error, and a subsequent spurious return from tdx_panic(), whether error() uses port I/O or not is rather moot at that point, as long as it never returns.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/coco/tdx/tdx-shared.c    | 35 ++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c           | 35 --------------------
 arch/x86/include/asm/shared/tdx.h |  1 +
 3 files changed, 36 insertions(+), 35 deletions(-)

Nah, that doesn't work:

ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_panic':
tdx-shared.c:(.text+0x2a3): undefined reference to `__fortify_panic'
ld: tdx-shared.c:(.text+0x2b2): undefined reference to `__fortify_panic'
ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__SCK__WARN_trap' isn't defined
ld: final link failed: bad value
make[3]: *** [arch/x86/boot/compressed/Makefile:117:
arch/x86/boot/compressed/vmlinux] Error 1
make[2]: *** [arch/x86/boot/Makefile:96:
arch/x86/boot/compressed/vmlinux] Error 2
make[1]: *** [arch/x86/Makefile:304: bzImage] Error 2
make: *** [Makefile:248: __sub-make] Error 2

This is a follow-up to [0].

Move arch_accept_memory(), which is only called by the EFI stub and never by the decompressor on a non-EFI boot, into the EFI stub, and avoid relying directly on decompressor APIs such as error().

Instead, call tdx_panic() on a failure to accept memory in a TDX guest.

This makes the decompressor's implementation of panic() obsolete, and allows it to be removed. This is a prerequisite for dropping the implementation of snprintf() from the EFI stub entirely, which is what the series containing [0] implements.

[0] http://lore.kernel.org/r/[email protected]

Cc Kiryl Shutsemau (Meta) <[email protected]>

Cc Borislav Petkov <[email protected]>

Ard Biesheuvel (3):
x86/tdx: Share tdx_panic() with the EFI stub
x86/boot: Move unaccepted memory handling out of the decompressor x86/boot: Drop unused implementation of panic()

 arch/x86/boot/compressed/error.c        | 19 ---------
 arch/x86/boot/compressed/error.h        |  1 -
 arch/x86/boot/compressed/mem.c          | 42 --------------------
 arch/x86/boot/compressed/sev.h          |  2 -
 arch/x86/coco/tdx/tdx-shared.c          | 35 ++++++++++++++++
 arch/x86/coco/tdx/tdx.c                 | 35 ----------------
 arch/x86/include/asm/sev.h              |  2 +
 arch/x86/include/asm/shared/tdx.h       |  1 +
 drivers/firmware/efi/libstub/x86-stub.c | 40 +++++++++++++++++++
 9 files changed, 78 insertions(+), 99 deletions(-)

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935

Signature · 1 line

2.47.3

This is a follow-up to [0].

Move arch_accept_memory(), which is only called by the EFI stub and never by the decompressor on a non-EFI boot, into the EFI stub, and avoid relying directly on decompressor APIs such as error().

Instead, call tdx_panic() on a failure to accept memory in a TDX guest.

Kiryl,

Quoted 6 lines · Ard Biesheuvel

This is a follow-up to [0].

Move arch_accept_memory(), which is only called by the EFI stub and never by the decompressor on a non-EFI boot, into the EFI stub, and avoid relying directly on decompressor APIs such as error().

Quoted 1 line

Instead, call tdx_panic() on a failure to accept memory in a TDX guest.

The TDG accept call can return:

TDX_OPERAND_INVALID - That would be a bug in Linux TDX code, we don't need to pass it to the caller. Probably don't need to handle it. TDX_PAGE_ALREADY_ACCEPTED - Potential security sensitive error that is the guests fault.

TDX_PAGE_SIZE_MISMATCH - Already handled. Sort of. Not sure if it is robust to S-EPT page size changes?
TDX_SUCCESS - Already handled
TDX_OPERAND_BUSY - Can happen from transient host side S-EPT locking. Or it could be the guest's fault if they are accepting the same page from different vCPUs at the same time, so the guest case is similar to TDX_PAGE_ALREADY_ACCEPTED.

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP doesn't do anything for this case. It seems like part of the problem is that we are passing errors back that the caller can't feasibly handle.

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

+Yan

Quoted 10 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Here is a strawman proposal. Please consider critically.

TDX_PAGE_ALREADY_ACCEPTED: Zero memory that was re-accepted and treat it as a success.

TDX_OPERAND_BUSY host contention: Loop and blame the host for any infinite loops. It can be not unlike if the host zap's the EPT and the guest gets stuck on infinite EPT violations. Except that the host does not get any notification of the trouble it is causing... Some sort of TDX arch change to notify the host of heavy guest-host contention could be worked on in the background?

TDX_OPERAND_BUSY guest contention: RW lock around accept. Normally take the lock for read, and a single fallback attempt takes the lock for write.

Then I think all errors accept() would want to return to the callers are handled.

The only thing I'm not sure about is TDX_PAGE_ALREADY_ACCEPTED. Some callers may want a warning on that case. I think a warning is more in line with what we have done in similar situations, rather than panic. But a warming would throw off the EFI stub callers apparently.

The memory acceptance occurs after ExitBootServices(), and so there is little the EFI stub can do at this point except maybe set a variable with an error message and reset the system. If that is better than a panic(), this is something we can consider.

However, I'd like to avoid going off into the weeds here with something that was intended to simply move some logic from one place to the other. The current code prints an error to the UART I/O port (rather than the PV console TDX uses when running in the decompressor) and then loops forever. Is that something we could retain for this series, and then continue the above discussion in the context of a separate series based on top of that?

+Yan

Quoted 6 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Quoted 4 lines

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Quoted 9 lines · Rick P

+Yan

Quoted 6 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Quoted 8 lines

Quoted 4 lines

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Re-accept attempts may occur due to:
(a) two concurrent ACCEPT TDCALLs, where the one that arrives slightly later returns either TDACCEPT_ALREADY_ACCEPTED or TDX_OPERAND_BUSY.
(b) two successive ACCEPT TDCALLs on the same GPA.

Since Linux guest always invokes ACCEPT TDCALL before a memory access, and accept_memory() always checks the unaccepted_table->bitmap before invoking the ACCEPT TDCALL, case (b) should be impossible in practice, right?

Is case (a) a valid scenario, and does it actually occur in a Linux guest?

Quoted 23 lines · Yan Zhao

Quoted 9 lines · Rick P

+Yan

Quoted 6 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Quoted 8 lines

Quoted 4 lines

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Re-accept attempts may occur due to:
(a) two concurrent ACCEPT TDCALLs, where the one that arrives slightly later returns either TDACCEPT_ALREADY_ACCEPTED or TDX_OPERAND_BUSY.
(b) two successive ACCEPT TDCALLs on the same GPA.

Quoted 5 lines

Since Linux guest always invokes ACCEPT TDCALL before a memory access, and accept_memory() always checks the unaccepted_table->bitmap before invoking the ACCEPT TDCALL, case (b) should be impossible in practice, right?

Is case (a) a valid scenario, and does it actually occur in a Linux guest?

Case (a) should be prevented by the unaccepted_memory_lock in accept_memory(), right?

Quoted 24 lines · Yan Zhao

Quoted 23 lines · Yan Zhao

Quoted 9 lines · Rick P

+Yan

Quoted 6 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Quoted 8 lines

Quoted 4 lines

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Re-accept attempts may occur due to:
(a) two concurrent ACCEPT TDCALLs, where the one that arrives slightly later returns either TDACCEPT_ALREADY_ACCEPTED or TDX_OPERAND_BUSY.
(b) two successive ACCEPT TDCALLs on the same GPA.

Quoted 7 lines

Quoted 5 lines

Since Linux guest always invokes ACCEPT TDCALL before a memory access, and accept_memory() always checks the unaccepted_table->bitmap before invoking the ACCEPT TDCALL, case (b) should be impossible in practice, right?

Is case (a) a valid scenario, and does it actually occur in a Linux guest?

Case (a) should be prevented by the unaccepted_memory_lock in accept_memory(), right?

Right, for accept_memory().

The lock itself is dropped around the TDCALL, but the range stays on accepting_list until the bits are cleared, and the overlap check is done in unit_size granularity, so a second caller for the same unit spins until the first one is done and then finds the bits clear. Both (a) and
(b) are covered there.

There is a second ACCEPT issuer that does not look at the bitmap at
all: shared->private conversion in tdx_enc_status_changed().

It does not need the bitmap because the memory came from the page allocator or memblock and got accepted before it was handed out. It also has no per-range serialization; mem_enc_lock is only taken for read. So ALREADY_ACCEPTED on that path means either the guest converted the same range twice (a double-free class of bug) or the VMM acked MapGPA(shared) but never removed the private page. Either way the page there is the one the guest accepted earlier, and the failure already comes back as -EIO rather than a panic.

Quoted 44 lines · Kiryl Shutsemau

Quoted 24 lines · Yan Zhao

Quoted 23 lines · Yan Zhao

Quoted 9 lines · Rick P

+Yan

Quoted 6 lines · Kiryl Shutsemau

Quoted 5 lines

I guess the caller could care about TDX_PAGE_ALREADY_ACCEPTED errors. But SNP
doesn't do anything for this case. It seems like part of the problem is that we
are passing errors back that the caller can't feasibly handle.

Quoted 8 lines

Quoted 4 lines

Maybe. I don't understand SNP model and why they don't care about errors here.

Do you have a proposal here?

Yan pointed out that future TDX modules will not take an S-EPT entry lock on accepting a NP S-EPT entry. However, I think this won't prevent guest caused busys on re-accept attempts?

Re-accept attempts may occur due to:
(a) two concurrent ACCEPT TDCALLs, where the one that arrives slightly later returns either TDACCEPT_ALREADY_ACCEPTED or TDX_OPERAND_BUSY.
(b) two successive ACCEPT TDCALLs on the same GPA.

Quoted 7 lines

Quoted 5 lines

Since Linux guest always invokes ACCEPT TDCALL before a memory access, and accept_memory() always checks the unaccepted_table->bitmap before invoking the ACCEPT TDCALL, case (b) should be impossible in practice, right?

Is case (a) a valid scenario, and does it actually occur in a Linux guest?

Case (a) should be prevented by the unaccepted_memory_lock in accept_memory(), right?

Right, for accept_memory().

The lock itself is dropped around the TDCALL, but the range stays on accepting_list until the bits are cleared, and the overlap check is done in unit_size granularity, so a second caller for the same unit spins until the first one is done and then finds the bits clear. Both (a) and
(b) are covered there.

There is a second ACCEPT issuer that does not look at the bitmap at
all: shared->private conversion in tdx_enc_status_changed().

Quoted 9 lines

It does not need the bitmap because the memory came from the page allocator or memblock and got accepted before it was handed out. It also has no per-range serialization; mem_enc_lock is only taken for read. So ALREADY_ACCEPTED on that path means either the guest converted the same range twice (a double-free class of bug) or the VMM acked MapGPA(shared) but never removed the private page. Either way the page there is the one the guest accepted earlier, and the failure already comes back as -EIO rather than a panic.

Is there any way we could make some progress here?

Ultimately, the thing I am after is to get rid of the call from the decompressor to EFI stub's snprintf(), which I want to remove. For the time being, I am more than happy leaving the memory acceptance where it is, and just dropping the call to snprintf().

Quoted 6 lines

Is there any way we could make some progress here?

Ultimately, the thing I am after is to get rid of the call from the decompressor to EFI stub's snprintf(), which I want to remove. For the time being, I am more than happy leaving the memory acceptance where it is, and just dropping the call to snprintf().

Please go ahead with the move. Rick's questions are about what tdx_accept_memory() should return, not about where the caller lives, and can be sorted out separately.

Quoted 7 lines · Kiryl Shutsemau

Quoted 6 lines

Is there any way we could make some progress here?

Ultimately, the thing I am after is to get rid of the call from the decompressor to EFI stub's snprintf(), which I want to remove. For the time being, I am more than happy leaving the memory acceptance where it is, and just dropping the call to snprintf().

Quoted 3 lines

Please go ahead with the move. Rick's questions are about what tdx_accept_memory() should return, not about where the caller lives, and can be sorted out separately.

Yep. I tried to do a quick smoke test and got a similar build error as Boris. I guess you will send a new revision?