[PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Tom Lendacky]

The dmidecode program fails to properly decode the SMBIOS data supplied
by OVMF/UEFI when running on an SEV guest. The SMBIOS area, under SEV, is
encrypted and resides in reserved memory that is marked as EFI runtime
services data. As a result, when memremap() is attempted for the SMBIOS
data, it can't be mapped as regular RAM (through try_ram_remap()) and,
since the address isn't part of the iomem resources list, it isn't mapped
encrypted through the fallback ioremap().

Update __ioremap_check_mem() to set the IORES_MAP_ENCRYPTED flag if SEV is
active and the memory being mapped is part of EFI runtime services data.
This allows any runtime services data, which has been created encrypted,
to be mapped encrypted.

Cc: Bruce Rogers <brogers@suse.com>
Cc: Joerg Roedel <jroedel@suse.de>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/mm/ioremap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 44e4beb4239f..382b6ca66820 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -135,6 +135,13 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
 	memset(desc, 0, sizeof(struct ioremap_desc));
 
 	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
+
+	/*
+	 * The EFI runtime services data area is not covered by walk_mem_res(),
+	 * but must be mapped encrypted when SEV is active.
+	 */
+	if (sev_active() && efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
+		desc->flags |= IORES_MAP_ENCRYPTED;
 }
 
 /*
-- 
2.17.1

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Joerg Roedel]

Hi Tom,

On Tue, Feb 25, 2020 at 09:42:07AM -0600, Tom Lendacky wrote:
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 44e4beb4239f..382b6ca66820 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -135,6 +135,13 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>  	memset(desc, 0, sizeof(struct ioremap_desc));
>  
>  	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
> +
> +	/*
> +	 * The EFI runtime services data area is not covered by walk_mem_res(),
> +	 * but must be mapped encrypted when SEV is active.
> +	 */
> +	if (sev_active() && efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
> +		desc->flags |= IORES_MAP_ENCRYPTED;
>  }

I can confirm that this fixes dmi-decode on my SEV guest. While
reviewing I looked into using walk_system_ram_range(), but since this is
only for the EFI_RUNTIME_SERVICES_DATA, it is not needed, so:

Reviewed-by: Joerg Roedel <jroedel@suse.de>
Tested-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Borislav Petkov]

On Tue, Feb 25, 2020 at 09:42:07AM -0600, Tom Lendacky wrote:
> The dmidecode program fails to properly decode the SMBIOS data supplied
> by OVMF/UEFI when running on an SEV guest. The SMBIOS area, under SEV, is
> encrypted and resides in reserved memory that is marked as EFI runtime
> services data. As a result, when memremap() is attempted for the SMBIOS
> data, it can't be mapped as regular RAM (through try_ram_remap()) and,
> since the address isn't part of the iomem resources list, it isn't mapped
> encrypted through the fallback ioremap().
> 
> Update __ioremap_check_mem() to set the IORES_MAP_ENCRYPTED flag if SEV is
> active and the memory being mapped is part of EFI runtime services data.
> This allows any runtime services data, which has been created encrypted,
> to be mapped encrypted.
> 
> Cc: Bruce Rogers <brogers@suse.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/mm/ioremap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 44e4beb4239f..382b6ca66820 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -135,6 +135,13 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>  	memset(desc, 0, sizeof(struct ioremap_desc));
>  
>  	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
> +
> +	/*
> +	 * The EFI runtime services data area is not covered by walk_mem_res(),
> +	 * but must be mapped encrypted when SEV is active.
> +	 */
> +	if (sev_active() && efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
> +		desc->flags |= IORES_MAP_ENCRYPTED;
>  }

Why isn't this done in __ioremap_check_encrypted() which is exactly for
SEV stuff like that?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Joerg Roedel]

On Tue, Mar 10, 2020 at 01:40:03PM +0100, Borislav Petkov wrote:
> On Tue, Feb 25, 2020 at 09:42:07AM -0600, Tom Lendacky wrote:
> > @@ -135,6 +135,13 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
> >  	memset(desc, 0, sizeof(struct ioremap_desc));
> >  
> >  	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
> > +
> > +	/*
> > +	 * The EFI runtime services data area is not covered by walk_mem_res(),
> > +	 * but must be mapped encrypted when SEV is active.
> > +	 */
> > +	if (sev_active() && efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
> > +		desc->flags |= IORES_MAP_ENCRYPTED;
> >  }
> 
> Why isn't this done in __ioremap_check_encrypted() which is exactly for
> SEV stuff like that?

