linux-kernel - Re: [PATCHv3 2/2] x86/mm: Make memremap(MEMREMAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mvosqsybplnqfh6wadw5ue7u3plqnfo5ojusvaq6htzzhtfce2@bbgogdund3ho>
Date: Wed, 15 Jan 2025 12:23:54 +0200
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexei Starovoitov <ast@...nel.org>, 
	Andrea Parri <parri.andrea@...il.com>, Arnd Bergmann <arnd@...db.de>, 
	Daniel Borkmann <daniel@...earbox.net>, Eric Chan <ericchancf@...gle.com>, 
	Jason Gunthorpe <jgg@...pe.ca>, Kai Huang <kai.huang@...el.com>, 
	Kefeng Wang <wangkefeng.wang@...wei.com>, Kent Overstreet <kent.overstreet@...ux.dev>, 
	Palmer Dabbelt <palmer@...osinc.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Russell King <linux@...linux.org.uk>, Samuel Holland <samuel.holland@...ive.com>, 
	Suren Baghdasaryan <surenb@...gle.com>, Yuntao Wang <ytcoode@...il.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	stable@...r.kernel.org, Ashish Kalra <ashish.kalra@....com>, 
	"Maciej W. Rozycki" <macro@...am.me.uk>
Subject: Re: [PATCHv3 2/2] x86/mm: Make memremap(MEMREMAP_WB) map memory as
 encrypted by default

On Tue, Jan 14, 2025 at 10:54:53AM -0600, Tom Lendacky wrote:
> On 1/14/25 09:06, Tom Lendacky wrote:
> > On 1/14/25 08:44, Kirill A. Shutemov wrote:
> >> On Tue, Jan 14, 2025 at 08:33:39AM -0600, Tom Lendacky wrote:
> >>> On 1/14/25 01:27, Kirill A. Shutemov wrote:
> >>>> On Mon, Jan 13, 2025 at 02:47:56PM -0600, Tom Lendacky wrote:
> >>>>> On 1/13/25 07:14, Kirill A. Shutemov wrote:
> >>>>>> Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
> >>>>>>
> >>>>>> memremap(MEMREMAP_WB)
> >>>>>>   arch_memremap_wb()
> >>>>>>     ioremap_cache()
> >>>>>>       __ioremap_caller(.encrytped = false)
> >>>>>>
> >>>>>> In such cases, the IORES_MAP_ENCRYPTED flag on the memory will determine
> >>>>>> if the resulting mapping is encrypted or decrypted.
> >>>>>>
> >>>>>> Creating a decrypted mapping without explicit request from the caller is
> >>>>>> risky:
> >>>>>>
> >>>>>>   - It can inadvertently expose the guest's data and compromise the
> >>>>>>     guest.
> >>>>>>
> >>>>>>   - Accessing private memory via shared/decrypted mapping on TDX will
> >>>>>>     either trigger implicit conversion to shared or #VE (depending on
> >>>>>>     VMM implementation).
> >>>>>>
> >>>>>>     Implicit conversion is destructive: subsequent access to the same
> >>>>>>     memory via private mapping will trigger a hard-to-debug #VE crash.
> >>>>>>
> >>>>>> The kernel already provides a way to request decrypted mapping
> >>>>>> explicitly via the MEMREMAP_DEC flag.
> >>>>>>
> >>>>>> Modify memremap(MEMREMAP_WB) to produce encrypted/private mapping by
> >>>>>> default unless MEMREMAP_DEC is specified.
> >>>>>>
> >>>>>> Fix the crash due to #VE on kexec in TDX guests if CONFIG_EISA is enabled.
> >>>>>
> >>>>> This patch causes my bare-metal system to crash during boot when using
> >>>>> mem_encrypt=on:
> >>>>>
> >>>>> [    2.392934] efi: memattr: Entry type should be RuntimeServiceCode/Data
> >>>>> [    2.393731] efi: memattr: ! 0x214c42f01f1162a-0xee70ac7bd1a9c629 [type=2028324321|attr=0x6590648fa4209879]
> >>>>
> >>>> Could you try if this helps?
> >>>>
> >>>> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> >>>> index c38b1a335590..b5051dcb7c1d 100644
> >>>> --- a/drivers/firmware/efi/memattr.c
> >>>> +++ b/drivers/firmware/efi/memattr.c
> >>>> @@ -160,7 +160,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
> >>>>  	if (WARN_ON(!efi_enabled(EFI_MEMMAP)))
> >>>>  		return 0;
> >>>>  
> >>>> -	tbl = memremap(efi_mem_attr_table, tbl_size, MEMREMAP_WB);
> >>>> +	tbl = memremap(efi_mem_attr_table, tbl_size, MEMREMAP_WB | MEMREMAP_DEC);
> >>>
> >>> Well that would work for SME where EFI tables/data are not encrypted,
> >>> but will break for SEV where EFI tables/data are encrypted.
> >>
> >> Hm. Why would it break for SEV? It brings the situation back to what it
> >> was before the patch.
> > 
> > Ah, true. I can try it and see how much further SME gets. Hopefully it
> > doesn't turn into a whack-a-mole thing.
> 
> Unfortunately, it is turning into a whack-a-mole thing.
> 
> But it looks the following works for SME:
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 3c36f3f5e688..ff3cd5fc8508 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -505,7 +505,7 @@ EXPORT_SYMBOL(iounmap);
>  
>  void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags)
>  {
> -	if (flags & MEMREMAP_DEC)
> +	if (flags & MEMREMAP_DEC || cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
>  		return (void __force *)ioremap_cache(phys_addr, size);
>  
>  	return (void __force *)ioremap_encrypted(phys_addr, size);
> 
> 
> I haven't had a chance to test the series on SEV, yet.

Please do.

I am okay with the change above. Borislav, is it acceptable direction for
you?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov