[PATCH v3 0/2] VM fork detection for RNG

[PATCH v3 0/2] VM fork detection for RNG

[Jason A. Donenfeld]

This small series picks up work from Amazon that seems to have stalled
out last year around this time: listening for the vmgenid ACPI
notification, and using it to "do something." Last year, folks proposed
a complicated userspace mmap chardev, which was frought with difficulty
and evidently abandoned. This year, instead, I have something much
simpler in mind: simply using those ACPI notifications to tell the RNG
to reinitialize safely, so we don't repeat random numbers in cloned,
forked, or rolled-back VM instances.

This series consists of two patches. The first one adds the right hooks
into the actual RNG, and the second is a driver for the ACPI notification.

Here's a little screencast showing it in action: https://data.zx2c4.com/vmgenid-appears-to-work.gif

As a side note, this series intentionally does _not_ focus on
notification of these events to userspace or to other kernel consumers.
Since these VM fork detection events first need to hit the RNG, we can
later talk about what sorts of notifications or mmap'd counters the RNG
should be making accessible to elsewhere. But that's a different sort of
project and ties into a lot of more complicated concerns beyond this
more basic patchset. So hopefully we can keep the discussion rather
focused here to this ACPI business.

Changes v2->v3:
- [Eric] Always put generation ID through the input pool, and then re-extract.
- [Lazlo] The ACPI bytes are just an opaque binary blob, rather than a real UUID.
Changes v1->v2:
- [Ard] Correct value of MODULE_LICENSE().
- [Ard] Use ordinary memory accesses instead of memcpy_fromio.
- [Ard] Make module a tristate and set MODULE_DEVICE_TABLE().
- [Ard] Free buffer after using.
- Use { } instead of { "", 0 }.
- Clean up interface into RNG.
- Minimize ACPI driver a bit.

In addition to the usual suspects, I'm CCing the original team from
Amazon who proposed this last year and the QEMU developers who added it
there, as well as the kernel Hyper-V maintainers, since this is
technically a Microsoft-proposed thing, though QEMU now implements it.

Cc: adrian@parity.io
Cc: dwmw@amazon.co.uk
Cc: graf@amazon.com
Cc: colmmacc@amazon.com
Cc: raduweis@amazon.com
Cc: berrange@redhat.com
Cc: lersek@redhat.com
Cc: imammedo@redhat.com
Cc: ehabkost@redhat.com
Cc: ben@skyportsystems.com
Cc: mst@redhat.com
Cc: kys@microsoft.com
Cc: haiyangz@microsoft.com
Cc: sthemmin@microsoft.com
Cc: wei.liu@kernel.org
Cc: decui@microsoft.com
Cc: linux@dominikbrodowski.net
Cc: ebiggers@kernel.org
Cc: ardb@kernel.org
Cc: jannh@google.com
Cc: gregkh@linuxfoundation.org
Cc: tytso@mit.edu

Jason A. Donenfeld (2):
  random: add mechanism for VM forks to reinitialize crng
  virt: vmgenid: introduce driver for reinitializing RNG on VM fork

 drivers/char/random.c  |  50 ++++++++++++-----
 drivers/virt/Kconfig   |   9 +++
 drivers/virt/Makefile  |   1 +
 drivers/virt/vmgenid.c | 121 +++++++++++++++++++++++++++++++++++++++++
 include/linux/random.h |   1 +
 5 files changed, 167 insertions(+), 15 deletions(-)
 create mode 100644 drivers/virt/vmgenid.c

-- 
2.35.1

[PATCH v3 1/2] random: add mechanism for VM forks to reinitialize crng

[Jason A. Donenfeld]

When a VM forks, we must immediately mix in additional information to
the stream of random output so that two forks or a rollback don't
produce the same stream of random numbers, which could have catastrophic
cryptographic consequences. This commit adds a simple API, add_vmfork_
randomness(), for that, by force reseeding the crng.

This has the added benefit of also draining the entropy pool and setting
its timer back, so that any old entropy that was there prior -- which
could have already been used by a different fork, or generally gone
stale -- does not contribute to the accounting of the next 256 bits.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Jann Horn <jannh@google.com>
Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 50 +++++++++++++++++++++++++++++-------------
 include/linux/random.h |  1 +
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 9fb06fc298d3..e8b84791cefe 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -289,14 +289,14 @@ static DEFINE_PER_CPU(struct crng, crngs) = {
 };
 
 /* Used by crng_reseed() to extract a new seed from the input pool. */
-static bool drain_entropy(void *buf, size_t nbytes);
+static bool drain_entropy(void *buf, size_t nbytes, bool force);
 
 /*
  * This extracts a new crng key from the input pool, but only if there is a
- * sufficient amount of entropy available, in order to mitigate bruteforcing
- * of newly added bits.
+ * sufficient amount of entropy available or force is true, in order to
+ * mitigate bruteforcing of newly added bits.
  */
-static void crng_reseed(void)
+static void crng_reseed(bool force)
 {
 	unsigned long flags;
 	unsigned long next_gen;
@@ -304,7 +304,7 @@ static void crng_reseed(void)
 	bool finalize_init = false;
 
 	/* Only reseed if we can, to prevent brute forcing a small amount of new bits. */
-	if (!drain_entropy(key, sizeof(key)))
+	if (!drain_entropy(key, sizeof(key), force))
 		return;
 
 	/*
@@ -406,7 +406,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
 	 * in turn bumps the generation counter that we check below.
 	 */
 	if (unlikely(time_after(jiffies, READ_ONCE(base_crng.birth) + CRNG_RESEED_INTERVAL)))
-		crng_reseed();
+		crng_reseed(false);
 
 	local_lock_irqsave(&crngs.lock, flags);
 	crng = raw_cpu_ptr(&crngs);
@@ -771,10 +771,10 @@ EXPORT_SYMBOL(get_random_bytes_arch);
  *
  * Finally, extract entropy via these two, with the latter one
  * setting the entropy count to zero and extracting only if there
- * is POOL_MIN_BITS entropy credited prior:
+ * is POOL_MIN_BITS entropy credited prior or force is true:
  *
  *     static void extract_entropy(void *buf, size_t nbytes)
- *     static bool drain_entropy(void *buf, size_t nbytes)
+ *     static bool drain_entropy(void *buf, size_t nbytes, bool force)
  *
  **********************************************************************/
 
@@ -832,7 +832,7 @@ static void credit_entropy_bits(size_t nbits)
 	} while (cmpxchg(&input_pool.entropy_count, orig, entropy_count) != orig);
 
 	if (crng_init < 2 && entropy_count >= POOL_MIN_BITS)
-		crng_reseed();
+		crng_reseed(false);
 }
 
 /*
@@ -882,16 +882,16 @@ static void extract_entropy(void *buf, size_t nbytes)
 }
 
 /*
- * First we make sure we have POOL_MIN_BITS of entropy in the pool, and then we
- * set the entropy count to zero (but don't actually touch any data). Only then
- * can we extract a new key with extract_entropy().
+ * First we make sure we have POOL_MIN_BITS of entropy in the pool unless force
+ * is true, and then we set the entropy count to zero (but don't actually touch
+ * any data). Only then can we extract a new key with extract_entropy().
  */
-static bool drain_entropy(void *buf, size_t nbytes)
+static bool drain_entropy(void *buf, size_t nbytes, bool force)
 {
 	unsigned int entropy_count;
 	do {
 		entropy_count = READ_ONCE(input_pool.entropy_count);
-		if (entropy_count < POOL_MIN_BITS)
+		if (!force && entropy_count < POOL_MIN_BITS)
 			return false;
 	} while (cmpxchg(&input_pool.entropy_count, entropy_count, 0) != entropy_count);
 	extract_entropy(buf, nbytes);
@@ -915,6 +915,7 @@ static bool drain_entropy(void *buf, size_t nbytes)
  *	void add_hwgenerator_randomness(const void *buffer, size_t count,
  *					size_t entropy);
  *	void add_bootloader_randomness(const void *buf, size_t size);
+ *	void add_vmfork_randomness(const void *unique_vm_id, size_t size);
  *	void add_interrupt_randomness(int irq);
  *
  * add_device_randomness() adds data to the input pool that
@@ -946,6 +947,10 @@ static bool drain_entropy(void *buf, size_t nbytes)
  * add_device_randomness(), depending on whether or not the configuration
  * option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
  *
+ * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
+ * representing the current instance of a VM to the pool, without crediting,
+ * and then force-reseeds the crng so that it takes effect immediately.
+ *
  * add_interrupt_randomness() uses the interrupt timing as random
  * inputs to the entropy pool. Using the cycle counters and the irq source
  * as inputs, it feeds the input pool roughly once a second or after 64
@@ -1175,6 +1180,21 @@ void add_bootloader_randomness(const void *buf, size_t size)
 }
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
+/*
+ * Handle a new unique VM ID, which is unique, not secret, so we
+ * don't credit it, but we do immediately force a reseed after so
+ * that it's used by the crng posthaste.
+ */
+void add_vmfork_randomness(const void *unique_vm_id, size_t size)
+{
+	add_device_randomness(unique_vm_id, size);
+	if (crng_ready()) {
+		crng_reseed(true);
+		pr_notice("crng reseeded due to virtual machine fork\n");
+	}
+}
+EXPORT_SYMBOL_GPL(add_vmfork_randomness);
+
 struct fast_pool {
 	union {
 		u32 pool32[4];
@@ -1564,7 +1584,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 			return -EPERM;
 		if (crng_init < 2)
 			return -ENODATA;
-		crng_reseed();
+		crng_reseed(false);
 		return 0;
 	default:
 		return -EINVAL;
diff --git a/include/linux/random.h b/include/linux/random.h
index 6148b8d1ccf3..51b8ed797732 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,6 +34,7 @@ extern void add_input_randomness(unsigned int type, unsigned int code,
 extern void add_interrupt_randomness(int irq) __latent_entropy;
 extern void add_hwgenerator_randomness(const void *buffer, size_t count,
 				       size_t entropy);
+extern void add_vmfork_randomness(const void *unique_vm_id, size_t size);
 
 extern void get_random_bytes(void *buf, size_t nbytes);
 extern int wait_for_random_bytes(void);
-- 
2.35.1

Re: [PATCH v3 1/2] random: add mechanism for VM forks to reinitialize crng

[Ard Biesheuvel]

On Thu, 24 Feb 2022 at 14:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> When a VM forks, we must immediately mix in additional information to
> the stream of random output so that two forks or a rollback don't
> produce the same stream of random numbers, which could have catastrophic
> cryptographic consequences. This commit adds a simple API, add_vmfork_
> randomness(), for that, by force reseeding the crng.
>
> This has the added benefit of also draining the entropy pool and setting
> its timer back, so that any old entropy that was there prior -- which
> could have already been used by a different fork, or generally gone
> stale -- does not contribute to the accounting of the next 256 bits.
>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Jann Horn <jannh@google.com>
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  drivers/char/random.c  | 50 +++++++++++++++++++++++++++++-------------
>  include/linux/random.h |  1 +
>  2 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 9fb06fc298d3..e8b84791cefe 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -289,14 +289,14 @@ static DEFINE_PER_CPU(struct crng, crngs) = {
>  };
>
>  /* Used by crng_reseed() to extract a new seed from the input pool. */
> -static bool drain_entropy(void *buf, size_t nbytes);
> +static bool drain_entropy(void *buf, size_t nbytes, bool force);
>
>  /*
>   * This extracts a new crng key from the input pool, but only if there is a
> - * sufficient amount of entropy available, in order to mitigate bruteforcing
> - * of newly added bits.
> + * sufficient amount of entropy available or force is true, in order to
> + * mitigate bruteforcing of newly added bits.
>   */
> -static void crng_reseed(void)
> +static void crng_reseed(bool force)
>  {
>         unsigned long flags;
>         unsigned long next_gen;
> @@ -304,7 +304,7 @@ static void crng_reseed(void)
>         bool finalize_init = false;
>
>         /* Only reseed if we can, to prevent brute forcing a small amount of new bits. */
> -       if (!drain_entropy(key, sizeof(key)))
> +       if (!drain_entropy(key, sizeof(key), force))
>                 return;
>
>         /*
> @@ -406,7 +406,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
>          * in turn bumps the generation counter that we check below.
>          */
>         if (unlikely(time_after(jiffies, READ_ONCE(base_crng.birth) + CRNG_RESEED_INTERVAL)))
> -               crng_reseed();
> +               crng_reseed(false);
>
>         local_lock_irqsave(&crngs.lock, flags);
>         crng = raw_cpu_ptr(&crngs);
> @@ -771,10 +771,10 @@ EXPORT_SYMBOL(get_random_bytes_arch);
>   *
>   * Finally, extract entropy via these two, with the latter one
>   * setting the entropy count to zero and extracting only if there
> - * is POOL_MIN_BITS entropy credited prior:
> + * is POOL_MIN_BITS entropy credited prior or force is true:
>   *
>   *     static void extract_entropy(void *buf, size_t nbytes)
> - *     static bool drain_entropy(void *buf, size_t nbytes)
> + *     static bool drain_entropy(void *buf, size_t nbytes, bool force)
>   *
>   **********************************************************************/
>
> @@ -832,7 +832,7 @@ static void credit_entropy_bits(size_t nbits)
>         } while (cmpxchg(&input_pool.entropy_count, orig, entropy_count) != orig);
>
>         if (crng_init < 2 && entropy_count >= POOL_MIN_BITS)
> -               crng_reseed();
> +               crng_reseed(false);
>  }
>
>  /*
> @@ -882,16 +882,16 @@ static void extract_entropy(void *buf, size_t nbytes)
>  }
>
>  /*
> - * First we make sure we have POOL_MIN_BITS of entropy in the pool, and then we
> - * set the entropy count to zero (but don't actually touch any data). Only then
> - * can we extract a new key with extract_entropy().
> + * First we make sure we have POOL_MIN_BITS of entropy in the pool unless force
> + * is true, and then we set the entropy count to zero (but don't actually touch
> + * any data). Only then can we extract a new key with extract_entropy().
>   */
> -static bool drain_entropy(void *buf, size_t nbytes)
> +static bool drain_entropy(void *buf, size_t nbytes, bool force)
>  {
>         unsigned int entropy_count;
>         do {
>                 entropy_count = READ_ONCE(input_pool.entropy_count);
> -               if (entropy_count < POOL_MIN_BITS)
> +               if (!force && entropy_count < POOL_MIN_BITS)
>                         return false;
>         } while (cmpxchg(&input_pool.entropy_count, entropy_count, 0) != entropy_count);
>         extract_entropy(buf, nbytes);
> @@ -915,6 +915,7 @@ static bool drain_entropy(void *buf, size_t nbytes)
>   *     void add_hwgenerator_randomness(const void *buffer, size_t count,
>   *                                     size_t entropy);
>   *     void add_bootloader_randomness(const void *buf, size_t size);
> + *     void add_vmfork_randomness(const void *unique_vm_id, size_t size);
>   *     void add_interrupt_randomness(int irq);
>   *
>   * add_device_randomness() adds data to the input pool that
> @@ -946,6 +947,10 @@ static bool drain_entropy(void *buf, size_t nbytes)
>   * add_device_randomness(), depending on whether or not the configuration
>   * option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
>   *
> + * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
> + * representing the current instance of a VM to the pool, without crediting,
> + * and then force-reseeds the crng so that it takes effect immediately.
> + *
>   * add_interrupt_randomness() uses the interrupt timing as random
>   * inputs to the entropy pool. Using the cycle counters and the irq source
>   * as inputs, it feeds the input pool roughly once a second or after 64
> @@ -1175,6 +1180,21 @@ void add_bootloader_randomness(const void *buf, size_t size)
>  }
>  EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>
> +/*
> + * Handle a new unique VM ID, which is unique, not secret, so we
> + * don't credit it, but we do immediately force a reseed after so
> + * that it's used by the crng posthaste.
> + */
> +void add_vmfork_randomness(const void *unique_vm_id, size_t size)
> +{
> +       add_device_randomness(unique_vm_id, size);
> +       if (crng_ready()) {
> +               crng_reseed(true);
> +               pr_notice("crng reseeded due to virtual machine fork\n");
> +       }
> +}
> +EXPORT_SYMBOL_GPL(add_vmfork_randomness);
> +
>  struct fast_pool {
>         union {
>                 u32 pool32[4];
> @@ -1564,7 +1584,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>                         return -EPERM;
>                 if (crng_init < 2)
>                         return -ENODATA;
> -               crng_reseed();
> +               crng_reseed(false);
>                 return 0;
>         default:
>                 return -EINVAL;
> diff --git a/include/linux/random.h b/include/linux/random.h
> index 6148b8d1ccf3..51b8ed797732 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -34,6 +34,7 @@ extern void add_input_randomness(unsigned int type, unsigned int code,
>  extern void add_interrupt_randomness(int irq) __latent_entropy;
>  extern void add_hwgenerator_randomness(const void *buffer, size_t count,
>                                        size_t entropy);
> +extern void add_vmfork_randomness(const void *unique_vm_id, size_t size);
>
>  extern void get_random_bytes(void *buf, size_t nbytes);
>  extern int wait_for_random_bytes(void);
> --
> 2.35.1
>

Re: [PATCH v3 1/2] random: add mechanism for VM forks to reinitialize crng

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 12:26 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Feb 2022 at 14:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > When a VM forks, we must immediately mix in additional information to
> > the stream of random output so that two forks or a rollback don't
> > produce the same stream of random numbers, which could have catastrophic
> > cryptographic consequences. This commit adds a simple API, add_vmfork_
> > randomness(), for that, by force reseeding the crng.
> >
> > This has the added benefit of also draining the entropy pool and setting
> > its timer back, so that any old entropy that was there prior -- which
> > could have already been used by a different fork, or generally gone
> > stale -- does not contribute to the accounting of the next 256 bits.
> >
> > Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Eric Biggers <ebiggers@google.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>

Okay if I treat this as a Reviewed-by instead?

Re: [PATCH v3 1/2] random: add mechanism for VM forks to reinitialize crng

[Ard Biesheuvel]

On Fri, 25 Feb 2022 at 12:44, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Fri, Feb 25, 2022 at 12:26 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 24 Feb 2022 at 14:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > When a VM forks, we must immediately mix in additional information to
> > > the stream of random output so that two forks or a rollback don't
> > > produce the same stream of random numbers, which could have catastrophic
> > > cryptographic consequences. This commit adds a simple API, add_vmfork_
> > > randomness(), for that, by force reseeding the crng.
> > >
> > > This has the added benefit of also draining the entropy pool and setting
> > > its timer back, so that any old entropy that was there prior -- which
> > > could have already been used by a different fork, or generally gone
> > > stale -- does not contribute to the accounting of the next 256 bits.
> > >
> > > Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> > > Cc: Theodore Ts'o <tytso@mit.edu>
> > > Cc: Jann Horn <jannh@google.com>
> > > Cc: Eric Biggers <ebiggers@google.com>
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> Okay if I treat this as a Reviewed-by instead?

Sure no problem.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

[PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

VM Generation ID is a feature from Microsoft, described at
<https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
<https://aka.ms/win10rng>, as:

    If the OS is running in a VM, there is a problem that most
    hypervisors can snapshot the state of the machine and later rewind
    the VM state to the saved state. This results in the machine running
    a second time with the exact same RNG state, which leads to serious
    security problems.  To reduce the window of vulnerability, Windows
    10 on a Hyper-V VM will detect when the VM state is reset, retrieve
    a unique (not random) value from the hypervisor, and reseed the root
    RNG with that unique value.  This does not eliminate the
    vulnerability, but it greatly reduces the time during which the RNG
    system will produce the same outputs as it did during a previous
    instantiation of the same VM state.

Linux has the same issue, and given that vmgenid is supported already by
multiple hypervisors, we can implement more or less the same solution.
So this commit wires up the vmgenid ACPI notification to the RNG's newly
added add_vmfork_randomness() function.

It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
After setting that, use `savevm` in the monitor to save the VM state,
then quit QEMU, start it again, and use `loadvm`. That will trigger this
driver's notify function, which hands the new UUID to the RNG. This is
described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
And there are hooks for this in libvirt as well, described in
<https://libvirt.org/formatdomain.html#general-metadata>.

Note, however, that the treatment of this as a UUID is considered to be
an accidental QEMU nuance, per
<https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
so this driver simply treats these bytes as an opaque 128-bit binary
blob, as per the spec. This doesn't really make a difference anyway,
considering that's how it ends up when handed to the RNG in the end.

This driver builds on prior work from Adrian Catangiu at Amazon, and it
is my hope that that team can resume maintenance of this driver.

Cc: Adrian Catangiu <adrian@parity.io>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/virt/Kconfig   |   9 +++
 drivers/virt/Makefile  |   1 +
 drivers/virt/vmgenid.c | 121 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/virt/vmgenid.c

diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 8061e8ef449f..d3276dc2095c 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
 
 if VIRT_DRIVERS
 
+config VMGENID
+	tristate "Virtual Machine Generation ID driver"
+	default y
+	depends on ACPI
+	help
+	  Say Y here to use the hypervisor-provided Virtual Machine Generation ID
+	  to reseed the RNG when the VM is cloned. This is highly recommended if
+	  you intend to do any rollback / cloning / snapshotting of VMs.
+
 config FSL_HV_MANAGER
 	tristate "Freescale hypervisor management driver"
 	depends on FSL_SOC
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 3e272ea60cd9..108d0ffcc9aa 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -4,6 +4,7 @@
 #
 
 obj-$(CONFIG_FSL_HV_MANAGER)	+= fsl_hypervisor.o
+obj-$(CONFIG_VMGENID)		+= vmgenid.o
 obj-y				+= vboxguest/
 
 obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
new file mode 100644
index 000000000000..5da4dc8f25e3
--- /dev/null
+++ b/drivers/virt/vmgenid.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Virtual Machine Generation ID driver
+ *
+ * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2020 Amazon. All rights reserved.
+ * Copyright (C) 2018 Red Hat Inc. All rights reserved.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/random.h>
+
+ACPI_MODULE_NAME("vmgenid");
+
+enum { VMGENID_SIZE = 16 };
+
+static struct {
+	u8 this_id[VMGENID_SIZE];
+	u8 *next_id;
+} state;
+
+static int vmgenid_acpi_add(struct acpi_device *device)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
+	union acpi_object *pss;
+	phys_addr_t phys_addr;
+	acpi_status status;
+	int ret = 0;
+
+	if (!device)
+		return -EINVAL;
+
+	status = acpi_evaluate_object(device->handle, "ADDR", NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
+		return -ENODEV;
+	}
+	pss = buffer.pointer;
+	if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2 ||
+	    pss->package.elements[0].type != ACPI_TYPE_INTEGER ||
+	    pss->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	phys_addr = (pss->package.elements[0].integer.value << 0) |
+		    (pss->package.elements[1].integer.value << 32);
+	state.next_id = acpi_os_map_memory(phys_addr, VMGENID_SIZE);
+	if (!state.next_id) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	device->driver_data = &state;
+
+	memcpy(state.this_id, state.next_id, sizeof(state.this_id));
+	add_device_randomness(state.this_id, sizeof(state.this_id));
+
+out:
+	ACPI_FREE(buffer.pointer);
+	return ret;
+}
+
+static int vmgenid_acpi_remove(struct acpi_device *device)
+{
+	if (!device || acpi_driver_data(device) != &state)
+		return -EINVAL;
+	device->driver_data = NULL;
+	if (state.next_id)
+		acpi_os_unmap_memory(state.next_id, VMGENID_SIZE);
+	state.next_id = NULL;
+	return 0;
+}
+
+static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
+{
+	u8 old_id[VMGENID_SIZE];
+
+	if (!device || acpi_driver_data(device) != &state)
+		return;
+	memcpy(old_id, state.this_id, sizeof(old_id));
+	memcpy(state.this_id, state.next_id, sizeof(state.this_id));
+	if (!memcmp(old_id, state.this_id, sizeof(old_id)))
+		return;
+	add_vmfork_randomness(state.this_id, sizeof(state.this_id));
+}
+
+static const struct acpi_device_id vmgenid_ids[] = {
+	{"VMGENID", 0},
+	{"QEMUVGID", 0},
+	{ },
+};
+
+static struct acpi_driver acpi_driver = {
+	.name = "vm_generation_id",
+	.ids = vmgenid_ids,
+	.owner = THIS_MODULE,
+	.ops = {
+		.add = vmgenid_acpi_add,
+		.remove = vmgenid_acpi_remove,
+		.notify = vmgenid_acpi_notify,
+	}
+};
+
+static int __init vmgenid_init(void)
+{
+	return acpi_bus_register_driver(&acpi_driver);
+}
+
+static void __exit vmgenid_exit(void)
+{
+	acpi_bus_unregister_driver(&acpi_driver);
+}
+
+module_init(vmgenid_init);
+module_exit(vmgenid_exit);
+
+MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
+MODULE_DESCRIPTION("Virtual Machine Generation ID");
+MODULE_LICENSE("GPL v2");
-- 
2.35.1

Re: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Laszlo Ersek]

On 02/24/22 14:39, Jason A. Donenfeld wrote:
> VM Generation ID is a feature from Microsoft, described at
> <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> <https://aka.ms/win10rng>, as:
> 
>     If the OS is running in a VM, there is a problem that most
>     hypervisors can snapshot the state of the machine and later rewind
>     the VM state to the saved state. This results in the machine running
>     a second time with the exact same RNG state, which leads to serious
>     security problems.  To reduce the window of vulnerability, Windows
>     10 on a Hyper-V VM will detect when the VM state is reset, retrieve
>     a unique (not random) value from the hypervisor, and reseed the root
>     RNG with that unique value.  This does not eliminate the
>     vulnerability, but it greatly reduces the time during which the RNG
>     system will produce the same outputs as it did during a previous
>     instantiation of the same VM state.
> 
> Linux has the same issue, and given that vmgenid is supported already by
> multiple hypervisors, we can implement more or less the same solution.
> So this commit wires up the vmgenid ACPI notification to the RNG's newly
> added add_vmfork_randomness() function.
> 
> It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> After setting that, use `savevm` in the monitor to save the VM state,
> then quit QEMU, start it again, and use `loadvm`. That will trigger this
> driver's notify function, which hands the new UUID to the RNG. This is
> described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> And there are hooks for this in libvirt as well, described in
> <https://libvirt.org/formatdomain.html#general-metadata>.
> 
> Note, however, that the treatment of this as a UUID is considered to be
> an accidental QEMU nuance, per
> <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> so this driver simply treats these bytes as an opaque 128-bit binary
> blob, as per the spec. This doesn't really make a difference anyway,
> considering that's how it ends up when handed to the RNG in the end.
> 
> This driver builds on prior work from Adrian Catangiu at Amazon, and it
> is my hope that that team can resume maintenance of this driver.
> 
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/virt/Kconfig   |   9 +++
>  drivers/virt/Makefile  |   1 +
>  drivers/virt/vmgenid.c | 121 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 drivers/virt/vmgenid.c
> 
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 8061e8ef449f..d3276dc2095c 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
>  
>  if VIRT_DRIVERS
>  
> +config VMGENID
> +	tristate "Virtual Machine Generation ID driver"
> +	default y
> +	depends on ACPI
> +	help
> +	  Say Y here to use the hypervisor-provided Virtual Machine Generation ID
> +	  to reseed the RNG when the VM is cloned. This is highly recommended if
> +	  you intend to do any rollback / cloning / snapshotting of VMs.
> +
>  config FSL_HV_MANAGER
>  	tristate "Freescale hypervisor management driver"
>  	depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index 3e272ea60cd9..108d0ffcc9aa 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  obj-$(CONFIG_FSL_HV_MANAGER)	+= fsl_hypervisor.o
> +obj-$(CONFIG_VMGENID)		+= vmgenid.o
>  obj-y				+= vboxguest/
>  
>  obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> new file mode 100644
> index 000000000000..5da4dc8f25e3
> --- /dev/null
> +++ b/drivers/virt/vmgenid.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual Machine Generation ID driver
> + *
> + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + * Copyright (C) 2020 Amazon. All rights reserved.
> + * Copyright (C) 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/random.h>
> +
> +ACPI_MODULE_NAME("vmgenid");
> +
> +enum { VMGENID_SIZE = 16 };
> +
> +static struct {
> +	u8 this_id[VMGENID_SIZE];
> +	u8 *next_id;
> +} state;
> +
> +static int vmgenid_acpi_add(struct acpi_device *device)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
> +	union acpi_object *pss;
> +	phys_addr_t phys_addr;
> +	acpi_status status;
> +	int ret = 0;
> +
> +	if (!device)
> +		return -EINVAL;
> +
> +	status = acpi_evaluate_object(device->handle, "ADDR", NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> +		return -ENODEV;
> +	}
> +	pss = buffer.pointer;
> +	if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2 ||
> +	    pss->package.elements[0].type != ACPI_TYPE_INTEGER ||
> +	    pss->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	phys_addr = (pss->package.elements[0].integer.value << 0) |
> +		    (pss->package.elements[1].integer.value << 32);
> +	state.next_id = acpi_os_map_memory(phys_addr, VMGENID_SIZE);
> +	if (!state.next_id) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	device->driver_data = &state;
> +
> +	memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> +	add_device_randomness(state.this_id, sizeof(state.this_id));
> +
> +out:
> +	ACPI_FREE(buffer.pointer);
> +	return ret;
> +}
> +
> +static int vmgenid_acpi_remove(struct acpi_device *device)
> +{
> +	if (!device || acpi_driver_data(device) != &state)
> +		return -EINVAL;
> +	device->driver_data = NULL;
> +	if (state.next_id)
> +		acpi_os_unmap_memory(state.next_id, VMGENID_SIZE);
> +	state.next_id = NULL;
> +	return 0;
> +}
> +
> +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> +{
> +	u8 old_id[VMGENID_SIZE];
> +
> +	if (!device || acpi_driver_data(device) != &state)
> +		return;
> +	memcpy(old_id, state.this_id, sizeof(old_id));
> +	memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> +	if (!memcmp(old_id, state.this_id, sizeof(old_id)))
> +		return;
> +	add_vmfork_randomness(state.this_id, sizeof(state.this_id));
> +}
> +
> +static const struct acpi_device_id vmgenid_ids[] = {
> +	{"VMGENID", 0},
> +	{"QEMUVGID", 0},
> +	{ },
> +};
> +
> +static struct acpi_driver acpi_driver = {
> +	.name = "vm_generation_id",
> +	.ids = vmgenid_ids,
> +	.owner = THIS_MODULE,
> +	.ops = {
> +		.add = vmgenid_acpi_add,
> +		.remove = vmgenid_acpi_remove,
> +		.notify = vmgenid_acpi_notify,
> +	}
> +};
> +
> +static int __init vmgenid_init(void)
> +{
> +	return acpi_bus_register_driver(&acpi_driver);
> +}
> +
> +static void __exit vmgenid_exit(void)
> +{
> +	acpi_bus_unregister_driver(&acpi_driver);
> +}
> +
> +module_init(vmgenid_init);
> +module_exit(vmgenid_exit);
> +
> +MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DESCRIPTION("Virtual Machine Generation ID");
> +MODULE_LICENSE("GPL v2");
> 

I'm not an experienced reviewer for the kernel.

I've made an effort to check several -- although not all -- aspects of
this patch, and it looks OK to me.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Ard Biesheuvel]

On Thu, 24 Feb 2022 at 14:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> VM Generation ID is a feature from Microsoft, described at
> <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> <https://aka.ms/win10rng>, as:
>
>     If the OS is running in a VM, there is a problem that most
>     hypervisors can snapshot the state of the machine and later rewind
>     the VM state to the saved state. This results in the machine running
>     a second time with the exact same RNG state, which leads to serious
>     security problems.  To reduce the window of vulnerability, Windows
>     10 on a Hyper-V VM will detect when the VM state is reset, retrieve
>     a unique (not random) value from the hypervisor, and reseed the root
>     RNG with that unique value.  This does not eliminate the
>     vulnerability, but it greatly reduces the time during which the RNG
>     system will produce the same outputs as it did during a previous
>     instantiation of the same VM state.
>
> Linux has the same issue, and given that vmgenid is supported already by
> multiple hypervisors, we can implement more or less the same solution.
> So this commit wires up the vmgenid ACPI notification to the RNG's newly
> added add_vmfork_randomness() function.
>
> It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> After setting that, use `savevm` in the monitor to save the VM state,
> then quit QEMU, start it again, and use `loadvm`. That will trigger this
> driver's notify function, which hands the new UUID to the RNG. This is
> described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> And there are hooks for this in libvirt as well, described in
> <https://libvirt.org/formatdomain.html#general-metadata>.
>
> Note, however, that the treatment of this as a UUID is considered to be
> an accidental QEMU nuance, per
> <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> so this driver simply treats these bytes as an opaque 128-bit binary
> blob, as per the spec. This doesn't really make a difference anyway,
> considering that's how it ends up when handed to the RNG in the end.
>
> This driver builds on prior work from Adrian Catangiu at Amazon, and it
> is my hope that that team can resume maintenance of this driver.
>
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/virt/Kconfig   |   9 +++
>  drivers/virt/Makefile  |   1 +
>  drivers/virt/vmgenid.c | 121 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 drivers/virt/vmgenid.c
>
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 8061e8ef449f..d3276dc2095c 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig

drivers/virt does not have a maintainer and this code needs one.

> @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
>
>  if VIRT_DRIVERS
>
> +config VMGENID
> +       tristate "Virtual Machine Generation ID driver"
> +       default y

Please make this default m - this code can run as a module and the
feature it relies on is discoverable by udev

> +       depends on ACPI
> +       help
> +         Say Y here to use the hypervisor-provided Virtual Machine Generation ID
> +         to reseed the RNG when the VM is cloned. This is highly recommended if
> +         you intend to do any rollback / cloning / snapshotting of VMs.
> +
>  config FSL_HV_MANAGER
>         tristate "Freescale hypervisor management driver"
>         depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index 3e272ea60cd9..108d0ffcc9aa 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -4,6 +4,7 @@
>  #
>
>  obj-$(CONFIG_FSL_HV_MANAGER)   += fsl_hypervisor.o
> +obj-$(CONFIG_VMGENID)          += vmgenid.o
>  obj-y                          += vboxguest/
>
>  obj-$(CONFIG_NITRO_ENCLAVES)   += nitro_enclaves/
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> new file mode 100644
> index 000000000000..5da4dc8f25e3
> --- /dev/null
> +++ b/drivers/virt/vmgenid.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual Machine Generation ID driver
> + *
> + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + * Copyright (C) 2020 Amazon. All rights reserved.
> + * Copyright (C) 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/random.h>
> +
> +ACPI_MODULE_NAME("vmgenid");
> +
> +enum { VMGENID_SIZE = 16 };
> +
> +static struct {
> +       u8 this_id[VMGENID_SIZE];
> +       u8 *next_id;
> +} state;
> +

This state is singular


> +static int vmgenid_acpi_add(struct acpi_device *device)
> +{

... whereas this may be called for multiple instances of the device.
This likely makes no sense, so it is better to reject it here.

Otherwise, the state should be allocated dynamically.

> +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
> +       union acpi_object *pss;
> +       phys_addr_t phys_addr;
> +       acpi_status status;
> +       int ret = 0;
> +
> +       if (!device)
> +               return -EINVAL;
> +
> +       status = acpi_evaluate_object(device->handle, "ADDR", NULL, &buffer);
> +       if (ACPI_FAILURE(status)) {
> +               ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> +               return -ENODEV;
> +       }
> +       pss = buffer.pointer;
> +       if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2 ||
> +           pss->package.elements[0].type != ACPI_TYPE_INTEGER ||
> +           pss->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       phys_addr = (pss->package.elements[0].integer.value << 0) |
> +                   (pss->package.elements[1].integer.value << 32);
> +       state.next_id = acpi_os_map_memory(phys_addr, VMGENID_SIZE);

No need to use acpi_os_map_memory() here, plain memremap() should be fine.

> +       if (!state.next_id) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +       device->driver_data = &state;
> +
> +       memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> +       add_device_randomness(state.this_id, sizeof(state.this_id));
> +
> +out:
> +       ACPI_FREE(buffer.pointer);
> +       return ret;
> +}
> +
> +static int vmgenid_acpi_remove(struct acpi_device *device)
> +{
> +       if (!device || acpi_driver_data(device) != &state)
> +               return -EINVAL;
> +       device->driver_data = NULL;
> +       if (state.next_id)
> +               acpi_os_unmap_memory(state.next_id, VMGENID_SIZE);

memunmap() here

> +       state.next_id = NULL;
> +       return 0;
> +}
> +
> +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> +{
> +       u8 old_id[VMGENID_SIZE];
> +
> +       if (!device || acpi_driver_data(device) != &state)
> +               return;
> +       memcpy(old_id, state.this_id, sizeof(old_id));
> +       memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> +       if (!memcmp(old_id, state.this_id, sizeof(old_id)))
> +               return;

Is this little dance really necessary? I.e., can we just do

add_vmfork_randomness(state.next_id, VMGENID_SIZE)

and be done with it?

And if we cannot, is it ok to just return without some kind of
diagnostic message?

> +       add_vmfork_randomness(state.this_id, sizeof(state.this_id));
> +}
> +
> +static const struct acpi_device_id vmgenid_ids[] = {
> +       {"VMGENID", 0},
> +       {"QEMUVGID", 0},
> +       { },
> +};
> +
> +static struct acpi_driver acpi_driver = {
> +       .name = "vm_generation_id",
> +       .ids = vmgenid_ids,
> +       .owner = THIS_MODULE,
> +       .ops = {
> +               .add = vmgenid_acpi_add,
> +               .remove = vmgenid_acpi_remove,
> +               .notify = vmgenid_acpi_notify,
> +       }
> +};
> +
> +static int __init vmgenid_init(void)
> +{
> +       return acpi_bus_register_driver(&acpi_driver);
> +}
> +
> +static void __exit vmgenid_exit(void)
> +{
> +       acpi_bus_unregister_driver(&acpi_driver);
> +}
> +
> +module_init(vmgenid_init);
> +module_exit(vmgenid_exit);
> +
> +MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DESCRIPTION("Virtual Machine Generation ID");
> +MODULE_LICENSE("GPL v2");
> --
> 2.35.1
>

Re: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Michael S. Tsirkin]

On Fri, Feb 25, 2022 at 12:24:05PM +0100, Ard Biesheuvel wrote:
> On Thu, 24 Feb 2022 at 14:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > VM Generation ID is a feature from Microsoft, described at
> > <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> > Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> > <https://aka.ms/win10rng>, as:
> >
> >     If the OS is running in a VM, there is a problem that most
> >     hypervisors can snapshot the state of the machine and later rewind
> >     the VM state to the saved state. This results in the machine running
> >     a second time with the exact same RNG state, which leads to serious
> >     security problems.  To reduce the window of vulnerability, Windows
> >     10 on a Hyper-V VM will detect when the VM state is reset, retrieve
> >     a unique (not random) value from the hypervisor, and reseed the root
> >     RNG with that unique value.  This does not eliminate the
> >     vulnerability, but it greatly reduces the time during which the RNG
> >     system will produce the same outputs as it did during a previous
> >     instantiation of the same VM state.
> >
> > Linux has the same issue, and given that vmgenid is supported already by
> > multiple hypervisors, we can implement more or less the same solution.
> > So this commit wires up the vmgenid ACPI notification to the RNG's newly
> > added add_vmfork_randomness() function.
> >
> > It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> > After setting that, use `savevm` in the monitor to save the VM state,
> > then quit QEMU, start it again, and use `loadvm`. That will trigger this
> > driver's notify function, which hands the new UUID to the RNG. This is
> > described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> > And there are hooks for this in libvirt as well, described in
> > <https://libvirt.org/formatdomain.html#general-metadata>.
> >
> > Note, however, that the treatment of this as a UUID is considered to be
> > an accidental QEMU nuance, per
> > <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> > so this driver simply treats these bytes as an opaque 128-bit binary
> > blob, as per the spec. This doesn't really make a difference anyway,
> > considering that's how it ends up when handed to the RNG in the end.
> >
> > This driver builds on prior work from Adrian Catangiu at Amazon, and it
> > is my hope that that team can resume maintenance of this driver.
> >
> > Cc: Adrian Catangiu <adrian@parity.io>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  drivers/virt/Kconfig   |   9 +++
> >  drivers/virt/Makefile  |   1 +
> >  drivers/virt/vmgenid.c | 121 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 131 insertions(+)
> >  create mode 100644 drivers/virt/vmgenid.c
> >
> > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> > index 8061e8ef449f..d3276dc2095c 100644
> > --- a/drivers/virt/Kconfig
> > +++ b/drivers/virt/Kconfig
> 
> drivers/virt does not have a maintainer and this code needs one.
> 
> > @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
> >
> >  if VIRT_DRIVERS
> >
> > +config VMGENID
> > +       tristate "Virtual Machine Generation ID driver"
> > +       default y
> 
> Please make this default m - this code can run as a module and the
> feature it relies on is discoverable by udev

Or don't supply a default - I don't see why this has any preference.

> > +       depends on ACPI
> > +       help
> > +         Say Y here to use the hypervisor-provided Virtual Machine Generation ID
> > +         to reseed the RNG when the VM is cloned. This is highly recommended if
> > +         you intend to do any rollback / cloning / snapshotting of VMs.
> > +
> >  config FSL_HV_MANAGER
> >         tristate "Freescale hypervisor management driver"
> >         depends on FSL_SOC
> > diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> > index 3e272ea60cd9..108d0ffcc9aa 100644
> > --- a/drivers/virt/Makefile
> > +++ b/drivers/virt/Makefile
> > @@ -4,6 +4,7 @@
> >  #
> >
> >  obj-$(CONFIG_FSL_HV_MANAGER)   += fsl_hypervisor.o
> > +obj-$(CONFIG_VMGENID)          += vmgenid.o
> >  obj-y                          += vboxguest/
> >
> >  obj-$(CONFIG_NITRO_ENCLAVES)   += nitro_enclaves/
> > diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> > new file mode 100644
> > index 000000000000..5da4dc8f25e3
> > --- /dev/null
> > +++ b/drivers/virt/vmgenid.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Virtual Machine Generation ID driver
> > + *
> > + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> > + * Copyright (C) 2020 Amazon. All rights reserved.
> > + * Copyright (C) 2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <linux/random.h>
> > +
> > +ACPI_MODULE_NAME("vmgenid");
> > +
> > +enum { VMGENID_SIZE = 16 };
> > +
> > +static struct {
> > +       u8 this_id[VMGENID_SIZE];
> > +       u8 *next_id;
> > +} state;
> > +
> 
> This state is singular
> 
> 
> > +static int vmgenid_acpi_add(struct acpi_device *device)
> > +{
> 
> ... whereas this may be called for multiple instances of the device.
> This likely makes no sense, so it is better to reject it here.
> 
> Otherwise, the state should be allocated dynamically.
> 
> > +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
> > +       union acpi_object *pss;
> > +       phys_addr_t phys_addr;
> > +       acpi_status status;
> > +       int ret = 0;
> > +
> > +       if (!device)
> > +               return -EINVAL;
> > +
> > +       status = acpi_evaluate_object(device->handle, "ADDR", NULL, &buffer);
> > +       if (ACPI_FAILURE(status)) {
> > +               ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> > +               return -ENODEV;
> > +       }
> > +       pss = buffer.pointer;
> > +       if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2 ||
> > +           pss->package.elements[0].type != ACPI_TYPE_INTEGER ||
> > +           pss->package.elements[1].type != ACPI_TYPE_INTEGER) {
> > +               ret = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       phys_addr = (pss->package.elements[0].integer.value << 0) |
> > +                   (pss->package.elements[1].integer.value << 32);
> > +       state.next_id = acpi_os_map_memory(phys_addr, VMGENID_SIZE);
> 
> No need to use acpi_os_map_memory() here, plain memremap() should be fine.
> 
> > +       if (!state.next_id) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +       device->driver_data = &state;
> > +
> > +       memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> > +       add_device_randomness(state.this_id, sizeof(state.this_id));
> > +
> > +out:
> > +       ACPI_FREE(buffer.pointer);
> > +       return ret;
> > +}
> > +
> > +static int vmgenid_acpi_remove(struct acpi_device *device)
> > +{
> > +       if (!device || acpi_driver_data(device) != &state)
> > +               return -EINVAL;
> > +       device->driver_data = NULL;
> > +       if (state.next_id)
> > +               acpi_os_unmap_memory(state.next_id, VMGENID_SIZE);
> 
> memunmap() here
> 
> > +       state.next_id = NULL;
> > +       return 0;
> > +}
> > +
> > +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> > +{
> > +       u8 old_id[VMGENID_SIZE];
> > +
> > +       if (!device || acpi_driver_data(device) != &state)
> > +               return;
> > +       memcpy(old_id, state.this_id, sizeof(old_id));
> > +       memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> > +       if (!memcmp(old_id, state.this_id, sizeof(old_id)))
> > +               return;
> 
> Is this little dance really necessary? I.e., can we just do
> 
> add_vmfork_randomness(state.next_id, VMGENID_SIZE)
> 
> and be done with it?
> 
> And if we cannot, is it ok to just return without some kind of
> diagnostic message?
> 
> > +       add_vmfork_randomness(state.this_id, sizeof(state.this_id));
> > +}
> > +
> > +static const struct acpi_device_id vmgenid_ids[] = {
> > +       {"VMGENID", 0},
> > +       {"QEMUVGID", 0},
> > +       { },
> > +};
> > +
> > +static struct acpi_driver acpi_driver = {
> > +       .name = "vm_generation_id",
> > +       .ids = vmgenid_ids,
> > +       .owner = THIS_MODULE,
> > +       .ops = {
> > +               .add = vmgenid_acpi_add,
> > +               .remove = vmgenid_acpi_remove,
> > +               .notify = vmgenid_acpi_notify,
> > +       }
> > +};
> > +
> > +static int __init vmgenid_init(void)
> > +{
> > +       return acpi_bus_register_driver(&acpi_driver);
> > +}
> > +
> > +static void __exit vmgenid_exit(void)
> > +{
> > +       acpi_bus_unregister_driver(&acpi_driver);
> > +}
> > +
> > +module_init(vmgenid_init);
> > +module_exit(vmgenid_exit);
> > +
> > +MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> > +MODULE_DESCRIPTION("Virtual Machine Generation ID");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.35.1
> >

Re: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 12:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >  if VIRT_DRIVERS
> > >
> > > +config VMGENID
> > > +       tristate "Virtual Machine Generation ID driver"
> > > +       default y
> >
> > Please make this default m - this code can run as a module and the
> > feature it relies on is discoverable by udev
>
> Or don't supply a default - I don't see why this has any preference.

It's inside of VIRT_DRIVERS. If you enabled VIRT_DRIVERS, you more
than likely want and need this.

Re: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 12:24 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
> >
> >  if VIRT_DRIVERS
> >
> > +config VMGENID
> > +       tristate "Virtual Machine Generation ID driver"
> > +       default y
>
> Please make this default m - this code can run as a module and the
> feature it relies on is discoverable by udev

Will do.

> > index 000000000000..5da4dc8f25e3
> > --- /dev/null
> > +++ b/drivers/virt/vmgenid.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Virtual Machine Generation ID driver
> > + *
> > + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> > + * Copyright (C) 2020 Amazon. All rights reserved.
> > + * Copyright (C) 2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <linux/random.h>
> > +
> > +ACPI_MODULE_NAME("vmgenid");
> > +
> > +enum { VMGENID_SIZE = 16 };
> > +
> > +static struct {
> > +       u8 this_id[VMGENID_SIZE];
> > +       u8 *next_id;
> > +} state;
> > +
>
> This state is singular
>
>
> > +static int vmgenid_acpi_add(struct acpi_device *device)
> > +{
>
> ... whereas this may be called for multiple instances of the device.
> This likely makes no sense, so it is better to reject it here.

Like, return early if it's called a second time? I can do that.

>
> > +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER };
> > +       union acpi_object *pss;
> > +       phys_addr_t phys_addr;
> > +       acpi_status status;
> > +       int ret = 0;
> > +
> > +       if (!device)
> > +               return -EINVAL;
> > +
> > +       status = acpi_evaluate_object(device->handle, "ADDR", NULL, &buffer);
> > +       if (ACPI_FAILURE(status)) {
> > +               ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> > +               return -ENODEV;
> > +       }
> > +       pss = buffer.pointer;
> > +       if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2 ||
> > +           pss->package.elements[0].type != ACPI_TYPE_INTEGER ||
> > +           pss->package.elements[1].type != ACPI_TYPE_INTEGER) {
> > +               ret = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       phys_addr = (pss->package.elements[0].integer.value << 0) |
> > +                   (pss->package.elements[1].integer.value << 32);
> > +       state.next_id = acpi_os_map_memory(phys_addr, VMGENID_SIZE);
>
> No need to use acpi_os_map_memory() here, plain memremap() should be fine.

As we discussed online, I'll use dev_memremap(), and then get rid of
the `.remove = ` function all together.

> > +               acpi_os_unmap_memory(state.next_id, VMGENID_SIZE);
>
> memunmap() here

And then as discussed, this goes away too.

>
> > +       state.next_id = NULL;
> > +       return 0;
> > +}
> > +
> > +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> > +{
> > +       u8 old_id[VMGENID_SIZE];
> > +
> > +       if (!device || acpi_driver_data(device) != &state)
> > +               return;
> > +       memcpy(old_id, state.this_id, sizeof(old_id));
> > +       memcpy(state.this_id, state.next_id, sizeof(state.this_id));
> > +       if (!memcmp(old_id, state.this_id, sizeof(old_id)))
> > +               return;
>
> Is this little dance really necessary? I.e., can we just do
>
> add_vmfork_randomness(state.next_id, VMGENID_SIZE)
>
> and be done with it?

Yes it is. vmfork induces a "premature-next" which we really should
not do unless it's really necessary. It torches the whole entropy
pool. In the even that this notifier fires but the ID hasn't changed,
we shouldn't touch anything.

Thanks for the review. v+1 coming up shortly.

[PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

VM Generation ID is a feature from Microsoft, described at
<https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
<https://aka.ms/win10rng>, as:

    If the OS is running in a VM, there is a problem that most
    hypervisors can snapshot the state of the machine and later rewind
    the VM state to the saved state. This results in the machine running
    a second time with the exact same RNG state, which leads to serious
    security problems.  To reduce the window of vulnerability, Windows
    10 on a Hyper-V VM will detect when the VM state is reset, retrieve
    a unique (not random) value from the hypervisor, and reseed the root
    RNG with that unique value.  This does not eliminate the
    vulnerability, but it greatly reduces the time during which the RNG
    system will produce the same outputs as it did during a previous
    instantiation of the same VM state.

Linux has the same issue, and given that vmgenid is supported already by
multiple hypervisors, we can implement more or less the same solution.
So this commit wires up the vmgenid ACPI notification to the RNG's newly
added add_vmfork_randomness() function.

It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
After setting that, use `savevm` in the monitor to save the VM state,
then quit QEMU, start it again, and use `loadvm`. That will trigger this
driver's notify function, which hands the new UUID to the RNG. This is
described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
And there are hooks for this in libvirt as well, described in
<https://libvirt.org/formatdomain.html#general-metadata>.

Note, however, that the treatment of this as a UUID is considered to be
an accidental QEMU nuance, per
<https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
so this driver simply treats these bytes as an opaque 128-bit binary
blob, as per the spec. This doesn't really make a difference anyway,
considering that's how it ends up when handed to the RNG in the end.

Cc: Adrian Catangiu <adrian@parity.io>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v3->v4:
- Add this driver to MAINTAINERS, per Ard's request.
  Note: I didn't really want to do this at first, because I was hoping the
  original Amazon team looking into this last year would step up. But it seems
  like that team has moved on, and anyway I've basically rewritten the driver
  from scratch at this point -- not a single line of the original exists --
  and so I guess I'll maintain it myself. Adding Greg to the CC for his ack on
  this.
- Don't use a static global state in case there are multiple instances.
- Use devm_memremap instead of the acpi internal functions.
- Default to being modular instead of a built-in, as apparently this is
  udev-able.

 MAINTAINERS            |   1 +
 drivers/virt/Kconfig   |   9 ++++
 drivers/virt/Makefile  |   1 +
 drivers/virt/vmgenid.c | 112 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+)
 create mode 100644 drivers/virt/vmgenid.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 777cd6fa2b3d..a10997e15146 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16211,6 +16211,7 @@ M:	Jason A. Donenfeld <Jason@zx2c4.com>
 T:	git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
 S:	Maintained
 F:	drivers/char/random.c
+F:	drivers/virt/vmgenid.c
 
 RAPIDIO SUBSYSTEM
 M:	Matt Porter <mporter@kernel.crashing.org>
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 8061e8ef449f..5596c7313f59 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
 
 if VIRT_DRIVERS
 
+config VMGENID
+	tristate "Virtual Machine Generation ID driver"
+	default m
+	depends on ACPI
+	help
+	  Say Y here to use the hypervisor-provided Virtual Machine Generation ID
+	  to reseed the RNG when the VM is cloned. This is highly recommended if
+	  you intend to do any rollback / cloning / snapshotting of VMs.
+
 config FSL_HV_MANAGER
 	tristate "Freescale hypervisor management driver"
 	depends on FSL_SOC
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 3e272ea60cd9..108d0ffcc9aa 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -4,6 +4,7 @@
 #
 
 obj-$(CONFIG_FSL_HV_MANAGER)	+= fsl_hypervisor.o
+obj-$(CONFIG_VMGENID)		+= vmgenid.o
 obj-y				+= vboxguest/
 
 obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
new file mode 100644
index 000000000000..e3dd4afb33c6
--- /dev/null
+++ b/drivers/virt/vmgenid.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ *
+ * The "Virtual Machine Generation ID" is exposed via ACPI and changes when a
+ * virtual machine forks or is cloned. This driver exists for shepherding that
+ * information to random.c.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/random.h>
+
+ACPI_MODULE_NAME("vmgenid");
+
+enum { VMGENID_SIZE = 16 };
+
+struct vmgenid_state {
+	u8 *next_id;
+	u8 this_id[VMGENID_SIZE];
+};
+
+static int vmgenid_acpi_add(struct acpi_device *device)
+{
+	struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
+	struct vmgenid_state *state;
+	union acpi_object *obj;
+	phys_addr_t phys_addr;
+	acpi_status status;
+	int ret = 0;
+
+	state = devm_kmalloc(&device->dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
+		return -ENODEV;
+	}
+	obj = parsed.pointer;
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2 ||
+	    obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
+	    obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	phys_addr = (obj->package.elements[0].integer.value << 0) |
+		    (obj->package.elements[1].integer.value << 32);
+	state->next_id = devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB);
+	if (!state->next_id) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(state->this_id, state->next_id, sizeof(state->this_id));
+	add_device_randomness(state->this_id, sizeof(state->this_id));
+
+	device->driver_data = state;
+
+out:
+	ACPI_FREE(parsed.pointer);
+	return ret;
+}
+
+static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
+{
+	struct vmgenid_state *state = acpi_driver_data(device);
+	u8 old_id[VMGENID_SIZE];
+
+	memcpy(old_id, state->this_id, sizeof(old_id));
+	memcpy(state->this_id, state->next_id, sizeof(state->this_id));
+	if (!memcmp(old_id, state->this_id, sizeof(old_id)))
+		return;
+	add_vmfork_randomness(state->this_id, sizeof(state->this_id));
+}
+
+static const struct acpi_device_id vmgenid_ids[] = {
+	{ "VMGENID", 0 },
+	{ "QEMUVGID", 0 },
+	{ },
+};
+
+static struct acpi_driver acpi_driver = {
+	.name = "vmgenid",
+	.ids = vmgenid_ids,
+	.owner = THIS_MODULE,
+	.ops = {
+		.add = vmgenid_acpi_add,
+		.notify = vmgenid_acpi_notify,
+	}
+};
+
+static int __init vmgenid_init(void)
+{
+	return acpi_bus_register_driver(&acpi_driver);
+}
+
+static void __exit vmgenid_exit(void)
+{
+	acpi_bus_unregister_driver(&acpi_driver);
+}
+
+module_init(vmgenid_init);
+module_exit(vmgenid_exit);
+
+MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
+MODULE_DESCRIPTION("Virtual Machine Generation ID");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Jason A. Donenfeld <Jason@zx2c4.com>");
-- 
2.35.1

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Greg KH]

