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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHmME9ov+4Eh0Gzi0XpS6dQ5u-FQnxXoMSQ-HEpFucEmWtNV2A@mail.gmail.com>
Date:   Fri, 25 Feb 2022 15:18:02 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Alexander Graf <graf@...zon.com>
Cc:     KVM list <kvm@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        linux-hyperv@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        adrian@...ity.io, Ard Biesheuvel <ardb@...nel.org>,
        ben@...portsystems.com,
        Daniel P. Berrangé <berrange@...hat.com>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        Dexuan Cui <decui@...rosoft.com>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        Eric Biggers <ebiggers@...nel.org>,
        Eduardo Habkost <ehabkost@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Igor Mammedov <imammedo@...hat.com>,
        Jann Horn <jannh@...gle.com>,
        "K . Y . Srinivasan" <kys@...rosoft.com>,
        Laszlo Ersek <lersek@...hat.com>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        QEMU Developers <qemu-devel@...gnu.org>,
        "Weiss, Radu" <raduweis@...zon.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "Theodore Ts'o" <tytso@....edu>, Wei Liu <wei.liu@...nel.org>
Subject: Re: [PATCH v4] virt: vmgenid: introduce driver for reinitializing RNG
 on VM fork

On Fri, Feb 25, 2022 at 3:12 PM Jason A. Donenfeld <Jason@...c4.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