See the comment added in the patch, walk_mem_res() does not iterate over
the resource which contains EFI_RUNTIME_SERVICES_DATA, so
__ioremap_check_encrypted() will not be called on that resource.

walk_system_ram_range() might do the job, but calling it only for
EFI_RUNTIME_SERVICES_DATA has some overhead.

Regards,

	Joerg

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Borislav Petkov]

On Tue, Mar 10, 2020 at 02:03:21PM +0100, Joerg Roedel wrote:
> See the comment added in the patch, walk_mem_res() does not iterate over
> the resource which contains EFI_RUNTIME_SERVICES_DATA, so
> __ioremap_check_encrypted() will not be called on that resource.
> 
> walk_system_ram_range() might do the job, but calling it only for
> EFI_RUNTIME_SERVICES_DATA has some overhead.

Ok, then.

Let's wrap this in a new function which is called at the end of
__ioremap_check_mem() instead of trying to map EFI_RUNTIME_SERVICES_DATA
to IORES_DESC types and match the flags just so that we can preserve the
flow. And add a comment above it why we're doing this.

As you said on IRC, none of the IO resource ranges covers the
EFI_RUNTIME_SERVICES_DATA.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Borislav Petkov]

On Tue, Mar 10, 2020 at 05:37:38PM +0100, Borislav Petkov wrote:
> Let's wrap this in a new function which is called at the end of
> __ioremap_check_mem() instead of trying to map EFI_RUNTIME_SERVICES_DATA
> to IORES_DESC types and match the flags just so that we can preserve the
> flow. And add a comment above it why we're doing this.
> 
> As you said on IRC, none of the IO resource ranges covers the
> EFI_RUNTIME_SERVICES_DATA.

Ok, here's what I have. @joro, I know it is trivially different from the
version you tested but I'd appreciate it if you ran it again, just to be
sure.

Thx.

---
From 244b62ca142c6296361bde953488fc64db31f1bd Mon Sep 17 00:00:00 2001
From: Tom Lendacky <thomas.lendacky@amd.com>
Date: Tue, 10 Mar 2020 18:35:57 +0100
Subject: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for
 SEV

The dmidecode program fails to properly decode the SMBIOS data supplied
by OVMF/UEFI when running in an SEV guest. The SMBIOS area, under SEV, is
encrypted and resides in reserved memory that is marked as EFI runtime
services data.

As a result, when memremap() is attempted for the SMBIOS data, it
can't be mapped as regular RAM (through try_ram_remap()) and, since
the address isn't part of the iomem resources list, it isn't mapped
encrypted through the fallback ioremap().

Add a new __ioremap_check_other() to deal with memory types like
EFI_RUNTIME_SERVICES_DATA which are not covered by the resource ranges.

This allows any runtime services data, which has been created encrypted,
to be mapped encrypted.

 [ bp: Move functionality to a separate function. ]

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: <stable@vger.kernel.org> # 5.3
Link: https://lkml.kernel.org/r/2d9e16eb5b53dc82665c95c6764b7407719df7a0.1582645327.git.thomas.lendacky@amd.com
---
 arch/x86/mm/ioremap.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 44e4beb4239f..935a91e1fd77 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -106,6 +106,19 @@ static unsigned int __ioremap_check_encrypted(struct resource *res)
 	return 0;
 }
 
