linux-kernel - Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

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]

Date: Thu, 2 May 2024 15:45:06 +0200
From: Borislav Petkov <bp@...en8.de>
To: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: adrian.hunter@...el.com, ashish.kalra@....com, bhe@...hat.com,
	dave.hansen@...ux.intel.com, elena.reshetova@...el.com,
	jun.nakajima@...el.com, kai.huang@...el.com,
	kexec@...ts.infradead.org, linux-coco@...ts.linux.dev,
	linux-kernel@...r.kernel.org, ltao@...hat.com, mingo@...hat.com,
	nik.borisov@...e.com, peterz@...radead.org, rafael@...nel.org,
	rick.p.edgecombe@...el.com,
	sathyanarayanan.kuppuswamy@...ux.intel.com, seanjc@...gle.com,
	tglx@...utronix.de, thomas.lendacky@....com, x86@...nel.org
Subject: Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted
 memory for kexec

On Sat, Apr 27, 2024 at 08:06:34PM +0300, Kirill A. Shutemov wrote:

> Subject: Re: [PATCHv10.1 09/18] x86/mm: Adding callbacks to prepare encrypted memory for kexec

s/Adding/Add/

> AMD SEV and Intel TDX guests allocate shared buffers for performing I/O.
> This is done by allocating pages normally from the buddy allocator and
> then converting them to shared using set_memory_decrypted().
> 
> On kexec, the second kernel is unaware of which memory has been
> converted in this manner. It only sees E820_TYPE_RAM. Accessing shared
> memory as private is fatal.
> 
> Therefore, the memory state must be reset to its original state before
> starting the new kernel with kexec.
> 
> The process of converting shared memory back to private occurs in two
> steps:
> 
> - enc_kexec_stop_conversion() stops new conversions.
> 
> - enc_kexec_unshare_mem() unshares all existing shared memory, reverting
>   it back to private.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>x
> Reviewed-by: Kai Huang <kai.huang@...el.com>
> Tested-by: Tao Liu <ltao@...hat.com>
> ---
>  arch/x86/include/asm/x86_init.h |  2 ++
>  arch/x86/kernel/crash.c         |  4 ++++
>  arch/x86/kernel/reboot.c        | 12 ++++++++++++
>  arch/x86/kernel/x86_init.c      |  4 ++++
>  4 files changed, 22 insertions(+)
> 
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 28ac3cb9b987..c731e6bc4343 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -155,6 +155,8 @@ struct x86_guest {
>  	int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
>  	bool (*enc_tlb_flush_required)(bool enc);
>  	bool (*enc_cache_flush_required)(void);
> +	void (*enc_kexec_stop_conversion)(bool crash);
> +	void (*enc_kexec_unshare_mem)(void);

This is all fine and dandy but those functions need documentation in the
kernel doc above it.

And if there's a "stop_conversion" function, then I'd expect there to be
a "start conversion" one.

I think you need to give those two better names to denote that they're
related, the order in which they should be called and why they're
separate.

>  };
>  
>  /**
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index e74d0c4286c1..f1b261be78b4 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -128,6 +128,10 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>  #ifdef CONFIG_HPET_TIMER
>  	hpet_disable();
>  #endif
> +
> +	x86_platform.guest.enc_kexec_stop_conversion(true);
> +	x86_platform.guest.enc_kexec_unshare_mem();
> +

You call them here back-to-back...

>  	crash_save_cpu(regs, safe_smp_processor_id());
>  }
>  
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f3130f762784..c1920ec34f0c 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -12,6 +12,7 @@
>  #include <linux/delay.h>
>  #include <linux/objtool.h>
>  #include <linux/pgtable.h>
> +#include <linux/kexec.h>
>  #include <acpi/reboot.h>
>  #include <asm/io.h>
>  #include <asm/apic.h>
> @@ -716,6 +717,14 @@ static void native_machine_emergency_restart(void)
>  
>  void native_machine_shutdown(void)
>  {
> +	/*
> +	 * Call enc_kexec_stop_conversion() while all CPUs are still active and
> +	 * interrupts are enabled. This will allow all in-flight memory
> +	 * conversions to finish cleanly.
> +	 */
> +	if (kexec_in_progress)
> +		x86_platform.guest.enc_kexec_stop_conversion(false);
> +
>  	/* Stop the cpus and apics */
>  #ifdef CONFIG_X86_IO_APIC
>  	/*
> @@ -752,6 +761,9 @@ void native_machine_shutdown(void)
>  #ifdef CONFIG_X86_64
>  	x86_platform.iommu_shutdown();
>  #endif
> +
> +	if (kexec_in_progress)
> +		x86_platform.guest.enc_kexec_unshare_mem();

.. but they're split here.

And I don't know why and nothing tells me...

Thx.

-- 
Regards/Gruss,
    Boris.

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