SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
Fixes: 1379edd59673 (x86/efi: Access EFI data as encrypted ...) Cc: Tom Lendacky thomas.lendacky@amd.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@suse.de Cc: linux-efi@vger.kernel.org Cc: kvm@vger.kernel.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Matt Fleming matt@codeblueprint.co.uk Cc: Andy Lutomirski luto@kernel.org Cc: stable@vger.kernel.org # 4.15.x Signed-off-by: Brijesh Singh brijesh.singh@amd.com --- arch/x86/platform/efi/efi_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 77873ce..5f2eb32 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va) if (!(md->attribute & EFI_MEMORY_WB)) flags |= _PAGE_PCD;
- if (sev_active()) + if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO) flags |= _PAGE_ENC;
pfn = md->phys_addr >> PAGE_SHIFT;
On 3 July 2018 at 15:32, Brijesh Singh brijesh.singh@amd.com wrote:
SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
I'm uncomfortable having to carry these heuristics in the kernel. The UEFI memory map should be the definitive source of information regarding how the OS should map the regions it describes, and if we need to guess the encryption status, we are likely to get it wrong at least some of the times.
Is any work underway to get the UEFI spec extended to take encrypted memory into account? I am aware that we cannot disclose specifics, but you should be able to disclose whether it is under discussion or not.
In the mean time, we will have to do something, I know that, but I would like to discuss the proper solution before proceeding with the stop gap one.
On 7/3/2018 8:32 AM, Brijesh Singh wrote:
SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
Fixes: 1379edd59673 (x86/efi: Access EFI data as encrypted ...) Cc: Tom Lendacky thomas.lendacky@amd.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@suse.de Cc: linux-efi@vger.kernel.org Cc: kvm@vger.kernel.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Matt Fleming matt@codeblueprint.co.uk Cc: Andy Lutomirski luto@kernel.org Cc: stable@vger.kernel.org # 4.15.x Signed-off-by: Brijesh Singh brijesh.singh@amd.com
Reviewed-by: Tom Lendacky thomas.lendacky@amd.com
arch/x86/platform/efi/efi_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 77873ce..5f2eb32 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va) if (!(md->attribute & EFI_MEMORY_WB)) flags |= _PAGE_PCD;
- if (sev_active())
- if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO) flags |= _PAGE_ENC;
pfn = md->phys_addr >> PAGE_SHIFT;
On 3 July 2018 at 15:32, Brijesh Singh brijesh.singh@amd.com wrote:
SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
Fixes: 1379edd59673 (x86/efi: Access EFI data as encrypted ...) Cc: Tom Lendacky thomas.lendacky@amd.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@suse.de Cc: linux-efi@vger.kernel.org Cc: kvm@vger.kernel.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Matt Fleming matt@codeblueprint.co.uk Cc: Andy Lutomirski luto@kernel.org Cc: stable@vger.kernel.org # 4.15.x Signed-off-by: Brijesh Singh brijesh.singh@amd.com
arch/x86/platform/efi/efi_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 77873ce..5f2eb32 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va) if (!(md->attribute & EFI_MEMORY_WB)) flags |= _PAGE_PCD;
if (sev_active())
if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO) flags |= _PAGE_ENC; pfn = md->phys_addr >> PAGE_SHIFT;
Is it safe to only update this occurrence and not the one in efi_runtime_update_mappings() ?
Hi Ard,
On 07/11/2018 05:00 AM, Ard Biesheuvel wrote:
On 3 July 2018 at 15:32, Brijesh Singh brijesh.singh@amd.com wrote:
SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
Fixes: 1379edd59673 (x86/efi: Access EFI data as encrypted ...) Cc: Tom Lendacky thomas.lendacky@amd.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@suse.de Cc: linux-efi@vger.kernel.org Cc: kvm@vger.kernel.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Matt Fleming matt@codeblueprint.co.uk Cc: Andy Lutomirski luto@kernel.org Cc: stable@vger.kernel.org # 4.15.x Signed-off-by: Brijesh Singh brijesh.singh@amd.com
arch/x86/platform/efi/efi_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 77873ce..5f2eb32 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va) if (!(md->attribute & EFI_MEMORY_WB)) flags |= _PAGE_PCD;
if (sev_active())
if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO) flags |= _PAGE_ENC; pfn = md->phys_addr >> PAGE_SHIFT;Is it safe to only update this occurrence and not the one in efi_runtime_update_mappings() ?
It's safe to update this occurrence only. The SEV support is added in recent EDK2 bios, and the version of bios provides the EFI_MEMORY_ATTRIBUTE_TABLE. Hence the efi_enabled(EFI_MEM_ATTR) check in efi_runtime_update_mappings() will always be true. When EFI_MEM_ATTR is set the code updates the mapping and returns (see below)
void __init efi_runtime_update_mappings(void) { ..... .....
/* * Use the EFI Memory Attribute Table for mapping permissions if it * exists, since it is intended to supersede EFI_PROPERTIES_TABLE. */ if (efi_enabled(EFI_MEM_ATTR)) { efi_memattr_apply_permissions(NULL, efi_update_mem_attr); return; }
...
}
The EFI_MEMORY_ATTRIBUTE_TABLE table does not include MMIO regions, the table describes the memory protections to EFI runtime code and data regions only. Both EFI runtime code and data should be mapped as encrypted. Hence I skipped updating the efi_runtime_update_mappings().
thanks
On 17 July 2018 at 03:15, Brijesh Singh brijesh.singh@amd.com wrote:
Hi Ard,
On 07/11/2018 05:00 AM, Ard Biesheuvel wrote:
On 3 July 2018 at 15:32, Brijesh Singh brijesh.singh@amd.com wrote:
SEV guest fails to update the UEFI runtime variables stored in the flash. commit 1379edd59673 ("x86/efi: Access EFI data as encrypted when SEV is active") unconditionally maps all the UEFI runtime data as 'encrypted' (C=1). When SEV is active the UEFI runtime data marked as EFI_MEMORY_MAPPED_IO should be mapped as 'unencrypted' so that both guest and hypervisor can access the data.
Fixes: 1379edd59673 (x86/efi: Access EFI data as encrypted ...) Cc: Tom Lendacky thomas.lendacky@amd.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@suse.de Cc: linux-efi@vger.kernel.org Cc: kvm@vger.kernel.org Cc: Ard Biesheuvel ard.biesheuvel@linaro.org Cc: Matt Fleming matt@codeblueprint.co.uk Cc: Andy Lutomirski luto@kernel.org Cc: stable@vger.kernel.org # 4.15.x Signed-off-by: Brijesh Singh brijesh.singh@amd.com
arch/x86/platform/efi/efi_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c index 77873ce..5f2eb32 100644 --- a/arch/x86/platform/efi/efi_64.c +++ b/arch/x86/platform/efi/efi_64.c @@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va) if (!(md->attribute & EFI_MEMORY_WB)) flags |= _PAGE_PCD;
if (sev_active())
if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO) flags |= _PAGE_ENC; pfn = md->phys_addr >> PAGE_SHIFT;Is it safe to only update this occurrence and not the one in efi_runtime_update_mappings() ?
It's safe to update this occurrence only. The SEV support is added in recent EDK2 bios, and the version of bios provides the EFI_MEMORY_ATTRIBUTE_TABLE. Hence the efi_enabled(EFI_MEM_ATTR) check in efi_runtime_update_mappings() will always be true. When EFI_MEM_ATTR is set the code updates the mapping and returns (see below)
void __init efi_runtime_update_mappings(void) { ..... .....
/* * Use the EFI Memory Attribute Table for mapping permissions if it * exists, since it is intended to supersede EFI_PROPERTIES_TABLE. */ if (efi_enabled(EFI_MEM_ATTR)) { efi_memattr_apply_permissions(NULL, efi_update_mem_attr); return; }
...
}
The EFI_MEMORY_ATTRIBUTE_TABLE table does not include MMIO regions, the table describes the memory protections to EFI runtime code and data regions only. Both EFI runtime code and data should be mapped as encrypted. Hence I skipped updating the efi_runtime_update_mappings().
Ok, thanks for clearing that up
Queued in efi/urgent
linux-stable-mirror@lists.linaro.org
-
Ard Biesheuvel -
Brijesh Singh -
Tom Lendacky