On Fri, Feb 25, 2022 at 01:48:48PM +0100, Jason A. Donenfeld wrote:
> VM Generation ID is a feature from Microsoft, described at
> <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> <https://aka.ms/win10rng>, as:
> 
>     If the OS is running in a VM, there is a problem that most
>     hypervisors can snapshot the state of the machine and later rewind
>     the VM state to the saved state. This results in the machine running
>     a second time with the exact same RNG state, which leads to serious
>     security problems.  To reduce the window of vulnerability, Windows
>     10 on a Hyper-V VM will detect when the VM state is reset, retrieve
>     a unique (not random) value from the hypervisor, and reseed the root
>     RNG with that unique value.  This does not eliminate the
>     vulnerability, but it greatly reduces the time during which the RNG
>     system will produce the same outputs as it did during a previous
>     instantiation of the same VM state.
> 
> Linux has the same issue, and given that vmgenid is supported already by
> multiple hypervisors, we can implement more or less the same solution.
> So this commit wires up the vmgenid ACPI notification to the RNG's newly
> added add_vmfork_randomness() function.
> 
> It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> After setting that, use `savevm` in the monitor to save the VM state,
> then quit QEMU, start it again, and use `loadvm`. That will trigger this
> driver's notify function, which hands the new UUID to the RNG. This is
> described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> And there are hooks for this in libvirt as well, described in
> <https://libvirt.org/formatdomain.html#general-metadata>.
> 
> Note, however, that the treatment of this as a UUID is considered to be
> an accidental QEMU nuance, per
> <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> so this driver simply treats these bytes as an opaque 128-bit binary
> blob, as per the spec. This doesn't really make a difference anyway,
> considering that's how it ends up when handed to the RNG in the end.
> 
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Greg KH]