+/*
+ * The EFI runtime services data area is not covered by walk_mem_res(), but must
+ * be mapped encrypted when SEV is active.
+ */
+static void __ioremap_check_other(resource_size_t addr, struct ioremap_desc *desc)
+{
+	if (!sev_active())
+		return;
+
+	if (efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
+		desc->flags |= IORES_MAP_ENCRYPTED;
+}
+
 static int __ioremap_collect_map_flags(struct resource *res, void *arg)
 {
 	struct ioremap_desc *desc = arg;
@@ -124,6 +137,9 @@ static int __ioremap_collect_map_flags(struct resource *res, void *arg)
  * To avoid multiple resource walks, this function walks resources marked as
  * IORESOURCE_MEM and IORESOURCE_BUSY and looking for system RAM and/or a
  * resource described not as IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
+ *
+ * After that, deal with misc other ranges in __ioremap_check_other() which do
+ * not fall into the above category.
  */
 static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
 				struct ioremap_desc *desc)
@@ -135,6 +151,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
 	memset(desc, 0, sizeof(struct ioremap_desc));
 
 	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
+
+	__ioremap_check_other(addr, desc);
 }
 
 /*
-- 
2.21.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Joerg Roedel]

Hi,

On Tue, Mar 10, 2020 at 06:47:31PM +0100, Borislav Petkov wrote:
> Ok, here's what I have. @joro, I know it is trivially different from the
> version you tested but I'd appreciate it if you ran it again, just to be
> sure.

Looks good and ested it, works fine.

Reviewed-by: Joerg Roedel <jroedel@suse.de>
Tested-by: Joerg Roedel <jroedel@suse.de>

> ---
> >From 244b62ca142c6296361bde953488fc64db31f1bd Mon Sep 17 00:00:00 2001
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Date: Tue, 10 Mar 2020 18:35:57 +0100
> Subject: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for
>  SEV
> 
> The dmidecode program fails to properly decode the SMBIOS data supplied
> by OVMF/UEFI when running in an SEV guest. The SMBIOS area, under SEV, is
> encrypted and resides in reserved memory that is marked as EFI runtime
> services data.
> 
> As a result, when memremap() is attempted for the SMBIOS data, it
> can't be mapped as regular RAM (through try_ram_remap()) and, since
> the address isn't part of the iomem resources list, it isn't mapped
> encrypted through the fallback ioremap().
> 
> Add a new __ioremap_check_other() to deal with memory types like
> EFI_RUNTIME_SERVICES_DATA which are not covered by the resource ranges.
> 
> This allows any runtime services data, which has been created encrypted,
> to be mapped encrypted.
> 
>  [ bp: Move functionality to a separate function. ]
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: <stable@vger.kernel.org> # 5.3
> Link: https://lkml.kernel.org/r/2d9e16eb5b53dc82665c95c6764b7407719df7a0.1582645327.git.thomas.lendacky@amd.com
> ---
>  arch/x86/mm/ioremap.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 44e4beb4239f..935a91e1fd77 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -106,6 +106,19 @@ static unsigned int __ioremap_check_encrypted(struct resource *res)
>  	return 0;
>  }
>  
> +/*
> + * The EFI runtime services data area is not covered by walk_mem_res(), but must
> + * be mapped encrypted when SEV is active.
> + */
> +static void __ioremap_check_other(resource_size_t addr, struct ioremap_desc *desc)
> +{
> +	if (!sev_active())
> +		return;
> +
> +	if (efi_mem_type(addr) == EFI_RUNTIME_SERVICES_DATA)
> +		desc->flags |= IORES_MAP_ENCRYPTED;
> +}
> +
>  static int __ioremap_collect_map_flags(struct resource *res, void *arg)
>  {
>  	struct ioremap_desc *desc = arg;
> @@ -124,6 +137,9 @@ static int __ioremap_collect_map_flags(struct resource *res, void *arg)
>   * To avoid multiple resource walks, this function walks resources marked as
>   * IORESOURCE_MEM and IORESOURCE_BUSY and looking for system RAM and/or a
>   * resource described not as IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
> + *
> + * After that, deal with misc other ranges in __ioremap_check_other() which do
> + * not fall into the above category.
>   */
>  static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>  				struct ioremap_desc *desc)
> @@ -135,6 +151,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>  	memset(desc, 0, sizeof(struct ioremap_desc));
>  
>  	walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
> +
> +	__ioremap_check_other(addr, desc);
>  }
>  
>  /*
> -- 
> 2.21.0
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/ioremap: Map EFI runtime services data as encrypted for SEV

[Borislav Petkov]

On Wed, Mar 11, 2020 at 10:04:47AM +0100, Joerg Roedel wrote:
> Hi,
> 
> On Tue, Mar 10, 2020 at 06:47:31PM +0100, Borislav Petkov wrote:
> > Ok, here's what I have. @joro, I know it is trivially different from the
> > version you tested but I'd appreciate it if you ran it again, just to be
> > sure.
> 
> Looks good and ested it, works fine.
> 
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> Tested-by: Joerg Roedel <jroedel@suse.de>

Thanks man!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette