On Thu, 09 Jun, at 01:33:30PM, Tom Lendacky wrote:

I was trying to play it safe here, but as you say, the firmware should
be using our page tables so we can get rid of this call. The problem
will actually be if we transition to a 32-bit efi. The encryption bit
will be lost in cr3 and so the pgd table will have to be un-encrypted.
The entries in the pgd can have the encryption bit set so I would only
need to worry about the pgd itself. I'll have to update the
efi_alloc_page_tables routine.

 
Interesting, I hadn't expected 32-bit EFI to be an option for
platforms with the SME technology. I'd assumed we could just ignore
that.

Are you saying that the encryption bit isn't supported in 32-bit
compatibility mode? We don't do a "full" switch to 32-bit protected
mode when in mixed mode, just load a 32-bit code segment descriptor.
The page tables are not modified at all.

The encryption bit in the cr3 register will indicate if the pgd table
is encrypted or not. Based on my comment above about the pgd having
to be un-encrypted in case we have to transition to 32-bit efi, this
can be removed.

 
I'm not (yet) sure that the pgd needs to be unencrypted for 32-bit EFI
when running a 64-bit kernel. In the AMD Programmer's Manual, Section
7.10.3 Operating Modes seems to indicate that running encrypted should
work fine.

I'll look into this a bit more. From looking at it I don't want the
_PAGE_ENC bit set for the memmap unless it gets re-allocated (which
I missed in these patches). Let me see what I can do with this.

 
I don't understand your comment about re-allocating the memmap.

The kernel builds its own EFI memory map at runtime, initially based
on the memory map provided by the firmware. We always allocate a new
memory map.

In efi_setup_page_tables() we're building our own page tables, which
should be encrypted, and mapping EFI regions described by the memmap
into those page tables.

So unless we're mapping an MMIO region (in which case _PAGE_PCD is set
in @flags for kernel_map_pages_in_pgd()) I would expect _PAGE_ENC to
be set.

I'll look further into this, but I saw that this area of virtual memory
was mapped un-encrypted and after freeing the boot services the
mappings were somehow reused as un-encrypted for DMA which assumes
(unless using swiotlb) encrypted. This resulted in DMA data being
transferred in as encrypted and then accessed un-encrypted.

That the mappings were re-used isn't a surprise.

efi_free_boot_services() lifts the reservation that was put in place
during efi_reserve_boot_services() and releases the pages to the
kernel's memory allocators.

What is surprising is that they were marked unencrypted at all.
There's nothing special about these pages as far as the __va() region
is concerned.