On Fri, Feb 25, 2022 at 01:48:48PM +0100, Jason A. Donenfeld wrote:
> +static struct acpi_driver acpi_driver = {
> +	.name = "vmgenid",
> +	.ids = vmgenid_ids,
> +	.owner = THIS_MODULE,
> +	.ops = {
> +		.add = vmgenid_acpi_add,
> +		.notify = vmgenid_acpi_notify,
> +	}
> +};
> +
> +static int __init vmgenid_init(void)
> +{
> +	return acpi_bus_register_driver(&acpi_driver);
> +}
> +
> +static void __exit vmgenid_exit(void)
> +{
> +	acpi_bus_unregister_driver(&acpi_driver);
> +}
> +
> +module_init(vmgenid_init);
> +module_exit(vmgenid_exit);

Nit, you could use module_acpi_driver() to make this even smaller if you
want to.

thanks,

greg k-h

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 1:53 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 25, 2022 at 01:48:48PM +0100, Jason A. Donenfeld wrote:
> > +static struct acpi_driver acpi_driver = {
> > +     .name = "vmgenid",
> > +     .ids = vmgenid_ids,
> > +     .owner = THIS_MODULE,
> > +     .ops = {
> > +             .add = vmgenid_acpi_add,
> > +             .notify = vmgenid_acpi_notify,
> > +     }
> > +};
> > +
> > +static int __init vmgenid_init(void)
> > +{
> > +     return acpi_bus_register_driver(&acpi_driver);
> > +}
> > +
> > +static void __exit vmgenid_exit(void)
> > +{
> > +     acpi_bus_unregister_driver(&acpi_driver);
> > +}
> > +
> > +module_init(vmgenid_init);
> > +module_exit(vmgenid_exit);
>
> Nit, you could use module_acpi_driver() to make this even smaller if you
> want to.

Nice! Will do.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Ard Biesheuvel]

On Fri, 25 Feb 2022 at 13:53, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 25, 2022 at 01:48:48PM +0100, Jason A. Donenfeld wrote:
> > +static struct acpi_driver acpi_driver = {
> > +     .name = "vmgenid",
> > +     .ids = vmgenid_ids,
> > +     .owner = THIS_MODULE,
> > +     .ops = {
> > +             .add = vmgenid_acpi_add,
> > +             .notify = vmgenid_acpi_notify,
> > +     }
> > +};
> > +
> > +static int __init vmgenid_init(void)
> > +{
> > +     return acpi_bus_register_driver(&acpi_driver);
> > +}
> > +
> > +static void __exit vmgenid_exit(void)
> > +{
> > +     acpi_bus_unregister_driver(&acpi_driver);
> > +}
> > +
> > +module_init(vmgenid_init);
> > +module_exit(vmgenid_exit);
>
> Nit, you could use module_acpi_driver() to make this even smaller if you
> want to.
>

With that suggestion adopted,

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 13:48, Jason A. Donenfeld wrote:
>
> VM Generation ID is a feature from Microsoft, described at
> <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> <https://aka.ms/win10rng>, as:
>
>      If the OS is running in a VM, there is a problem that most
>      hypervisors can snapshot the state of the machine and later rewind
>      the VM state to the saved state. This results in the machine running
>      a second time with the exact same RNG state, which leads to serious
>      security problems.  To reduce the window of vulnerability, Windows
>      10 on a Hyper-V VM will detect when the VM state is reset, retrieve
>      a unique (not random) value from the hypervisor, and reseed the root
>      RNG with that unique value.  This does not eliminate the
>      vulnerability, but it greatly reduces the time during which the RNG
>      system will produce the same outputs as it did during a previous
>      instantiation of the same VM state.
>
> Linux has the same issue, and given that vmgenid is supported already by
> multiple hypervisors, we can implement more or less the same solution.
> So this commit wires up the vmgenid ACPI notification to the RNG's newly
> added add_vmfork_randomness() function.
>
> It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> After setting that, use `savevm` in the monitor to save the VM state,
> then quit QEMU, start it again, and use `loadvm`. That will trigger this
> driver's notify function, which hands the new UUID to the RNG. This is
> described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> And there are hooks for this in libvirt as well, described in
> <https://libvirt.org/formatdomain.html#general-metadata>.
>
> Note, however, that the treatment of this as a UUID is considered to be
> an accidental QEMU nuance, per
> <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> so this driver simply treats these bytes as an opaque 128-bit binary
> blob, as per the spec. This doesn't really make a difference anyway,
> considering that's how it ends up when handed to the RNG in the end.
>
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v3->v4:
> - Add this driver to MAINTAINERS, per Ard's request.
>    Note: I didn't really want to do this at first, because I was hoping the
>    original Amazon team looking into this last year would step up. But it seems
>    like that team has moved on, and anyway I've basically rewritten the driver
>    from scratch at this point -- not a single line of the original exists --
>    and so I guess I'll maintain it myself. Adding Greg to the CC for his ack on
>    this.
> - Don't use a static global state in case there are multiple instances.
> - Use devm_memremap instead of the acpi internal functions.
> - Default to being modular instead of a built-in, as apparently this is
>    udev-able.
>
>   MAINTAINERS            |   1 +
>   drivers/virt/Kconfig   |   9 ++++
>   drivers/virt/Makefile  |   1 +
>   drivers/virt/vmgenid.c | 112 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 123 insertions(+)
>   create mode 100644 drivers/virt/vmgenid.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 777cd6fa2b3d..a10997e15146 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16211,6 +16211,7 @@ M:      Jason A. Donenfeld <Jason@zx2c4.com>
>   T:     git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
>   S:     Maintained
>   F:     drivers/char/random.c
> +F:     drivers/virt/vmgenid.c
>
>   RAPIDIO SUBSYSTEM
>   M:     Matt Porter <mporter@kernel.crashing.org>
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 8061e8ef449f..5596c7313f59 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
>
>   if VIRT_DRIVERS
>
> +config VMGENID
> +       tristate "Virtual Machine Generation ID driver"
> +       default m
> +       depends on ACPI
> +       help
> +         Say Y here to use the hypervisor-provided Virtual Machine Generation ID
> +         to reseed the RNG when the VM is cloned. This is highly recommended if
> +         you intend to do any rollback / cloning / snapshotting of VMs.
> +
>   config FSL_HV_MANAGER
>          tristate "Freescale hypervisor management driver"
>          depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index 3e272ea60cd9..108d0ffcc9aa 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -4,6 +4,7 @@
>   #
>
>   obj-$(CONFIG_FSL_HV_MANAGER)   += fsl_hypervisor.o
> +obj-$(CONFIG_VMGENID)          += vmgenid.o
>   obj-y                          += vboxguest/
>
>   obj-$(CONFIG_NITRO_ENCLAVES)   += nitro_enclaves/
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> new file mode 100644
> index 000000000000..e3dd4afb33c6
> --- /dev/null
> +++ b/drivers/virt/vmgenid.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + *
> + * The "Virtual Machine Generation ID" is exposed via ACPI and changes when a
> + * virtual machine forks or is cloned. This driver exists for shepherding that
> + * information to random.c.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/random.h>
> +
> +ACPI_MODULE_NAME("vmgenid");
> +
> +enum { VMGENID_SIZE = 16 };
> +
> +struct vmgenid_state {
> +       u8 *next_id;
> +       u8 this_id[VMGENID_SIZE];
> +};
> +
> +static int vmgenid_acpi_add(struct acpi_device *device)
> +{
> +       struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
> +       struct vmgenid_state *state;
> +       union acpi_object *obj;
> +       phys_addr_t phys_addr;
> +       acpi_status status;
> +       int ret = 0;
> +
> +       state = devm_kmalloc(&device->dev, sizeof(*state), GFP_KERNEL);
> +       if (!state)
> +               return -ENOMEM;
> +
> +       status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
> +       if (ACPI_FAILURE(status)) {
> +               ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> +               return -ENODEV;
> +       }
> +       obj = parsed.pointer;
> +       if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2 ||
> +           obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
> +           obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       phys_addr = (obj->package.elements[0].integer.value << 0) |
> +                   (obj->package.elements[1].integer.value << 32);
> +       state->next_id = devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB);
> +       if (!state->next_id) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
> +       add_device_randomness(state->this_id, sizeof(state->this_id));


