lkml.org 
[lkml]   [2022]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/2] virt: vmgenid: introduce driver for reinitializing RNG on VM fork
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
> >

\
 
 \ /
  Last update: 2022-02-25 12:54    [W:0.138 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site