[PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Brijesh Singh]

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;
-- 
2.7.4

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Ard Biesheuvel]

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.

-- 
Ard.


> 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;
> --
> 2.7.4
>

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Borislav Petkov]

(dropping stable@ as this is not how you send patches to stable).

On Tue, Jul 03, 2018 at 05:37:18PM +0200, 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.
> >
> 
> 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.

I think the problem here is that IO memory can't be encrypted, at least
at the moment. Thus this patch. I believe future versions will be able
to handle encrypted IO but that's something Brijesh can correct me on.

So it is not really about the UEFI spec but about what the hardware
does/supports currently.

And I don't think that change matters on anything else besides AMD with
SEV enabled...

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Tom Lendacky]

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;
> 

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Brijesh Singh]

On 7/3/18 10:44 AM, Borislav Petkov wrote:
> (dropping stable@ as this is not how you send patches to stable).
>
> On Tue, Jul 03, 2018 at 05:37:18PM +0200, 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.
>>>
>> 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.

I agree with Ard,  it may be good idea to extend the UEFI spec to
include encryption information. Having this information may be helpful
in some cases, e.g if we ever need to map a specific non IO memory as
unencrypted. So far we have not seen the need for it. But I will ask AMD
folks working closely with UEFI committee to float this and submit it as
enhancement in Tianocore BZ.

> I think the problem here is that IO memory can't be encrypted, at least
> at the moment. Thus this patch. I believe future versions will be able
> to handle encrypted IO but that's something Brijesh can correct me on.

Yes you are right, IO memory can't be encrypted. We map all IO memory
ranges as unencrypted everywhere else in the kernel. The
EFI_MEMORY_MAPPED_IO type should also be mapped as unencrypted.


> So it is not really about the UEFI spec but about what the hardware
> does/supports currently.
>
> And I don't think that change matters on anything else besides AMD with
> SEV enabled...
>
> Thx.
>

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Borislav Petkov]

On Tue, Jul 03, 2018 at 04:16:57PM -0500, Brijesh Singh wrote:
> I agree with Ard,  it may be good idea to extend the UEFI spec to
> include encryption information. Having this information may be helpful
> in some cases, e.g if we ever need to map a specific non IO memory as
> unencrypted. So far we have not seen the need for it. But I will ask AMD
> folks working closely with UEFI committee to float this and submit it as
> enhancement in Tianocore BZ.

Except that if the IO memory handling unencrypted changes in future
incarnations, the changes to the spec become moot. I'm just saying...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Ard Biesheuvel]

On 3 July 2018 at 23:46, Borislav Petkov <bp@suse.de> wrote:
> On Tue, Jul 03, 2018 at 04:16:57PM -0500, Brijesh Singh wrote:
>> I agree with Ard,  it may be good idea to extend the UEFI spec to
>> include encryption information. Having this information may be helpful
>> in some cases, e.g if we ever need to map a specific non IO memory as
>> unencrypted. So far we have not seen the need for it. But I will ask AMD
>> folks working closely with UEFI committee to float this and submit it as
>> enhancement in Tianocore BZ.
>
> Except that if the IO memory handling unencrypted changes in future
> incarnations, the changes to the spec become moot. I'm just saying...
>

Quite the opposite. If we allocate a EFI_MEMORY_xx bit to signify that
a region should be mapped as encrypted, we no longer have to reason
about these things in the kernel but we can simply apply the
attributes that the UEFI memory map provides us. A platform could then
describe both encrypted and unencrypted MMIO regions at will.

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Ard Biesheuvel]

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() ?

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Brijesh Singh]

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

Re: [PATCH] x86/efi: Access EFI MMIO data as unencrypted when SEV is active

[Ard Biesheuvel]

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