Please expose the vmgenid via /sysfs so that user space even remotely 
has a chance to check if it's been cloned.


> +
> +       device->driver_data = state;
> +
> +out:
> +       ACPI_FREE(parsed.pointer);
> +       return ret;
> +}
> +
> +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> +{
> +       struct vmgenid_state *state = acpi_driver_data(device);
> +       u8 old_id[VMGENID_SIZE];
> +
> +       memcpy(old_id, state->this_id, sizeof(old_id));
> +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
> +       if (!memcmp(old_id, state->this_id, sizeof(old_id)))
> +               return;
> +       add_vmfork_randomness(state->this_id, sizeof(state->this_id));
> +}
> +
> +static const struct acpi_device_id vmgenid_ids[] = {
> +       { "VMGENID", 0 },
> +       { "QEMUVGID", 0 },


According to the VMGenID spec[1], you can only rely on _CID and _DDN for 
matching. They both contain "VM_Gen_Counter". The list above contains 
_HID values which are not an official identifier for the VMGenID device.

IIRC the ACPI device match logic does match _CID in addition to _HID. 
However, it is limited to 8 characters. Let me paste an experimental 
hack I did back then to do the _CID matching instead.

[1] 
https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx


Alex

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 1682f8b454a2..452443d79d87 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device 
*device,
          /* First, check the ACPI/PNP IDs provided by the caller. */
          if (acpi_ids) {
              for (id = acpi_ids; id->id[0] || id->cls; id++) {
-                if (id->id[0] && !strcmp((char *)id->id, hwid->id))
+                if (id->id[0] && !strncmp((char *)id->id, hwid->id, 
ACPI_ID_LEN - 1))
                      goto out_acpi_match;
                  if (id->cls && __acpi_match_device_cls(id, hwid))
                      goto out_acpi_match;
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
index 75a787da8aad..0bfa422cf094 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device 
*device, u32 event)
  }

  static const struct acpi_device_id vmgenid_ids[] = {
-    {"QEMUVGID", 0},
+    /* This really is VM_Gen_Counter, but we can only match 8 characters */
+    {"VM_GEN_C", 0},
      {"", 0},
  };




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi Alex,

On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
> > +static const struct acpi_device_id vmgenid_ids[] = {
> > +       { "VMGENID", 0 },
> > +       { "QEMUVGID", 0 },
> 
> 
> According to the VMGenID spec[1], you can only rely on _CID and _DDN for 
> matching. They both contain "VM_Gen_Counter". The list above contains 
> _HID values which are not an official identifier for the VMGenID device.
> 
> IIRC the ACPI device match logic does match _CID in addition to _HID. 
> However, it is limited to 8 characters. Let me paste an experimental 
> hack I did back then to do the _CID matching instead.
> 
> [1] 
> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
> 
> 
> Alex
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 1682f8b454a2..452443d79d87 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device 
> *device,
>           /* First, check the ACPI/PNP IDs provided by the caller. */
>           if (acpi_ids) {
>               for (id = acpi_ids; id->id[0] || id->cls; id++) {
> -                if (id->id[0] && !strcmp((char *)id->id, hwid->id))
> +                if (id->id[0] && !strncmp((char *)id->id, hwid->id, 
> ACPI_ID_LEN - 1))
>                       goto out_acpi_match;
>                   if (id->cls && __acpi_match_device_cls(id, hwid))
>                       goto out_acpi_match;
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> index 75a787da8aad..0bfa422cf094 100644
> --- a/drivers/virt/vmgenid.c
> +++ b/drivers/virt/vmgenid.c
> @@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device 
> *device, u32 event)
>   }
> 
>   static const struct acpi_device_id vmgenid_ids[] = {
> -    {"QEMUVGID", 0},
> +    /* This really is VM_Gen_Counter, but we can only match 8 characters */
> +    {"VM_GEN_C", 0},
>       {"", 0},
>   };

I recall this part of the old thread. From what I understood, using
"VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
technically in-spec. Ard noted that relying on _CID like that is
technically an ACPI spec notification. So we're between one spec and
another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
changes, as mentioned, appears to work fine in my testing.

However, with that said, I think supporting this via "VM_Gen_Counter"
would be a better eventual thing to do, but will require acks and
changes from the ACPI maintainers. Do you think you could prepare your
patch proposal above as something on-top of my tree [1]? And if you can
convince the ACPI maintainers that that's okay, then I'll happily take
the patch.

Jason

[1] https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 3:12 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Alex,
>
> On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
> > > +static const struct acpi_device_id vmgenid_ids[] = {
> > > +       { "VMGENID", 0 },
> > > +       { "QEMUVGID", 0 },
> >
> >
> > According to the VMGenID spec[1], you can only rely on _CID and _DDN for
> > matching. They both contain "VM_Gen_Counter". The list above contains
> > _HID values which are not an official identifier for the VMGenID device.
> >
> > IIRC the ACPI device match logic does match _CID in addition to _HID.
> > However, it is limited to 8 characters. Let me paste an experimental
> > hack I did back then to do the _CID matching instead.
> >
> > [1]
> > https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
> >
> >
> > Alex
> >
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 1682f8b454a2..452443d79d87 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device
> > *device,
> >           /* First, check the ACPI/PNP IDs provided by the caller. */
> >           if (acpi_ids) {
> >               for (id = acpi_ids; id->id[0] || id->cls; id++) {
> > -                if (id->id[0] && !strcmp((char *)id->id, hwid->id))
> > +                if (id->id[0] && !strncmp((char *)id->id, hwid->id,
> > ACPI_ID_LEN - 1))
> >                       goto out_acpi_match;
> >                   if (id->cls && __acpi_match_device_cls(id, hwid))
> >                       goto out_acpi_match;
> > diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> > index 75a787da8aad..0bfa422cf094 100644
> > --- a/drivers/virt/vmgenid.c
> > +++ b/drivers/virt/vmgenid.c
> > @@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device
> > *device, u32 event)
> >   }
> >
> >   static const struct acpi_device_id vmgenid_ids[] = {
> > -    {"QEMUVGID", 0},
> > +    /* This really is VM_Gen_Counter, but we can only match 8 characters */
> > +    {"VM_GEN_C", 0},
> >       {"", 0},
> >   };
>
> I recall this part of the old thread. From what I understood, using
> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
> technically in-spec. Ard noted that relying on _CID like that is
> technically an ACPI spec notification. So we're between one spec and
> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
> changes, as mentioned, appears to work fine in my testing.
>
> However, with that said, I think supporting this via "VM_Gen_Counter"
> would be a better eventual thing to do, but will require acks and
> changes from the ACPI maintainers. Do you think you could prepare your
> patch proposal above as something on-top of my tree [1]? And if you can
> convince the ACPI maintainers that that's okay, then I'll happily take
> the patch.

Closely related concern that whatever patch you come up with will have
to handle is MODULE_DEVICE_TABLE and udev autoloading. I don't know if
_CID matching is something that happens in udev or what its limits
are, so that'll have to be researched and tested a bit.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 15:12, Jason A. Donenfeld wrote:
> Hi Alex,
>
> On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
>>> +static const struct acpi_device_id vmgenid_ids[] = {
>>> +       { "VMGENID", 0 },
>>> +       { "QEMUVGID", 0 },
>>
>> According to the VMGenID spec[1], you can only rely on _CID and _DDN for
>> matching. They both contain "VM_Gen_Counter". The list above contains
>> _HID values which are not an official identifier for the VMGenID device.
>>
>> IIRC the ACPI device match logic does match _CID in addition to _HID.
>> However, it is limited to 8 characters. Let me paste an experimental
>> hack I did back then to do the _CID matching instead.
>>
>> [1]
>> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>>
>>
>> Alex
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 1682f8b454a2..452443d79d87 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device
>> *device,
>>            /* First, check the ACPI/PNP IDs provided by the caller. */
>>            if (acpi_ids) {
>>                for (id = acpi_ids; id->id[0] || id->cls; id++) {
>> -                if (id->id[0] && !strcmp((char *)id->id, hwid->id))
>> +                if (id->id[0] && !strncmp((char *)id->id, hwid->id,
>> ACPI_ID_LEN - 1))
>>                        goto out_acpi_match;
>>                    if (id->cls && __acpi_match_device_cls(id, hwid))
>>                        goto out_acpi_match;
>> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
>> index 75a787da8aad..0bfa422cf094 100644
>> --- a/drivers/virt/vmgenid.c
>> +++ b/drivers/virt/vmgenid.c
>> @@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device
>> *device, u32 event)
>>    }
>>
>>    static const struct acpi_device_id vmgenid_ids[] = {
>> -    {"QEMUVGID", 0},
>> +    /* This really is VM_Gen_Counter, but we can only match 8 characters */
>> +    {"VM_GEN_C", 0},
>>        {"", 0},
>>    };
> I recall this part of the old thread. From what I understood, using
> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
> technically in-spec. Ard noted that relying on _CID like that is
> technically an ACPI spec notification. So we're between one spec and
> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
> changes, as mentioned, appears to work fine in my testing.
>
> However, with that said, I think supporting this via "VM_Gen_Counter"
> would be a better eventual thing to do, but will require acks and
> changes from the ACPI maintainers. Do you think you could prepare your
> patch proposal above as something on-top of my tree [1]? And if you can
> convince the ACPI maintainers that that's okay, then I'll happily take
> the patch.


Sure, let me send the ACPI patch stand alone. No need to include the 
VMGenID change in there.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 03:18:43PM +0100, Alexander Graf wrote:
> > I recall this part of the old thread. From what I understood, using
> > "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
> > technically in-spec. Ard noted that relying on _CID like that is
> > technically an ACPI spec notification. So we're between one spec and
> > another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
> > changes, as mentioned, appears to work fine in my testing.
> >
> > However, with that said, I think supporting this via "VM_Gen_Counter"
> > would be a better eventual thing to do, but will require acks and
> > changes from the ACPI maintainers. Do you think you could prepare your
> > patch proposal above as something on-top of my tree [1]? And if you can
> > convince the ACPI maintainers that that's okay, then I'll happily take
> > the patch.
> 
> 
> Sure, let me send the ACPI patch stand alone. No need to include the 
> VMGenID change in there.

That's fine. If the ACPI people take it for 5.18, then we can count on
it being there and adjust the vmgenid driver accordingly also for 5.18.

I just booted up a Windows VM, and it looks like Hyper-V uses
"Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
HID match on that either.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 15:33, Jason A. Donenfeld wrote:
> On Fri, Feb 25, 2022 at 03:18:43PM +0100, Alexander Graf wrote:
>>> I recall this part of the old thread. From what I understood, using
>>> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
>>> technically in-spec. Ard noted that relying on _CID like that is
>>> technically an ACPI spec notification. So we're between one spec and
>>> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
>>> changes, as mentioned, appears to work fine in my testing.
>>>
>>> However, with that said, I think supporting this via "VM_Gen_Counter"
>>> would be a better eventual thing to do, but will require acks and
>>> changes from the ACPI maintainers. Do you think you could prepare your
>>> patch proposal above as something on-top of my tree [1]? And if you can
>>> convince the ACPI maintainers that that's okay, then I'll happily take
>>> the patch.
>>
>> Sure, let me send the ACPI patch stand alone. No need to include the
>> VMGenID change in there.
> That's fine. If the ACPI people take it for 5.18, then we can count on
> it being there and adjust the vmgenid driver accordingly also for 5.18.
>
> I just booted up a Windows VM, and it looks like Hyper-V uses
> "Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
> HID match on that either.


Yes, due to the same problem. I'd really prefer we sort out the ACPI 
matching before this goes mainline. Matching on _HID is explicitly 
discouraged in the VMGenID spec.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Ard Biesheuvel]

On Fri, 25 Feb 2022 at 16:12, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 25.02.22 15:33, Jason A. Donenfeld wrote:
> > On Fri, Feb 25, 2022 at 03:18:43PM +0100, Alexander Graf wrote:
> >>> I recall this part of the old thread. From what I understood, using
> >>> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
> >>> technically in-spec. Ard noted that relying on _CID like that is
> >>> technically an ACPI spec notification. So we're between one spec and
> >>> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
> >>> changes, as mentioned, appears to work fine in my testing.
> >>>
> >>> However, with that said, I think supporting this via "VM_Gen_Counter"
> >>> would be a better eventual thing to do, but will require acks and
> >>> changes from the ACPI maintainers. Do you think you could prepare your
> >>> patch proposal above as something on-top of my tree [1]? And if you can
> >>> convince the ACPI maintainers that that's okay, then I'll happily take
> >>> the patch.
> >>
> >> Sure, let me send the ACPI patch stand alone. No need to include the
> >> VMGenID change in there.
> > That's fine. If the ACPI people take it for 5.18, then we can count on
> > it being there and adjust the vmgenid driver accordingly also for 5.18.
> >
> > I just booted up a Windows VM, and it looks like Hyper-V uses
> > "Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
> > HID match on that either.
>
>
> Yes, due to the same problem. I'd really prefer we sort out the ACPI
> matching before this goes mainline. Matching on _HID is explicitly
> discouraged in the VMGenID spec.
>

OK, this really sucks. Quoting the ACPI spec:

"""
A _HID object evaluates to either a numeric 32-bit compressed EISA
type ID or a string. If a string, the format must be an alphanumeric
PNP or ACPI ID with no asterisk or other leading characters.
A valid PNP ID must be of the form "AAA####" where A is an uppercase
letter and # is a hex digit.
A valid ACPI ID must be of the form "NNNN####" where N is an uppercase
letter or a digit ('0'-'9') and # is a hex digit. This specification
reserves the string "ACPI" for use only with devices defined herein.
It further reserves all strings representing 4 HEX digits for
exclusive use with PCI-assigned Vendor IDs.
"""

So now we have to implement Microsoft's fork of ACPI to be able to use
this device, even if we expose it from QEMU instead of Hyper-V? I
strongly object to that.

Instead, we can match on _HID exposed by QEMU, and cordially invite
Microsoft to align their spec with the ACPI spec.

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 16:16, Ard Biesheuvel wrote:
> On Fri, 25 Feb 2022 at 16:12, Alexander Graf <graf@amazon.com> wrote:
>>
>> On 25.02.22 15:33, Jason A. Donenfeld wrote:
>>> On Fri, Feb 25, 2022 at 03:18:43PM +0100, Alexander Graf wrote:
>>>>> I recall this part of the old thread. From what I understood, using
>>>>> "VMGENID" + "QEMUVGID" worked /well enough/, even if that wasn't
>>>>> technically in-spec. Ard noted that relying on _CID like that is
>>>>> technically an ACPI spec notification. So we're between one spec and
>>>>> another, basically, and doing "VMGENID" + "QEMUVGID" requires fewer
>>>>> changes, as mentioned, appears to work fine in my testing.
>>>>>
>>>>> However, with that said, I think supporting this via "VM_Gen_Counter"
>>>>> would be a better eventual thing to do, but will require acks and
>>>>> changes from the ACPI maintainers. Do you think you could prepare your
>>>>> patch proposal above as something on-top of my tree [1]? And if you can
>>>>> convince the ACPI maintainers that that's okay, then I'll happily take
>>>>> the patch.
>>>> Sure, let me send the ACPI patch stand alone. No need to include the
>>>> VMGenID change in there.
>>> That's fine. If the ACPI people take it for 5.18, then we can count on
>>> it being there and adjust the vmgenid driver accordingly also for 5.18.
>>>
>>> I just booted up a Windows VM, and it looks like Hyper-V uses
>>> "Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
>>> HID match on that either.
>>
>> Yes, due to the same problem. I'd really prefer we sort out the ACPI
>> matching before this goes mainline. Matching on _HID is explicitly
>> discouraged in the VMGenID spec.
>>
> OK, this really sucks. Quoting the ACPI spec:
>
> """
> A _HID object evaluates to either a numeric 32-bit compressed EISA
> type ID or a string. If a string, the format must be an alphanumeric
> PNP or ACPI ID with no asterisk or other leading characters.
> A valid PNP ID must be of the form "AAA####" where A is an uppercase
> letter and # is a hex digit.
> A valid ACPI ID must be of the form "NNNN####" where N is an uppercase
> letter or a digit ('0'-'9') and # is a hex digit. This specification
> reserves the string "ACPI" for use only with devices defined herein.
> It further reserves all strings representing 4 HEX digits for
> exclusive use with PCI-assigned Vendor IDs.
> """
>
> So now we have to implement Microsoft's fork of ACPI to be able to use
> this device, even if we expose it from QEMU instead of Hyper-V? I
> strongly object to that.
>
> Instead, we can match on _HID exposed by QEMU, and cordially invite
> Microsoft to align their spec with the ACPI spec.


Doing that would be a backwards incompatible change for Hyper-V, no? I 
understand that you're upset about their spec, but that doesn't mean we 
can't find a path forward to make it all compatible.

IMHO just matching on the first 9 bytes of the _CID/_HID string is 
perfectly fine. It follows the spec, but still allows for weird 
identifiers like this one to work.

I don't understand the rush here. This had been sitting on the ML for 1 
year - and now suddenly talking the match through properly and getting 
VMGenID spec compatible matching support into the ACPI core is a 
problem? What did I miss? :)


Alex




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi Alex,

On Fri, Feb 25, 2022 at 04:22:54PM +0100, Alexander Graf wrote:
> I don't understand the rush here. This had been sitting on the ML for 1 
> year - and now suddenly talking the match through properly and getting 
> VMGenID spec compatible matching support into the ACPI core is a 
> problem? What did I miss? :)

I don't think this is a question about speed. Ard doesn't like the spec.
You like the feature more than you dislike the spec. Apparently that
means there's a disagreement.

As I mentioned earlier, I'd encourage you to send a patch to the ACPI
people and let them decide. If that gets in, then I'm fine with
modifying vmgenid to meet the spec and take advantage of the change
you'll be making to the ACPI code. If it is rejected by the ACPI people,
and consequently Linux isn't able to match on _CID, then I guess we'll
have the next best thing, which is "well, it still works on QEMU."
Hopefully you'll convince them. Feel free to CC me on that patch.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 16:43, Jason A. Donenfeld wrote:
>
> Hi Alex,
>
> On Fri, Feb 25, 2022 at 04:22:54PM +0100, Alexander Graf wrote:
>> I don't understand the rush here. This had been sitting on the ML for 1
>> year - and now suddenly talking the match through properly and getting
>> VMGenID spec compatible matching support into the ACPI core is a
>> problem? What did I miss? :)
> I don't think this is a question about speed. Ard doesn't like the spec.
> You like the feature more than you dislike the spec. Apparently that
> means there's a disagreement.
>
> As I mentioned earlier, I'd encourage you to send a patch to the ACPI
> people and let them decide. If that gets in, then I'm fine with
> modifying vmgenid to meet the spec and take advantage of the change
> you'll be making to the ACPI code. If it is rejected by the ACPI people,
> and consequently Linux isn't able to match on _CID, then I guess we'll
> have the next best thing, which is "well, it still works on QEMU."
> Hopefully you'll convince them. Feel free to CC me on that patch.


I agree with that approach and CC'ed you on the ACPI patch :). Let's 
explore all options to match against _CID before we give up.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

On Fri, Feb 25, 2022 at 04:16:27PM +0100, Ard Biesheuvel wrote:
> > > I just booted up a Windows VM, and it looks like Hyper-V uses
> > > "Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
> > > HID match on that either.
> >
> >
> > Yes, due to the same problem. I'd really prefer we sort out the ACPI
> > matching before this goes mainline. Matching on _HID is explicitly
> > discouraged in the VMGenID spec.
> >
> 
> OK, this really sucks. Quoting the ACPI spec:
> 
> """
> A _HID object evaluates to either a numeric 32-bit compressed EISA
> type ID or a string. If a string, the format must be an alphanumeric
> PNP or ACPI ID with no asterisk or other leading characters.
> A valid PNP ID must be of the form "AAA####" where A is an uppercase
> letter and # is a hex digit.
> A valid ACPI ID must be of the form "NNNN####" where N is an uppercase
> letter or a digit ('0'-'9') and # is a hex digit. This specification
> reserves the string "ACPI" for use only with devices defined herein.
> It further reserves all strings representing 4 HEX digits for
> exclusive use with PCI-assigned Vendor IDs.
> """
> 
> So now we have to implement Microsoft's fork of ACPI to be able to use
> this device, even if we expose it from QEMU instead of Hyper-V? I
> strongly object to that.
> 
> Instead, we can match on _HID exposed by QEMU, and cordially invite
> Microsoft to align their spec with the ACPI spec.

I don't know about that... Seems a bit extreme. Hopefully Alex will be
able to sort something out with the ACPI people, and this driver will
work inside of Hyper-V.

Here's what we currently have:

  static const struct acpi_device_id vmgenid_ids[] = {
    { "VMGENID", 0 },  <------------------------------------ ???
    { "QEMUVGID", 0 }, <------------------------------------ QEMU
    { },
  };

Adrian added "VMGENID" in last year's v4, so I copied that for this new
driver here. But does anybody know which hypervisor it is for? Some
internal Amazon thing? Firecracker? VMware? In case Alex does not
succeed with the ACPI changes, it'd be nice to know which HIDs for
which hypervisors we do and do not support.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 16:34, Jason A. Donenfeld wrote:
> On Fri, Feb 25, 2022 at 04:16:27PM +0100, Ard Biesheuvel wrote:
>>>> I just booted up a Windows VM, and it looks like Hyper-V uses
>>>> "Hyper_V_Gen_Counter_V1", which is also quite long, so we can't really
>>>> HID match on that either.
>>>
>>> Yes, due to the same problem. I'd really prefer we sort out the ACPI
>>> matching before this goes mainline. Matching on _HID is explicitly
>>> discouraged in the VMGenID spec.
>>>
>> OK, this really sucks. Quoting the ACPI spec:
>>
>> """
>> A _HID object evaluates to either a numeric 32-bit compressed EISA
>> type ID or a string. If a string, the format must be an alphanumeric
>> PNP or ACPI ID with no asterisk or other leading characters.
>> A valid PNP ID must be of the form "AAA####" where A is an uppercase
>> letter and # is a hex digit.
>> A valid ACPI ID must be of the form "NNNN####" where N is an uppercase
>> letter or a digit ('0'-'9') and # is a hex digit. This specification
>> reserves the string "ACPI" for use only with devices defined herein.
>> It further reserves all strings representing 4 HEX digits for
>> exclusive use with PCI-assigned Vendor IDs.
>> """
>>
>> So now we have to implement Microsoft's fork of ACPI to be able to use
>> this device, even if we expose it from QEMU instead of Hyper-V? I
>> strongly object to that.
>>
>> Instead, we can match on _HID exposed by QEMU, and cordially invite
>> Microsoft to align their spec with the ACPI spec.
> I don't know about that... Seems a bit extreme. Hopefully Alex will be
> able to sort something out with the ACPI people, and this driver will
> work inside of Hyper-V.
>
> Here's what we currently have:
>
>    static const struct acpi_device_id vmgenid_ids[] = {
>      { "VMGENID", 0 },  <------------------------------------ ???
>      { "QEMUVGID", 0 }, <------------------------------------ QEMU
>      { },
>    };
>
> Adrian added "VMGENID" in last year's v4, so I copied that for this new
> driver here. But does anybody know which hypervisor it is for? Some
> internal Amazon thing? Firecracker? VMware? In case Alex does not
> succeed with the ACPI changes, it'd be nice to know which HIDs for
> which hypervisors we do and do not support.


I believe "VMGENID" was for the firecracker prototype that Adrian built 
back then, yeah. Matching on _HID for this is a rat hole unfortunately, 
so let's see what the ACPI patch gets us :).


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi Alex,

On Fri, Feb 25, 2022 at 04:37:43PM +0100, Alexander Graf wrote:
> I believe "VMGENID" was for the firecracker prototype that Adrian built 
> back then, yeah. Matching on _HID for this is a rat hole unfortunately, 
> so let's see what the ACPI patch gets us :).

Thanks. I'll add a comment to the code about Firecracker. And indeed
hopefully that'll all go away anyway.

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Greg KH]

On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
> > +
> > +       phys_addr = (obj->package.elements[0].integer.value << 0) |
> > +                   (obj->package.elements[1].integer.value << 32);
> > +       state->next_id = devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB);
> > +       if (!state->next_id) {
> > +               ret = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
> > +       add_device_randomness(state->this_id, sizeof(state->this_id));
> 
> 
> Please expose the vmgenid via /sysfs so that user space even remotely has a
> chance to check if it's been cloned.

Export it how?  And why, who would care?

thanks,

greg k-h

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 15:36, Greg KH wrote:
> On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
>>> +
>>> +       phys_addr = (obj->package.elements[0].integer.value << 0) |
>>> +                   (obj->package.elements[1].integer.value << 32);
>>> +       state->next_id = devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB);
>>> +       if (!state->next_id) {
>>> +               ret = -ENOMEM;
>>> +               goto out;
>>> +       }
>>> +
>>> +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
>>> +       add_device_randomness(state->this_id, sizeof(state->this_id));
>>
>> Please expose the vmgenid via /sysfs so that user space even remotely has a
>> chance to check if it's been cloned.
> Export it how?  And why, who would care?


You can just create a sysfs file that contains it. The same way we have 
sysfs files for UEFI config tables. Or sysfs files for the acpi device 
nodes themselves.

I personally don't care if we put this into a generic location 
(/sys/hypervisor for example) or into the existing acpi device node as 
additional file you can just read.

Who would care? Well, for starters I would care for debugging purposes 
:). Extracting the ID and validating that it's different than before is 
quite useful when you want to check if the clone rng adjustment actually 
worked.

I don't have very strong feelings on it though - unlike the _CID 
conversation.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi again,

On Fri, Feb 25, 2022 at 04:31:02PM +0100, Alexander Graf wrote:
> >> Please expose the vmgenid via /sysfs so that user space even remotely has a
> >> chance to check if it's been cloned.
> > Export it how?  And why, who would care?
> You can just

As mentioned in <https://lore.kernel.org/lkml/Yhj1nYHXmimPsqFd@zx2c4.com/>,
propose something later for all of this. It doesn't need to happen all
at once *now*.

Thanks,
Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi Alex,

Missed this remark before:

On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
> Please expose the vmgenid via /sysfs so that user space even remotely 
> has a chance to check if it's been cloned.

No. Did you read the 0/2 cover letter? I'll quote it for you here:

> As a side note, this series intentionally does _not_ focus on
> notification of these events to userspace or to other kernel consumers.
> Since these VM fork detection events first need to hit the RNG, we can
> later talk about what sorts of notifications or mmap'd counters the RNG
> should be making accessible to elsewhere. But that's a different sort of
> project and ties into a lot of more complicated concerns beyond this
> more basic patchset. So hopefully we can keep the discussion rather
> focused here to this ACPI business.

What about that was unclear to you?

Anyway, it's a different thing that will have to be designed and
considered carefully, and that design doesn't have a whole lot to do
with this little driver here, except insofar as it could build on top of
it in one way or another. Yes, it's an important thing to do. No, I'm
not going to do it in this patch here. If you want to have a discussion
about that, start a different thread.

Thanks,
Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

On 25.02.22 15:54, Jason A. Donenfeld wrote:
> Hi Alex,
>
> Missed this remark before:
>
> On Fri, Feb 25, 2022 at 02:57:38PM +0100, Alexander Graf wrote:
>> Please expose the vmgenid via /sysfs so that user space even remotely
>> has a chance to check if it's been cloned.
> No. Did you read the 0/2 cover letter? I'll quote it for you here:
>
>> As a side note, this series intentionally does _not_ focus on
>> notification of these events to userspace or to other kernel consumers.
>> Since these VM fork detection events first need to hit the RNG, we can
>> later talk about what sorts of notifications or mmap'd counters the RNG
>> should be making accessible to elsewhere. But that's a different sort of
>> project and ties into a lot of more complicated concerns beyond this
>> more basic patchset. So hopefully we can keep the discussion rather
>> focused here to this ACPI business.
> What about that was unclear to you?
>
> Anyway, it's a different thing that will have to be designed and
> considered carefully, and that design doesn't have a whole lot to do
> with this little driver here, except insofar as it could build on top of
> it in one way or another. Yes, it's an important thing to do. No, I'm
> not going to do it in this patch here. If you want to have a discussion
> about that, start a different thread.


I'm not talking about a notification interface - we've gone through 
great length on that one in the previous submission. What I'm more 
interested in is *any* way for user space to read the current VM Gen ID. 
The same way I'm interested to see other device attributes of my system 
through sysfs.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Jason A. Donenfeld]

Hi Alex,

On Fri, Feb 25, 2022 at 04:15:59PM +0100, Alexander Graf wrote:
> I'm not talking about a notification interface - we've gone through 
> great length on that one in the previous submission. What I'm more 
> interested in is *any* way for user space to read the current VM Gen ID. 
> The same way I'm interested to see other device attributes of my system 
> through sysfs.

Again, no. Same basic objection: we can do this later and design it
coherently with the rest. For example, maybe it's better to expose a
generation counter rather than 16 byte blob, and expect userspace to
call getrandom() subsequently to get something fresh. Or not! But maybe
it should be hashed with a fixed prefix string before being exposed to
userspace. Or not! I don't know, but that's not going to happen on this
patchset. There is no reason at all why that needs to be done here and
now. Trying to do too much at the same time is likely why the previous
efforts from your team stalled out last year. Propose something later,
in a new thread, and we can discuss then. One step at a time...

Jason

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Ard Biesheuvel]

On Fri, 25 Feb 2022 at 14:58, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 25.02.22 13:48, Jason A. Donenfeld wrote:
> >
> > VM Generation ID is a feature from Microsoft, described at
> > <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
> > Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
> > <https://aka.ms/win10rng>, as:
> >
> >      If the OS is running in a VM, there is a problem that most
> >      hypervisors can snapshot the state of the machine and later rewind
> >      the VM state to the saved state. This results in the machine running
> >      a second time with the exact same RNG state, which leads to serious
> >      security problems.  To reduce the window of vulnerability, Windows
> >      10 on a Hyper-V VM will detect when the VM state is reset, retrieve
> >      a unique (not random) value from the hypervisor, and reseed the root
> >      RNG with that unique value.  This does not eliminate the
> >      vulnerability, but it greatly reduces the time during which the RNG
> >      system will produce the same outputs as it did during a previous
> >      instantiation of the same VM state.
> >
> > Linux has the same issue, and given that vmgenid is supported already by
> > multiple hypervisors, we can implement more or less the same solution.
> > So this commit wires up the vmgenid ACPI notification to the RNG's newly
> > added add_vmfork_randomness() function.
> >
> > It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
> > After setting that, use `savevm` in the monitor to save the VM state,
> > then quit QEMU, start it again, and use `loadvm`. That will trigger this
> > driver's notify function, which hands the new UUID to the RNG. This is
> > described in <https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
> > And there are hooks for this in libvirt as well, described in
> > <https://libvirt.org/formatdomain.html#general-metadata>.
> >
> > Note, however, that the treatment of this as a UUID is considered to be
> > an accidental QEMU nuance, per
> > <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
> > so this driver simply treats these bytes as an opaque 128-bit binary
> > blob, as per the spec. This doesn't really make a difference anyway,
> > considering that's how it ends up when handed to the RNG in the end.
> >
> > Cc: Adrian Catangiu <adrian@parity.io>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
...
> > +
> > +       device->driver_data = state;
> > +
> > +out:
> > +       ACPI_FREE(parsed.pointer);
> > +       return ret;
> > +}
> > +
> > +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
> > +{
> > +       struct vmgenid_state *state = acpi_driver_data(device);
> > +       u8 old_id[VMGENID_SIZE];
> > +
> > +       memcpy(old_id, state->this_id, sizeof(old_id));
> > +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
> > +       if (!memcmp(old_id, state->this_id, sizeof(old_id)))
> > +               return;
> > +       add_vmfork_randomness(state->this_id, sizeof(state->this_id));
> > +}
> > +
> > +static const struct acpi_device_id vmgenid_ids[] = {
> > +       { "VMGENID", 0 },
> > +       { "QEMUVGID", 0 },
>
>
> According to the VMGenID spec[1], you can only rely on _CID and _DDN for
> matching. They both contain "VM_Gen_Counter". The list above contains
> _HID values which are not an official identifier for the VMGenID device.
>
> IIRC the ACPI device match logic does match _CID in addition to _HID.
> However, it is limited to 8 characters. Let me paste an experimental
> hack I did back then to do the _CID matching instead.
>
> [1]
> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>

I think matching on the HIDs of two known existing implementations is
fine, as opposed to matching on the (broken) CID of any implementation
that claims to be compatible with it. And dumping random strings into
the _CID property doesn't mesh well with the ACPI spec either, which
is why we don't currently support it.

We could still check _DDN if we wanted to, but I don't think this is
necessary. Other implementations that want to target his driver
explicitly can always put VMGENID or QEMUVGID into the _CID.

Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 5404 bytes --]


On 25.02.22 16:03, Ard Biesheuvel wrote:
> On Fri, 25 Feb 2022 at 14:58, Alexander Graf<graf@amazon.com>  wrote:
>>
>> On 25.02.22 13:48, Jason A. Donenfeld wrote:
>>> VM Generation ID is a feature from Microsoft, described at
>>> <https://go.microsoft.com/fwlink/?LinkId=260709>, and supported by
>>> Hyper-V and QEMU. Its usage is described in Microsoft's RNG whitepaper,
>>> <https://aka.ms/win10rng>, as:
>>>
>>>       If the OS is running in a VM, there is a problem that most
>>>       hypervisors can snapshot the state of the machine and later rewind
>>>       the VM state to the saved state. This results in the machine running
>>>       a second time with the exact same RNG state, which leads to serious
>>>       security problems.  To reduce the window of vulnerability, Windows
>>>       10 on a Hyper-V VM will detect when the VM state is reset, retrieve
>>>       a unique (not random) value from the hypervisor, and reseed the root
>>>       RNG with that unique value.  This does not eliminate the
>>>       vulnerability, but it greatly reduces the time during which the RNG
>>>       system will produce the same outputs as it did during a previous
>>>       instantiation of the same VM state.
>>>
>>> Linux has the same issue, and given that vmgenid is supported already by
>>> multiple hypervisors, we can implement more or less the same solution.
>>> So this commit wires up the vmgenid ACPI notification to the RNG's newly
>>> added add_vmfork_randomness() function.
>>>
>>> It can be used from qemu via the `-device vmgenid,guid=auto` parameter.
>>> After setting that, use `savevm` in the monitor to save the VM state,
>>> then quit QEMU, start it again, and use `loadvm`. That will trigger this
>>> driver's notify function, which hands the new UUID to the RNG. This is
>>> described in<https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vmgenid.txt>.
>>> And there are hooks for this in libvirt as well, described in
>>> <https://libvirt.org/formatdomain.html#general-metadata>.
>>>
>>> Note, however, that the treatment of this as a UUID is considered to be
>>> an accidental QEMU nuance, per
>>> <https://github.com/libguestfs/virt-v2v/blob/master/docs/vm-generation-id-across-hypervisors.txt>,
>>> so this driver simply treats these bytes as an opaque 128-bit binary
>>> blob, as per the spec. This doesn't really make a difference anyway,
>>> considering that's how it ends up when handed to the RNG in the end.
>>>
>>> Cc: Adrian Catangiu<adrian@parity.io>
>>> Cc: Daniel P. Berrangé<berrange@redhat.com>
>>> Cc: Dominik Brodowski<linux@dominikbrodowski.net>
>>> Cc: Ard Biesheuvel<ardb@kernel.org>
>>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>> Reviewed-by: Laszlo Ersek<lersek@redhat.com>
>>> Signed-off-by: Jason A. Donenfeld<Jason@zx2c4.com>
>>> ---
> ...
>>> +
>>> +       device->driver_data = state;
>>> +
>>> +out:
>>> +       ACPI_FREE(parsed.pointer);
>>> +       return ret;
>>> +}
>>> +
>>> +static void vmgenid_acpi_notify(struct acpi_device *device, u32 event)
>>> +{
>>> +       struct vmgenid_state *state = acpi_driver_data(device);
>>> +       u8 old_id[VMGENID_SIZE];
>>> +
>>> +       memcpy(old_id, state->this_id, sizeof(old_id));
>>> +       memcpy(state->this_id, state->next_id, sizeof(state->this_id));
>>> +       if (!memcmp(old_id, state->this_id, sizeof(old_id)))
>>> +               return;
>>> +       add_vmfork_randomness(state->this_id, sizeof(state->this_id));
>>> +}
>>> +
>>> +static const struct acpi_device_id vmgenid_ids[] = {
>>> +       { "VMGENID", 0 },
>>> +       { "QEMUVGID", 0 },
>>
>> According to the VMGenID spec[1], you can only rely on _CID and _DDN for
>> matching. They both contain "VM_Gen_Counter". The list above contains
>> _HID values which are not an official identifier for the VMGenID device.
>>
>> IIRC the ACPI device match logic does match _CID in addition to _HID.
>> However, it is limited to 8 characters. Let me paste an experimental
>> hack I did back then to do the _CID matching instead.
>>
>> [1]
>> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>>
> I think matching on the HIDs of two known existing implementations is
> fine, as opposed to matching on the (broken) CID of any implementation
> that claims to be compatible with it. And dumping random strings into
> the _CID property doesn't mesh well with the ACPI spec either, which
> is why we don't currently support it.
>
> We could still check _DDN if we wanted to, but I don't think this is
> necessary. Other implementations that want to target his driver
> explicitly can always put VMGENID or QEMUVGID into the _CID.


No, they can not. The spec explicitly also states that _HID has to be 
unique per hypervisor:

"A hypervisor must also expose an object below the device, named "_HID", 
which is the "hardware ID." This value is hypervisor specific and must 
be different for each vendor. The Windows generation ID guest driver 
will load on the compatible ID of "VM_Gen_Counter", so it will be 
present regardless of which hypervisor it is running on top of, as long 
as that hypervisor follows the rules above."


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



[-- Attachment #2: Type: text/html, Size: 10471 bytes --]