Message-ID: <36684e56-242f-4161-b1b6-ca0cd21c0a8b@linux.microsoft.com>
Date: Mon, 18 Mar 2024 10:21:46 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: mhklinux@...look.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 decui@...rosoft.com, catalin.marinas@....com, will@...nel.org,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, arnd@...db.de, tytso@....edu,
 Jason@...c4.com, x86@...nel.org, linux-kernel@...r.kernel.org,
 linux-hyperv@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH v3 1/1] x86/hyperv: Use Hyper-V entropy to seed guest
 random number generator

On 3/18/2024 8:54 AM, mhkelley58@...il.com wrote:
> From: Michael Kelley <mhklinux@...look.com>
> 
> A Hyper-V host provides its guest VMs with entropy in a custom ACPI
> table named "OEM0".  The entropy bits are updated each time Hyper-V
> boots the VM, and are suitable for seeding the Linux guest random
> number generator (rng). See a brief description of OEM0 in [1].
> 
> Generation 2 VMs on Hyper-V use UEFI to boot. Existing EFI code in
> Linux seeds the rng with entropy bits from the EFI_RNG_PROTOCOL.
> Via this path, the rng is seeded very early during boot with good
> entropy. The ACPI OEM0 table provided in such VMs is an additional
> source of entropy.
> 
> Generation 1 VMs on Hyper-V boot from BIOS. For these VMs, Linux
> doesn't currently get any entropy from the Hyper-V host. While this
> is not fundamentally broken because Linux can generate its own entropy,
> using the Hyper-V host provided entropy would get the rng off to a
> better start and would do so earlier in the boot process.
> 
> Improve the rng seeding for Generation 1 VMs by having Hyper-V specific
> code in Linux take advantage of the OEM0 table to seed the rng. For
> Generation 2 VMs, use the OEM0 table to provide additional entropy
> beyond the EFI_RNG_PROTOCOL. Because the OEM0 table is custom to
> Hyper-V, parse it directly in the Hyper-V code in the Linux kernel
> and use add_bootloader_randomness() to add it to the rng. Once the
> entropy bits are read from OEM0, zero them out in the table so
> they don't appear in /sys/firmware/acpi/tables/OEM0 in the running
> VM. The zero'ing is done out of an abundance of caution to avoid
> potential security risks to the rng. Also set the OEM0 data length
> to zero so a kexec or other subsequent use of the table won't try
> to use the zero'ed bits.
> 
> [1] https://download.microsoft.com/download/1/c/9/1c9813b8-089c-4fef-b2ad-ad80e79403ba/Whitepaper%20-%20The%20Windows%2010%20random%20number%20generation%20infrastructure.pdf
> 
> Signed-off-by: Michael Kelley <mhklinux@...look.com>
> ---
> Changes in v3:
> * Removed restriction to just Generation 1 VMs. Generation 2 VMs
>   now also use the additional entropy even though they also get
>   initial entropy via EFI_RNG_PROTOCOL [Jason Donenfeld]
> * Process the OEM0 table on ARM64 systems in addition to x86/x64,
>   as a result of no longer excluding Generation 2 VM.
> * Enlarge the range of entropy byte counts that are considered valid
>   in the OEM0 table. New range is 8 to 4K; previously the range was
>   32 to 256. [Jason Donenfeld]
> * After processing the entropy bits in OEM0, also set the OEM0
>   table length to indicate that the entropy byte count is zero,
>   to prevent a subsequent kexec or other use of the table from
>   trying to use the zero'ed bits. [Jason Donenfeld]
> 
> Changes in v2:
> * Tweaked commit message [Wei Liu]
> * Removed message when OEM0 table isn't found. Added debug-level
>   message when OEM0 is successfully used to add randomness. [Wei Liu]
> 
>  arch/arm64/hyperv/mshyperv.c   |  2 +
>  arch/x86/kernel/cpu/mshyperv.c |  1 +
>  drivers/hv/hv_common.c         | 69 ++++++++++++++++++++++++++++++++++
>  include/asm-generic/mshyperv.h |  2 +
>  4 files changed, 74 insertions(+)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index f1b8a04ee9f2..c8193cec1b90 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -74,6 +74,8 @@ static int __init hyperv_init(void)
>  		return ret;
>  	}
>  
> +	ms_hyperv_late_init();
> +
>  	hyperv_initialized = true;
>  	return 0;
>  }
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 303fef824167..65c9cbdd2282 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -648,6 +648,7 @@ const __initconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
>  	.init.x2apic_available	= ms_hyperv_x2apic_available,
>  	.init.msi_ext_dest_id	= ms_hyperv_msi_ext_dest_id,
>  	.init.init_platform	= ms_hyperv_init_platform,
> +	.init.guest_late_init	= ms_hyperv_late_init,
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  	.runtime.sev_es_hcall_prepare = hv_sev_es_hcall_prepare,
>  	.runtime.sev_es_hcall_finish = hv_sev_es_hcall_finish,
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 0285a74363b3..724de94d885f 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -20,8 +20,11 @@
>  #include <linux/sched/task_stack.h>
>  #include <linux/panic_notifier.h>
>  #include <linux/ptrace.h>
> +#include <linux/random.h>
> +#include <linux/efi.h>
>  #include <linux/kdebug.h>
>  #include <linux/kmsg_dump.h>
> +#include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/dma-map-ops.h>
>  #include <linux/set_memory.h>
> @@ -347,6 +350,72 @@ int __init hv_common_init(void)
>  	return 0;
>  }
>  
> +void __init ms_hyperv_late_init(void)
> +{
> +	struct acpi_table_header *header;
> +	acpi_status status;
> +	u8 *randomdata;
> +	u32 length, i;
> +
> +	/*
> +	 * Seed the Linux random number generator with entropy provided by
> +	 * the Hyper-V host in ACPI table OEM0.
> +	 */
> +	if (!IS_ENABLED(CONFIG_ACPI))
> +		return;
> +
> +	status = acpi_get_table("OEM0", 0, &header);
> +	if (ACPI_FAILURE(status) || !header)
> +		return;
> +
> +	/*
> +	 * Since the "OEM0" table name is for OEM specific usage, verify
> +	 * that what we're seeing purports to be from Microsoft.
> +	 */
> +	if (strncmp(header->oem_table_id, "MICROSFT", 8))
> +		goto error;
> +
> +	/*
> +	 * Ensure the length is reasonable. Requiring at least 8 bytes and
> +	 * no more than 4K bytes is somewhat arbitrary and just protects
> +	 * against a malformed table. Hyper-V currently provides 64 bytes,
> +	 * but allow for a change in a later version.
> +	 */
> +	if (header->length < sizeof(*header) + 8 ||
> +	    header->length > sizeof(*header) + SZ_4K> +		goto error;
> +
> +	length = header->length - sizeof(*header);
> +	randomdata = (u8 *)(header + 1);
> +
> +	pr_debug("Hyper-V: Seeding rng with %d random bytes from ACPI table OEM0\n",
> +			length);
> +
> +	add_bootloader_randomness(randomdata, length);
> +
> +	/*
> +	 * To prevent the seed data from being visible in /sys/firmware/acpi,
> +	 * zero out the random data in the ACPI table and fixup the checksum.
> +	 * The zero'ing is done out of an abundance of caution in avoiding
> +	 * potential security risks to the rng. Similarly, reset the table
> +	 * length to just the header size so that a subsequent kexec doesn't
> +	 * try to use the zero'ed out random data.
> +	 */
> +	for (i = 0; i < length; i++) {
> +		header->checksum += randomdata[i];
> +		randomdata[i] = 0;
> +	}
> +
> +	for (i = 0; i < sizeof(header->length); i++)
> +		header->checksum += ((u8 *)&header->length)[i];
> +	header->length = sizeof(*header);
> +	for (i = 0; i < sizeof(header->length); i++)
> +		header->checksum -= ((u8 *)&header->length)[i];
> +
> +error:
> +	acpi_put_table(header);
> +}
> +
>  /*
>   * Hyper-V specific initialization and die code for
>   * individual CPUs that is common across all architectures.
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 430f0ae0dde2..e861223093df 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -193,6 +193,7 @@ extern u64 (*hv_read_reference_counter)(void);
>  
>  int __init hv_common_init(void);
>  void __init hv_common_free(void);
> +void __init ms_hyperv_late_init(void);
>  int hv_common_cpu_init(unsigned int cpu);
>  int hv_common_cpu_die(unsigned int cpu);
>  
> @@ -290,6 +291,7 @@ void hv_setup_dma_ops(struct device *dev, bool coherent);
>  static inline bool hv_is_hyperv_initialized(void) { return false; }
>  static inline bool hv_is_hibernation_supported(void) { return false; }
>  static inline void hyperv_cleanup(void) {}
> +static inline void ms_hyperv_late_init(void) {}
>  static inline bool hv_is_isolation_supported(void) { return false; }
>  static inline enum hv_isolation_type hv_get_isolation_type(void)
>  {

This patch looks good to me. The code comments were very helpful in explaining
what is going on.

Nuno

Powered by blists - more mailing lists