[PATCH v5 0/3] ACPI: VM fork detection for RNG

[PATCH v5 0/3] ACPI: VM fork detection for RNG

[Jason A. Donenfeld]

Hi Rafael & Len,

This patchset is directed toward you two specifically. Patches 1/3 and
3/3 have been through the ringer of review a bit already and do not
specifically require your attention, but in v4 we wound up getting hung
up on an ACPI API limitation. This v5 fixes that limitation with its 2/3
patch, with a trivial one line fix, which does require your attention.

Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
actually depends on 2/3 in order to compile without warnings (and be
functional at all). Therefore, it would be nice if you would provide an
"Acked-by" on it and permit me to /also/ take it through my random.git
tree (if it looks like a correct patch to you, of course). This would
make the merge logistics a lot easier. Plus it's a small +1/-1 line
change.

Please have a look and let me know what you think.

Thanks,
Jason

Cc: linux-crypto@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Alexander Graf <graf@amazon.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Alexander Graf (1):
  ACPI: allow longer device IDs

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

 MAINTAINERS                     |   1 +
 drivers/char/random.c           |  50 +++++++++++-----
 drivers/virt/Kconfig            |  11 ++++
 drivers/virt/Makefile           |   1 +
 drivers/virt/vmgenid.c          | 100 ++++++++++++++++++++++++++++++++
 include/linux/mod_devicetable.h |   2 +-
 include/linux/random.h          |   1 +
 7 files changed, 150 insertions(+), 16 deletions(-)
 create mode 100644 drivers/virt/vmgenid.c

-- 
2.35.1

[PATCH v5 1/3] 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>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
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 8171c3bbf460..d9321b9bd3e3 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
@@ -1173,6 +1178,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];
@@ -1563,7 +1583,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

[PATCH v5 2/3] ACPI: allow longer device IDs

[Jason A. Donenfeld]

From: Alexander Graf <graf@amazon.com>

We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
entries of the respective devices. However, we squeeze them into struct
acpi_device_id, which only has 9 bytes space to store the identifier. It
originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
("mod/file2alias: make modalias generation safe for cross compiling"),
presumably on the theory that it would match the ACPI spec so it didn't
matter.

Unfortunately, while most people adhere to the ACPI specs, Microsoft
decided that its VM Generation Counter device [1] should only be
identifiable by _CID with a value of "VM_Gen_Counter", which is longer
than 9 characters.

To allow device drivers to match identifiers that exceed the 9 byte
limit, this simply ups the length to 16, just like it was before the
aforementioned commit. Empirical testing indicates that this
doesn't actually increase vmlinux size, because the ulong in the same
struct caused there to be 7 bytes of padding anyway.

This patch is a prerequisite to add support for VMGenID in Linux, the
subsequent patch in this series. It has been confirmed to also work on
the udev/modalias side in userspace.

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

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Alexander Graf <graf@amazon.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Hi Rafael & Len,

This patchset is directed toward you two specifically. Patches 1/3 and
3/3 have been through the ringer of review a bit already and do not
specifically require your attention, but in v4 we wound up getting hung
up on an ACPI API limitation. This v5 fixes that limitation with this
2/3 patch that you see here, with a trivial one line fix, which does
require your attention.

Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
actually depends on this one here, 2/3, in order to compile without
warnings (and be functional at all). Therefore, it would be nice if you
would provide an "Acked-by" on it and permit me to /also/ take it
through my random.git tree (if it looks like a correct patch to you, of
course). This would make the merge logistics a lot easier. Plus it's a
small +1/-1 line change.

Please have a look and let me know what you think.

Thanks,
Jason

 include/linux/mod_devicetable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4bb71979a8fd..5da5d990ff58 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -211,7 +211,7 @@ struct css_device_id {
 	kernel_ulong_t driver_data;
 };
 
-#define ACPI_ID_LEN	9
+#define ACPI_ID_LEN	16
 
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];
-- 
2.35.1

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> From: Alexander Graf <graf@amazon.com>
>

Please don't invent patch authors like that. Alex's patch that started
this discussion was completely different.

> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> entries of the respective devices. However, we squeeze them into struct
> acpi_device_id, which only has 9 bytes space to store the identifier. It
> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
> ("mod/file2alias: make modalias generation safe for cross compiling"),
> presumably on the theory that it would match the ACPI spec so it didn't
> matter.
>

Please clarify that this applies to the module metadata side of
things. The ACPI subsystem already captures and exposes _HIDs and
_CIDs that are longer than 8 characters, which is why simply
increasing the size of this field is sufficient to create modules that
can match devices that expose a CID that is longer than 8 bytes.

> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> decided that its VM Generation Counter device [1] should only be
> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> than 9 characters.
>
> To allow device drivers to match identifiers that exceed the 9 byte
> limit, this simply ups the length to 16, just like it was before the
> aforementioned commit. Empirical testing indicates that this
> doesn't actually increase vmlinux size, because the ulong in the same
> struct caused there to be 7 bytes of padding anyway.
>

The padding situation only applies to struct acpi_device_id, whereas
ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
only covers statically allocated instances in the core kernel, and
most of the ACPI_ID_LEN uses are probably in drivers. So whether
vmlinux changes size or not is not that relevant.


> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
>
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
to take into account any other uses of ACPI_ID_LEN, and did not bother
to explain why the change was necessary in the context of what it was
trying to achieve.

So, given that we need more than 8 characters to match drivers to
devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
spec), I think this change is necessary and correct.

So, with the authorship/signoff corrected, and the commit log clarified,

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


> ---
> Hi Rafael & Len,
>
> This patchset is directed toward you two specifically. Patches 1/3 and
> 3/3 have been through the ringer of review a bit already and do not
> specifically require your attention, but in v4 we wound up getting hung
> up on an ACPI API limitation. This v5 fixes that limitation with this
> 2/3 patch that you see here, with a trivial one line fix, which does
> require your attention.
>
> Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
> actually depends on this one here, 2/3, in order to compile without
> warnings (and be functional at all). Therefore, it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
>
> Please have a look and let me know what you think.
>
> Thanks,
> Jason
>
>  include/linux/mod_devicetable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>         kernel_ulong_t driver_data;
>  };
>
> -#define ACPI_ID_LEN    9
> +#define ACPI_ID_LEN    16
>
>  struct acpi_device_id {
>         __u8 id[ACPI_ID_LEN];
> --
> 2.35.1
>

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sun, 27 Feb 2022 at 08:31, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > From: Alexander Graf <graf@amazon.com>
> >
>
> Please don't invent patch authors like that. Alex's patch that started
> this discussion was completely different.
>
> > We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> > entries of the respective devices. However, we squeeze them into struct
> > acpi_device_id, which only has 9 bytes space to store the identifier. It
> > originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
> > ("mod/file2alias: make modalias generation safe for cross compiling"),
> > presumably on the theory that it would match the ACPI spec so it didn't
> > matter.
> >
>
> Please clarify that this applies to the module metadata side of
> things. The ACPI subsystem already captures and exposes _HIDs and
> _CIDs that are longer than 8 characters, which is why simply
> increasing the size of this field is sufficient to create modules that
> can match devices that expose a CID that is longer than 8 bytes.
>
> > Unfortunately, while most people adhere to the ACPI specs, Microsoft
> > decided that its VM Generation Counter device [1] should only be
> > identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> > than 9 characters.
> >
> > To allow device drivers to match identifiers that exceed the 9 byte
> > limit, this simply ups the length to 16, just like it was before the
> > aforementioned commit. Empirical testing indicates that this
> > doesn't actually increase vmlinux size, because the ulong in the same
> > struct caused there to be 7 bytes of padding anyway.
> >
>
> The padding situation only applies to struct acpi_device_id, whereas
> ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
> only covers statically allocated instances in the core kernel, and
> most of the ACPI_ID_LEN uses are probably in drivers. So whether
> vmlinux changes size or not is not that relevant.
>
>
> > This patch is a prerequisite to add support for VMGenID in Linux, the
> > subsequent patch in this series. It has been confirmed to also work on
> > the udev/modalias side in userspace.
> >
> > [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
> >
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Signed-off-by: Alexander Graf <graf@amazon.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
> to take into account any other uses of ACPI_ID_LEN, and did not bother
> to explain why the change was necessary in the context of what it was
> trying to achieve.
>

Hmm, actually, ACPI_ID_LEN wasn't used outside of
linux/mod_device_table.h before 6543becf26ff, so changing it at that
point was fine.

I do wonder how much code is out there that blindly assumes the ACPI
core will never deliver more than 8 bytes' worth of _HID/_CID, and
subsequently runs off the end of a statically sized buffer.

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Jason A. Donenfeld]

On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> From: Alexander Graf <graf@amazon.com>
>>
>
> Please don't invent patch authors like that. Alex's patch that started
> this discussion was completely different.

Considering the investigative side ("why won't the _CID match?") and
most the commit message were Alex's, and that those things comprise
95% of what this patch is, and that the code change itself isn't even
part of anything Turing complete, I most certainly did not feel
comfortable stripping Alex's authorship. Instead I added myself as a
co-author at the bottom. When in doubt, err on the side of crediting
others. Alex also took a look at this patch, I am under the impression
of at least, before it went out. Let's minimize the paperwork
policing, okay? I think it'd make for a much more pleasant space here.
If Alex objects he can just simply say, "hey feel free to remove me as
author," and it'll be simple as that, and again doesn't involve your
policing.

>
>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
>> entries of the respective devices. However, we squeeze them into struct
>> acpi_device_id, which only has 9 bytes space to store the identifier. It
>> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
>> ("mod/file2alias: make modalias generation safe for cross compiling"),
>> presumably on the theory that it would match the ACPI spec so it didn't
>> matter.
>>
>
> Please clarify that this applies to the module metadata side of
> things. The ACPI subsystem already captures and exposes _HIDs and
> _CIDs that are longer than 8 characters, which is why simply
> increasing the size of this field is sufficient to create modules that
> can match devices that expose a CID that is longer than 8 bytes.

Good point for strengthening the argument here. Will do.

>
>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
>> decided that its VM Generation Counter device [1] should only be
>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
>> than 9 characters.
>>
>> To allow device drivers to match identifiers that exceed the 9 byte
>> limit, this simply ups the length to 16, just like it was before the
>> aforementioned commit. Empirical testing indicates that this
>> doesn't actually increase vmlinux size, because the ulong in the same
>> struct caused there to be 7 bytes of padding anyway.
>>
>
> The padding situation only applies to struct acpi_device_id, whereas
> ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
> only covers statically allocated instances in the core kernel, and
> most of the ACPI_ID_LEN uses are probably in drivers. So whether
> vmlinux changes size or not is not that relevant.

I actually looked at every usage in the tree (there aren't that many)
and couldn't find a single one where behavior changed, performance
changed, or memory usage changed. I thought we looked together on IRC
so I'm surprised to see you mention this, but maybe I misunderstood
you. Anyway, I can't see the size increase impacting anything at all.
If you see a case, this would be the time to mention that you see
something. I didn't find anything though.

> Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
> to take into account any other uses of ACPI_ID_LEN, and did not bother
> to explain why the change was necessary in the context of what it was
> trying to achieve.

I'm not sure there really were other usages back then. The commit
message seems descriptive enough to me too. This was about cross
compiling, so padding. But it certainly did seem to limit future
drivers in an unintended way, as you wrote:

> So, given that we need more than 8 characters to match drivers to
> devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
> spec), I think this change is necessary and correct.

Right, that's the idea.


>
> So, with the authorship/signoff corrected, and the commit log clarified,
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Thanks.

Hopefully we'll hear from Rafael this week.

Jason

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>
> >> From: Alexander Graf <graf@amazon.com>
> >>
> >
> > Please don't invent patch authors like that. Alex's patch that started
> > this discussion was completely different.
>
> Considering the investigative side ("why won't the _CID match?") and
> most the commit message were Alex's, and that those things comprise
> 95% of what this patch is, and that the code change itself isn't even
> part of anything Turing complete, I most certainly did not feel
> comfortable stripping Alex's authorship. Instead I added myself as a
> co-author at the bottom. When in doubt, err on the side of crediting
> others. Alex also took a look at this patch, I am under the impression
> of at least, before it went out. Let's minimize the paperwork
> policing, okay? I think it'd make for a much more pleasant space here.

Seriously? You want to go down the road again of poisoning the
atmosphere here, and blaming everyone else for doing it? I had enough
of that with the Zinc debacle, and I thought we had moved beyond that,
after having collaborated constructively with you on various topics
over the past couple of years.

Please stop with the ad hominems in response to criticism on factual
aspects of your code. Putting someone else's authorship on code they
did not write is not cool, and pointing that out is *not* what is
making this space unpleasant.
And 'paperwork policing' is sadly an important aspect of a high
profile open source project such as Linux.

> If Alex objects he can just simply say, "hey feel free to remove me as
> author," and it'll be simple as that, and again doesn't involve your
> policing.
>

When you ask for my review, you get my review. If there are aspects of
your contributions that are a priori exempt from criticism, I think
you're in the wrong place.

> >> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> >> entries of the respective devices. However, we squeeze them into struct
> >> acpi_device_id, which only has 9 bytes space to store the identifier. It
> >> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
> >> ("mod/file2alias: make modalias generation safe for cross compiling"),
> >> presumably on the theory that it would match the ACPI spec so it didn't
> >> matter.
> >>
> >
> > Please clarify that this applies to the module metadata side of
> > things. The ACPI subsystem already captures and exposes _HIDs and
> > _CIDs that are longer than 8 characters, which is why simply
> > increasing the size of this field is sufficient to create modules that
> > can match devices that expose a CID that is longer than 8 bytes.
>
> Good point for strengthening the argument here. Will do.
>
> >
> >> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> >> decided that its VM Generation Counter device [1] should only be
> >> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> >> than 9 characters.
> >>
> >> To allow device drivers to match identifiers that exceed the 9 byte
> >> limit, this simply ups the length to 16, just like it was before the
> >> aforementioned commit. Empirical testing indicates that this
> >> doesn't actually increase vmlinux size, because the ulong in the same
> >> struct caused there to be 7 bytes of padding anyway.
> >>
> >
> > The padding situation only applies to struct acpi_device_id, whereas
> > ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
> > only covers statically allocated instances in the core kernel, and
> > most of the ACPI_ID_LEN uses are probably in drivers. So whether
> > vmlinux changes size or not is not that relevant.
>
> I actually looked at every usage in the tree (there aren't that many)
> and couldn't find a single one where behavior changed, performance
> changed, or memory usage changed. I thought we looked together on IRC
> so I'm surprised to see you mention this, but maybe I misunderstood
> you. Anyway, I can't see the size increase impacting anything at all.
> If you see a case, this would be the time to mention that you see
> something. I didn't find anything though.
>

If you did not check the rodata/data/bss sizes of all modules
depending on this macro, or checked their memory usage at runtime, you
cannot definitively say that nothing has changed.

*However*, as I argued below, using ACPI_ID_LEN to allocate a string
that needs to hold a HID or CID provided by the ACPI subsystem might
well result in a buffer overrun, as the ACPI subsystem will happily
return longer strings.

So my conclusion is that the change is ok, which is why I gave my reviewed-by.

> > Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
> > to take into account any other uses of ACPI_ID_LEN, and did not bother
> > to explain why the change was necessary in the context of what it was
> > trying to achieve.
>
> I'm not sure there really were other usages back then. The commit
> message seems descriptive enough to me too. This was about cross
> compiling, so padding. But it certainly did seem to limit future
> drivers in an unintended way, as you wrote:
>
> > So, given that we need more than 8 characters to match drivers to
> > devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
> > spec), I think this change is necessary and correct.
>
> Right, that's the idea.
>
>
> >
> > So, with the authorship/signoff corrected, and the commit log clarified,
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
> Thanks.
>
> Hopefully we'll hear from Rafael this week.
>
> Jason

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >>
> > >> From: Alexander Graf <graf@amazon.com>
> > >>
> > >
> > > Please don't invent patch authors like that. Alex's patch that started
> > > this discussion was completely different.
> >
> > Considering the investigative side ("why won't the _CID match?") and
> > most the commit message were Alex's, and that those things comprise
> > 95% of what this patch is, and that the code change itself isn't even
> > part of anything Turing complete, I most certainly did not feel
> > comfortable stripping Alex's authorship. Instead I added myself as a
> > co-author at the bottom. When in doubt, err on the side of crediting
> > others. Alex also took a look at this patch, I am under the impression
> > of at least, before it went out. Let's minimize the paperwork
> > policing, okay? I think it'd make for a much more pleasant space here.
>
...
>
> Please stop with the ad hominems in response to criticism on factual
> aspects of your code. Putting someone else's authorship on code they
> did not write is not cool, and pointing that out is *not* what is
> making this space unpleasant.
> And 'paperwork policing' is sadly an important aspect of a high
> profile open source project such as Linux.
>

I typed this before reading your message on IRC, which reads:

"Alex looked at that patch before i sent it out and did not object to
me keeping his authorship. I wouldn't have sent it out otherwise."

and so I stand corrected if this is true. But please, next time,
please be more clear about these things.

-- 
Ard.

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Alexander Graf]

On 27.02.22 11:47, Ard Biesheuvel wrote:
> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>> On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>>> From: Alexander Graf <graf@amazon.com>
>>>>>
>>>> Please don't invent patch authors like that. Alex's patch that started
>>>> this discussion was completely different.
>>> Considering the investigative side ("why won't the _CID match?") and
>>> most the commit message were Alex's, and that those things comprise
>>> 95% of what this patch is, and that the code change itself isn't even
>>> part of anything Turing complete, I most certainly did not feel
>>> comfortable stripping Alex's authorship. Instead I added myself as a
>>> co-author at the bottom. When in doubt, err on the side of crediting
>>> others. Alex also took a look at this patch, I am under the impression
>>> of at least, before it went out. Let's minimize the paperwork
>>> policing, okay? I think it'd make for a much more pleasant space here.
> ...
>> Please stop with the ad hominems in response to criticism on factual
>> aspects of your code. Putting someone else's authorship on code they
>> did not write is not cool, and pointing that out is *not* what is
>> making this space unpleasant.
>> And 'paperwork policing' is sadly an important aspect of a high
>> profile open source project such as Linux.
>>
> I typed this before reading your message on IRC, which reads:
>
> "Alex looked at that patch before i sent it out and did not object to
> me keeping his authorship. I wouldn't have sent it out otherwise."
>
> and so I stand corrected if this is true. But please, next time,
> please be more clear about these things.


Yes, he did reach out to me on a separate channel and I told him to go 
for it :). Sorry if I created some confusion with that.


Thanks,

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 v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 27.02.22 11:47, Ard Biesheuvel wrote:
> > On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
> >> On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>>> From: Alexander Graf <graf@amazon.com>
> >>>>>
> >>>> Please don't invent patch authors like that. Alex's patch that started
> >>>> this discussion was completely different.
> >>> Considering the investigative side ("why won't the _CID match?") and
> >>> most the commit message were Alex's, and that those things comprise
> >>> 95% of what this patch is, and that the code change itself isn't even
> >>> part of anything Turing complete, I most certainly did not feel
> >>> comfortable stripping Alex's authorship. Instead I added myself as a
> >>> co-author at the bottom. When in doubt, err on the side of crediting
> >>> others. Alex also took a look at this patch, I am under the impression
> >>> of at least, before it went out. Let's minimize the paperwork
> >>> policing, okay? I think it'd make for a much more pleasant space here.
> > ...
> >> Please stop with the ad hominems in response to criticism on factual
> >> aspects of your code. Putting someone else's authorship on code they
> >> did not write is not cool, and pointing that out is *not* what is
> >> making this space unpleasant.
> >> And 'paperwork policing' is sadly an important aspect of a high
> >> profile open source project such as Linux.
> >>
> > I typed this before reading your message on IRC, which reads:
> >
> > "Alex looked at that patch before i sent it out and did not object to
> > me keeping his authorship. I wouldn't have sent it out otherwise."
> >
> > and so I stand corrected if this is true. But please, next time,
> > please be more clear about these things.
>
>
> Yes, he did reach out to me on a separate channel and I told him to go
> for it :). Sorry if I created some confusion with that.
>

No, my bad. But in my defence, everyone on the original thread knows
that this single oneline change was suggested by Jason, not you, and
so seeing him posting it as your patch did confuse me a little.

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Alexander Graf]

On 27.02.22 12:43, Ard Biesheuvel wrote:
> On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
>>
>> On 27.02.22 11:47, Ard Biesheuvel wrote:
>>> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>>> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>>>>> From: Alexander Graf <graf@amazon.com>
>>>>>>>
>>>>>> Please don't invent patch authors like that. Alex's patch that started
>>>>>> this discussion was completely different.
>>>>> Considering the investigative side ("why won't the _CID match?") and
>>>>> most the commit message were Alex's, and that those things comprise
>>>>> 95% of what this patch is, and that the code change itself isn't even
>>>>> part of anything Turing complete, I most certainly did not feel
>>>>> comfortable stripping Alex's authorship. Instead I added myself as a
>>>>> co-author at the bottom. When in doubt, err on the side of crediting
>>>>> others. Alex also took a look at this patch, I am under the impression
>>>>> of at least, before it went out. Let's minimize the paperwork
>>>>> policing, okay? I think it'd make for a much more pleasant space here.
>>> ...
>>>> Please stop with the ad hominems in response to criticism on factual
>>>> aspects of your code. Putting someone else's authorship on code they
>>>> did not write is not cool, and pointing that out is *not* what is
>>>> making this space unpleasant.
>>>> And 'paperwork policing' is sadly an important aspect of a high
>>>> profile open source project such as Linux.
>>>>
>>> I typed this before reading your message on IRC, which reads:
>>>
>>> "Alex looked at that patch before i sent it out and did not object to
>>> me keeping his authorship. I wouldn't have sent it out otherwise."
>>>
>>> and so I stand corrected if this is true. But please, next time,
>>> please be more clear about these things.
>>
>> Yes, he did reach out to me on a separate channel and I told him to go
>> for it :). Sorry if I created some confusion with that.
>>
> No, my bad. But in my defence, everyone on the original thread knows
> that this single oneline change was suggested by Jason, not you, and
> so seeing him posting it as your patch did confuse me a little.


The idea came up 1y ago in conversations with Adrian when we tried to 
make _CID matching work. Unfortunately I did not file a patent for the 
mechanism to increase the array size until data fits :). It's such a 
revolutionary invention!

Back to seriousness, I'm pretty indifferent on the attribution for it. 
What I'm more interested in is a solution that allows us to match the 
correct identifier :). My take is that Jason just wanted to be nice and 
was trying to give credit.


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 v5 2/3] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Sun, 27 Feb 2022 at 12:48, Alexander Graf <graf@amazon.com> wrote:
>
>
> On 27.02.22 12:43, Ard Biesheuvel wrote:
> > On Sun, 27 Feb 2022 at 12:39, Alexander Graf <graf@amazon.com> wrote:
> >>
> >> On 27.02.22 11:47, Ard Biesheuvel wrote:
> >>> On Sun, 27 Feb 2022 at 11:30, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>> On Sun, 27 Feb 2022 at 11:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>>> On 2/27/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>>> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>>>>> From: Alexander Graf <graf@amazon.com>
> >>>>>>>
> >>>>>> Please don't invent patch authors like that. Alex's patch that started
> >>>>>> this discussion was completely different.
> >>>>> Considering the investigative side ("why won't the _CID match?") and
> >>>>> most the commit message were Alex's, and that those things comprise
> >>>>> 95% of what this patch is, and that the code change itself isn't even
> >>>>> part of anything Turing complete, I most certainly did not feel
> >>>>> comfortable stripping Alex's authorship. Instead I added myself as a
> >>>>> co-author at the bottom. When in doubt, err on the side of crediting
> >>>>> others. Alex also took a look at this patch, I am under the impression
> >>>>> of at least, before it went out. Let's minimize the paperwork
> >>>>> policing, okay? I think it'd make for a much more pleasant space here.
> >>> ...
> >>>> Please stop with the ad hominems in response to criticism on factual
> >>>> aspects of your code. Putting someone else's authorship on code they
> >>>> did not write is not cool, and pointing that out is *not* what is
> >>>> making this space unpleasant.
> >>>> And 'paperwork policing' is sadly an important aspect of a high
> >>>> profile open source project such as Linux.
> >>>>
> >>> I typed this before reading your message on IRC, which reads:
> >>>
> >>> "Alex looked at that patch before i sent it out and did not object to
> >>> me keeping his authorship. I wouldn't have sent it out otherwise."
> >>>
> >>> and so I stand corrected if this is true. But please, next time,
> >>> please be more clear about these things.
> >>
> >> Yes, he did reach out to me on a separate channel and I told him to go
> >> for it :). Sorry if I created some confusion with that.
> >>
> > No, my bad. But in my defence, everyone on the original thread knows
> > that this single oneline change was suggested by Jason, not you, and
> > so seeing him posting it as your patch did confuse me a little.
>
>
> The idea came up 1y ago in conversations with Adrian when we tried to
> make _CID matching work. Unfortunately I did not file a patent for the
> mechanism to increase the array size until data fits :). It's such a
> revolutionary invention!
>
> Back to seriousness, I'm pretty indifferent on the attribution for it.
> What I'm more interested in is a solution that allows us to match the
> correct identifier :). My take is that Jason just wanted to be nice and
> was trying to give credit.
>

Giving credit is nice, but I do think that obfuscating authorship like
this is generally not preferred.

But in this particular case, it hardly matters, so let's not debate
this any further.

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Alexander Graf]

On 26.02.22 23:06, Jason A. Donenfeld wrote:
> From: Alexander Graf <graf@amazon.com>
>
> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> entries of the respective devices. However, we squeeze them into struct
> acpi_device_id, which only has 9 bytes space to store the identifier. It
> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
> ("mod/file2alias: make modalias generation safe for cross compiling"),
> presumably on the theory that it would match the ACPI spec so it didn't
> matter.
>
> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> decided that its VM Generation Counter device [1] should only be
> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> than 9 characters.
>
> To allow device drivers to match identifiers that exceed the 9 byte
> limit, this simply ups the length to 16, just like it was before the
> aforementioned commit. Empirical testing indicates that this


This is only true for 64bit systems where padding automatically bloated 
to 9 byte array to 16. I still believe the patch is fine as it is, but 
there will be minor .rodata overhead on 32bit targets which you may want 
to quantify in the patch description.


Thanks a lot for sending this out! And let's hope that 16 bytes is 
enough for everyone.

Alex


> doesn't actually increase vmlinux size, because the ulong in the same
> struct caused there to be 7 bytes of padding anyway.
>
> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
>
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Hi Rafael & Len,
>
> This patchset is directed toward you two specifically. Patches 1/3 and
> 3/3 have been through the ringer of review a bit already and do not
> specifically require your attention, but in v4 we wound up getting hung
> up on an ACPI API limitation. This v5 fixes that limitation with this
> 2/3 patch that you see here, with a trivial one line fix, which does
> require your attention.
>
> Patches 1/3 and 3/3 will go through my random.git tree. However, 3/3
> actually depends on this one here, 2/3, in order to compile without
> warnings (and be functional at all). Therefore, it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
>
> Please have a look and let me know what you think.
>
> Thanks,
> Jason
>
>   include/linux/mod_devicetable.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>          kernel_ulong_t driver_data;
>   };
>
> -#define ACPI_ID_LEN    9
> +#define ACPI_ID_LEN    16
>
>   struct acpi_device_id {
>          __u8 id[ACPI_ID_LEN];
> --
> 2.35.1
>



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 v5 2/3] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Alex,

On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > To allow device drivers to match identifiers that exceed the 9 byte
> > limit, this simply ups the length to 16, just like it was before the
> > aforementioned commit. Empirical testing indicates that this
> 
> 
> This is only true for 64bit systems where padding automatically bloated 
> to 9 byte array to 16. I still believe the patch is fine as it is, but 
> there will be minor .rodata overhead on 32bit targets which you may want 
> to quantify in the patch description.

Good point. So I just tried this out with a 32-bit i686 kernel and the
results are the same again for the size of vmlinux. I then ran `objdump
--headers` and looked at the size of the .rodata section, where it's
also the same. I'm not quite sure what to make of this, as it's not what
I was expecting, but I think I tested it right. So maybe we're lucky
here?

Jason

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hey again,

On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Alex,
>
> On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > > To allow device drivers to match identifiers that exceed the 9 byte
> > > limit, this simply ups the length to 16, just like it was before the
> > > aforementioned commit. Empirical testing indicates that this
> >
> >
> > This is only true for 64bit systems where padding automatically bloated
> > to 9 byte array to 16. I still believe the patch is fine as it is, but
> > there will be minor .rodata overhead on 32bit targets which you may want
> > to quantify in the patch description.
>
> Good point. So I just tried this out with a 32-bit i686 kernel and the
> results are the same again for the size of vmlinux. I then ran `objdump
> --headers` and looked at the size of the .rodata section, where it's
> also the same. I'm not quite sure what to make of this, as it's not what
> I was expecting, but I think I tested it right. So maybe we're lucky
> here?

I tried a little harder to get _some_ difference on 32-bit, and
managed to get one by doing i386_defconfig and then switching off
modules to make all M into Y, and then compared sizes:

vmlinux: 25590780 -> 25598972, so a 0.032% increase.
bzImage: 8698944 -> 8699424, so a 0.0055% increase.

So it does increase, ever so slightly, but a) on 32-bit, and b) a
super, super tiny amount.

In other words, I still think this patch is very much a-okay. But very
eager to hear from Rafael on the approach.

Jason

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Rafael J. Wysocki]

+Mika, Andy and Hans in case they have something to add

On Mon, Feb 28, 2022 at 12:27 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey again,
>
> On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Alex,
> >
> > On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
> > > > To allow device drivers to match identifiers that exceed the 9 byte
> > > > limit, this simply ups the length to 16, just like it was before the
> > > > aforementioned commit. Empirical testing indicates that this
> > >
> > >
> > > This is only true for 64bit systems where padding automatically bloated
> > > to 9 byte array to 16. I still believe the patch is fine as it is, but
> > > there will be minor .rodata overhead on 32bit targets which you may want
> > > to quantify in the patch description.
> >
> > Good point. So I just tried this out with a 32-bit i686 kernel and the
> > results are the same again for the size of vmlinux. I then ran `objdump
> > --headers` and looked at the size of the .rodata section, where it's
> > also the same. I'm not quite sure what to make of this, as it's not what
> > I was expecting, but I think I tested it right. So maybe we're lucky
> > here?
>
> I tried a little harder to get _some_ difference on 32-bit, and
> managed to get one by doing i386_defconfig and then switching off
> modules to make all M into Y, and then compared sizes:
>
> vmlinux: 25590780 -> 25598972, so a 0.032% increase.
> bzImage: 8698944 -> 8699424, so a 0.0055% increase.
>
> So it does increase, ever so slightly, but a) on 32-bit, and b) a
> super, super tiny amount.
>
> In other words, I still think this patch is very much a-okay. But very
> eager to hear from Rafael on the approach.

Increasing the ACPI_ID_LEN value is fine with me, but the patch
changelog is not entirely accurate.

The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
device ID matching and it is generally used for creating lists of ACPI
device IDs in drivers (and allow/deny lists etc).  The device IDs
extracted from the ACPI tables can be longer than ACPI_ID_LEN.

This means that drivers cannot match device IDs longer than 8
characters (excluding the terminating 0), because the IDs in the lists
used by them for ID matching cannot be longer than this and not
because the ACPI subsystem is limited by that value.

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Rafael,

On Mon, Feb 28, 2022 at 7:19 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> Increasing the ACPI_ID_LEN value is fine with me, but the patch
> changelog is not entirely accurate.
>
> The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
> device ID matching and it is generally used for creating lists of ACPI
> device IDs in drivers (and allow/deny lists etc).  The device IDs
> extracted from the ACPI tables can be longer than ACPI_ID_LEN.
>
> This means that drivers cannot match device IDs longer than 8
> characters (excluding the terminating 0), because the IDs in the lists
> used by them for ID matching cannot be longer than this and not
> because the ACPI subsystem is limited by that value.

Thanks for your notes there. I think Ard more or less pointed out
something similar too. I'll amend the commit message, send a v2, and
hopefully this change is okay with Mika/Andy/Hans.

Regards,
Jason

[PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

From: Alexander Graf <graf@amazon.com>

We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
entries of the respective devices. However, when making structs for
matching, we squeeze those IDs into acpi_device_id, which only has 9
bytes space to store the identifier. The subsystem actually captures the
full length of the IDs, and the modalias has the full length, but this
struct we use for matching is limited. It originally had 16 bytes, but
was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
modalias generation safe for cross compiling"), presumably on the theory
that it would match the ACPI spec so it didn't matter.

Unfortunately, while most people adhere to the ACPI specs, Microsoft
decided that its VM Generation Counter device [1] should only be
identifiable by _CID with a value of "VM_Gen_Counter", which is longer
than 9 characters.

To allow device drivers to match identifiers that exceed the 9 byte
limit, this simply ups the length to 16, just like it was before the
aforementioned commit. Empirical testing indicates that this
doesn't actually increase vmlinux size on 64-bit, because the ulong in
the same struct caused there to be 7 bytes of padding anyway, and when
doing a s/M/Y/g i386_defconfig build, the bzImage only increased by
0.0055%, so negligible.

This patch is a prerequisite to add support for VMGenID in Linux, the
subsequent patch in this series. It has been confirmed to also work on
the udev/modalias side in userspace.

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

Signed-off-by: Alexander Graf <graf@amazon.com>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
[Jason: reworked commit message a bit, went with len=16 approach.]
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Hi Rafael,

This patch is directed toward you specifically. The first and last patch
of the series this is part of have been through the ringer of review a
bit already and do not specifically require your attention, but we wound
up getting hung up on an ACPI ID matching API limitation. This patch
fixes that limitation with this patch that you see here, with a trivial
one line fix, which does require your attention.

The other patches will go through my random.git tree naturally, but
because those patches depend on this one here, in order to compile
without warnings (and be functional at all), it would be nice if you
would provide an "Acked-by" on it and permit me to /also/ take it
through my random.git tree (if it looks like a correct patch to you, of
course). This would make the merge logistics a lot easier. Plus it's a
small +1/-1 line change.

This v6 updates the commit message.

Please have a look and let me know what you think.

Thanks,
Jason

 include/linux/mod_devicetable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4bb71979a8fd..5da5d990ff58 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -211,7 +211,7 @@ struct css_device_id {
 	kernel_ulong_t driver_data;
 };
 
-#define ACPI_ID_LEN	9
+#define ACPI_ID_LEN	16
 
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];
-- 
2.35.1

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Andy Shevchenko]

On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> From: Alexander Graf <graf@amazon.com>
>
> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> entries of the respective devices. However, when making structs for
> matching, we squeeze those IDs into acpi_device_id, which only has 9
> bytes space to store the identifier. The subsystem actually captures the
> full length of the IDs, and the modalias has the full length, but this
> struct we use for matching is limited. It originally had 16 bytes, but
> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> modalias generation safe for cross compiling"), presumably on the theory
> that it would match the ACPI spec so it didn't matter.

> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> decided that its VM Generation Counter device [1] should only be
> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> than 9 characters.

Then why do we not see the ECR from somebody to update the spec or to
fix MS' abuse of it?
I believe _this_ should be the prerequisite to the proposed change.

> To allow device drivers to match identifiers that exceed the 9 byte
> limit, this simply ups the length to 16, just like it was before the
> aforementioned commit. Empirical testing indicates that this
> doesn't actually increase vmlinux size on 64-bit, because the ulong in
> the same struct caused there to be 7 bytes of padding anyway, and when
> doing a s/M/Y/g i386_defconfig build, the bzImage only increased by
> 0.0055%, so negligible.
>
> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Andy,

On Mon, Feb 28, 2022 at 9:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Then why do we not see the ECR from somebody to update the spec or to
> fix MS' abuse of it?
> I believe _this_ should be the prerequisite to the proposed change.

Sorry, but no; that seems like a ridiculous requirement. There's
virtual "hardware" out there that is behaving like this. It serves an
important security function. There are no technical downsides to
making the change. It works well.

Yet you're upset that they didn't follow the rules. Indeed, tsk tsk on
Microsoft. But that doesn't mean Linux users should suffer and have
poorer security than Windows.

This kind of thing happens all the time. We do our best to find the
cleanest solution to the problem, and live with it. Linux isn't a
one-hundred-per-cent-by-the-spec operating system, because the things
it works with are often not one-hundred-per-cent-by-the-spec. We even
have quirks tables. It's part of life.

And in this case, the solution is so straightforward that it doesn't
even require anything closely resembling a "hack" or a layering
violation. And that space is already being used (zero filled) anyway
because of struct padding.

This fix is the "clean" way of doing it. Why make this harder than it
needs to be?

Jason

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > From: Alexander Graf <graf@amazon.com>
> >
> > We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> > entries of the respective devices. However, when making structs for
> > matching, we squeeze those IDs into acpi_device_id, which only has 9
> > bytes space to store the identifier. The subsystem actually captures the
> > full length of the IDs, and the modalias has the full length, but this
> > struct we use for matching is limited. It originally had 16 bytes, but
> > was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> > modalias generation safe for cross compiling"), presumably on the theory
> > that it would match the ACPI spec so it didn't matter.
>
> > Unfortunately, while most people adhere to the ACPI specs, Microsoft
> > decided that its VM Generation Counter device [1] should only be
> > identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> > than 9 characters.
>
> Then why do we not see the ECR from somebody to update the spec or to
> fix MS' abuse of it?
> I believe _this_ should be the prerequisite to the proposed change.
>

What exactly are you suggesting here? That the contributor of this
patch joins the UEFI forum as an individual adopter in order to get
the ACPI spec updated before we can advance with this patch? Or that
he works with Microsoft to get them to refrain from violating it?

I don't think that is reasonable or realistic. The kernel is already
riddled with UEFI and ACPI quirks that are only there because some
teams at MS don't take the ACPI spec too literally (which is why they
have their own AML compiler, for one), and PC vendors only care about
the Windows sticker, so they don't care about the ACPI spec either.

So I don't think this is the right time to get pedantic about this.
Our ACPI subsystem already deals with CIDs that are longer than 8
characters (which are btw permitted by the ACPI spec for bus topology
related metadata), the only thing being changed here is the ability to
actually match against such identifiers.

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Andy Shevchenko]

On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > From: Alexander Graf <graf@amazon.com>
> > >
> > > We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> > > entries of the respective devices. However, when making structs for
> > > matching, we squeeze those IDs into acpi_device_id, which only has 9
> > > bytes space to store the identifier. The subsystem actually captures the
> > > full length of the IDs, and the modalias has the full length, but this
> > > struct we use for matching is limited. It originally had 16 bytes, but
> > > was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> > > modalias generation safe for cross compiling"), presumably on the theory
> > > that it would match the ACPI spec so it didn't matter.
> >
> > > Unfortunately, while most people adhere to the ACPI specs, Microsoft
> > > decided that its VM Generation Counter device [1] should only be
> > > identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> > > than 9 characters.
> >
> > Then why do we not see the ECR from somebody to update the spec or to
> > fix MS' abuse of it?
> > I believe _this_ should be the prerequisite to the proposed change.
> 
> What exactly are you suggesting here? That the contributor of this
> patch joins the UEFI forum as an individual adopter in order to get
> the ACPI spec updated before we can advance with this patch? Or that
> he works with Microsoft to get them to refrain from violating it?
> 
> I don't think that is reasonable or realistic. The kernel is already
> riddled with UEFI and ACPI quirks that are only there because some
> teams at MS don't take the ACPI spec too literally (which is why they
> have their own AML compiler, for one), and PC vendors only care about
> the Windows sticker, so they don't care about the ACPI spec either.
> 
> So I don't think this is the right time to get pedantic about this.
> Our ACPI subsystem already deals with CIDs that are longer than 8
> characters (which are btw permitted by the ACPI spec for bus topology
> related metadata), the only thing being changed here is the ability to
> actually match against such identifiers.

My point is that this is clear abuse of the spec and:
1) we have to enable the broken, because it is already in the wild with
   the comment that this is an issue

AND

2) issue an ECR / work with MS to make sure they understand the problem.

This can be done in parallel. What I meant as a prerequisite is to start doing
2) while we have 1) on table.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Andy,

On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> My point is that this is clear abuse of the spec and:
> 1) we have to enable the broken, because it is already in the wild with
>    the comment that this is an issue
>
> AND
>
> 2) issue an ECR / work with MS to make sure they understand the problem.
>
> This can be done in parallel. What I meant as a prerequisite is to start doing
> 2) while we have 1) on table.

Oh, okay, that makes sense. If you want to get (2) going, by all means
go for it. I have no idea how to do this myself; Ard said something
about joining the UEFI forum as an individual something or another but
I don't think I'm the man for the job there. Is this something that
Intel can do with their existing membership (is that the right term?)
at the UEFI forum? Or maybe a Microsoft engineer on the list?

From my side, regarding (1), I'm basically just waiting for Rafael's
"Acked-by" (or an explicit nack) so I can put this in my tree and move
on.

Jason

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022 1:55 PM
> 
> Hi Andy,
> 
> On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > My point is that this is clear abuse of the spec and:
> > 1) we have to enable the broken, because it is already in the wild with
> >    the comment that this is an issue
> >
> > AND
> >
> > 2) issue an ECR / work with MS to make sure they understand the problem.
> >
> > This can be done in parallel. What I meant as a prerequisite is to start doing
> > 2) while we have 1) on table.
> 
> Oh, okay, that makes sense. If you want to get (2) going, by all means
> go for it. I have no idea how to do this myself; Ard said something
> about joining the UEFI forum as an individual something or another but
> I don't think I'm the man for the job there. Is this something that
> Intel can do with their existing membership (is that the right term?)
> at the UEFI forum? Or maybe a Microsoft engineer on the list?

My team at Microsoft, which works on Linux, filed a bug on this
issue against the Hyper-V team about a year ago, probably when the issue
was raised during the previous attempt to implement the functionality
in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
changing to an identifier that meets the spec is problematic because
of backwards compatibility with Windows guests on Hyper-V that
consume the current identifier.  There's no practical way to have Hyper-V
provide a conformant identifier AND fix all the Windows guests out in
the wild to consume the new identifier.   As a result, at this point Hyper-V
is not planning to change anything.

It's a lousy state-of-affairs, but as mentioned previously in this thread,
it seems to be one that we will have to live with.

Michael

> 
> From my side, regarding (1), I'm basically just waiting for Rafael's
> "Acked-by" (or an explicit nack) so I can put this in my tree and move
> on.
> 
> Jason

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

On Mon, Feb 28, 2022 at 11:14 PM Michael Kelley (LINUX)
<mikelley@microsoft.com> wrote:
> the wild to consume the new identifier.   As a result, at this point Hyper-V
> is not planning to change anything.
>
> It's a lousy state-of-affairs, but as mentioned previously in this thread,
> it seems to be one that we will have to live with.

I should note that QEMU and VMware also support this too. So, yea, I
guess that boat has sailed.

Should that Hyper-V team do the ECR thing Andy was talking about?

Jason

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
<mikelley@microsoft.com> wrote:
>
> From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022 1:55 PM
> >
> > Hi Andy,
> >
> > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > My point is that this is clear abuse of the spec and:
> > > 1) we have to enable the broken, because it is already in the wild with
> > >    the comment that this is an issue
> > >
> > > AND
> > >
> > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > >
> > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > 2) while we have 1) on table.
> >
> > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > go for it. I have no idea how to do this myself; Ard said something
> > about joining the UEFI forum as an individual something or another but
> > I don't think I'm the man for the job there. Is this something that
> > Intel can do with their existing membership (is that the right term?)
> > at the UEFI forum? Or maybe a Microsoft engineer on the list?
>
> My team at Microsoft, which works on Linux, filed a bug on this
> issue against the Hyper-V team about a year ago, probably when the issue
> was raised during the previous attempt to implement the functionality
> in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> changing to an identifier that meets the spec is problematic because
> of backwards compatibility with Windows guests on Hyper-V that
> consume the current identifier.  There's no practical way to have Hyper-V
> provide a conformant identifier AND fix all the Windows guests out in
> the wild to consume the new identifier.   As a result, at this point Hyper-V
> is not planning to change anything.
>
> It's a lousy state-of-affairs, but as mentioned previously in this thread,
> it seems to be one that we will have to live with.
>

Thanks for chiming in.

Why not do something like

Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )

?

That way, older clients can match on the existing _CID and new clients
can match on the spec compliant one.

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:22 PM
> 
> On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
> <mikelley@microsoft.com> wrote:
> >
> > From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022
> 1:55 PM
> > >
> > > Hi Andy,
> > >
> > > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > My point is that this is clear abuse of the spec and:
> > > > 1) we have to enable the broken, because it is already in the wild with
> > > >    the comment that this is an issue
> > > >
> > > > AND
> > > >
> > > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > > >
> > > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > > 2) while we have 1) on table.
> > >
> > > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > > go for it. I have no idea how to do this myself; Ard said something
> > > about joining the UEFI forum as an individual something or another but
> > > I don't think I'm the man for the job there. Is this something that
> > > Intel can do with their existing membership (is that the right term?)
> > > at the UEFI forum? Or maybe a Microsoft engineer on the list?
> >
> > My team at Microsoft, which works on Linux, filed a bug on this
> > issue against the Hyper-V team about a year ago, probably when the issue
> > was raised during the previous attempt to implement the functionality
> > in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> > that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> > changing to an identifier that meets the spec is problematic because
> > of backwards compatibility with Windows guests on Hyper-V that
> > consume the current identifier.  There's no practical way to have Hyper-V
> > provide a conformant identifier AND fix all the Windows guests out in
> > the wild to consume the new identifier.   As a result, at this point Hyper-V
> > is not planning to change anything.
> >
> > It's a lousy state-of-affairs, but as mentioned previously in this thread,
> > it seems to be one that we will have to live with.
> >
> 
> Thanks for chiming in.
> 
> Why not do something like
> 
> Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )
> 
> ?
> 
> That way, older clients can match on the existing _CID and new clients
> can match on the spec compliant one.

I'll run this by the Hyper-V guys.  I don't have the ACPI expertise to disagree
with them when they say they can't change it. :-(

Michael

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Mon, 28 Feb 2022 at 23:38, Michael Kelley (LINUX)
<mikelley@microsoft.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:22 PM
> >
> > On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
> > <mikelley@microsoft.com> wrote:
> > >
> > > From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022
> > 1:55 PM
> > > >
> > > > Hi Andy,
> > > >
> > > > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > My point is that this is clear abuse of the spec and:
> > > > > 1) we have to enable the broken, because it is already in the wild with
> > > > >    the comment that this is an issue
> > > > >
> > > > > AND
> > > > >
> > > > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > > > >
> > > > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > > > 2) while we have 1) on table.
> > > >
> > > > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > > > go for it. I have no idea how to do this myself; Ard said something
> > > > about joining the UEFI forum as an individual something or another but
> > > > I don't think I'm the man for the job there. Is this something that
> > > > Intel can do with their existing membership (is that the right term?)
> > > > at the UEFI forum? Or maybe a Microsoft engineer on the list?
> > >
> > > My team at Microsoft, which works on Linux, filed a bug on this
> > > issue against the Hyper-V team about a year ago, probably when the issue
> > > was raised during the previous attempt to implement the functionality
> > > in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> > > that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> > > changing to an identifier that meets the spec is problematic because
> > > of backwards compatibility with Windows guests on Hyper-V that
> > > consume the current identifier.  There's no practical way to have Hyper-V
> > > provide a conformant identifier AND fix all the Windows guests out in
> > > the wild to consume the new identifier.   As a result, at this point Hyper-V
> > > is not planning to change anything.
> > >
> > > It's a lousy state-of-affairs, but as mentioned previously in this thread,
> > > it seems to be one that we will have to live with.
> > >
> >
> > Thanks for chiming in.
> >
> > Why not do something like
> >
> > Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )
> >
> > ?
> >
> > That way, older clients can match on the existing _CID and new clients
> > can match on the spec compliant one.
>
> I'll run this by the Hyper-V guys.  I don't have the ACPI expertise to disagree
> with them when they say they can't change it. :-(
>

Yes, please, even if it makes no difference for this particular patch.

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:47 PM
> 
> On Mon, 28 Feb 2022 at 23:38, Michael Kelley (LINUX)
> <mikelley@microsoft.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:22 PM
> > >
> > > On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
> > > <mikelley@microsoft.com> wrote:
> > > >
> > > > From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022
> > > 1:55 PM
> > > > >
> > > > > Hi Andy,
> > > > >
> > > > > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > My point is that this is clear abuse of the spec and:
> > > > > > 1) we have to enable the broken, because it is already in the wild with
> > > > > >    the comment that this is an issue
> > > > > >
> > > > > > AND
> > > > > >
> > > > > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > > > > >
> > > > > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > > > > 2) while we have 1) on table.
> > > > >
> > > > > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > > > > go for it. I have no idea how to do this myself; Ard said something
> > > > > about joining the UEFI forum as an individual something or another but
> > > > > I don't think I'm the man for the job there. Is this something that
> > > > > Intel can do with their existing membership (is that the right term?)
> > > > > at the UEFI forum? Or maybe a Microsoft engineer on the list?
> > > >
> > > > My team at Microsoft, which works on Linux, filed a bug on this
> > > > issue against the Hyper-V team about a year ago, probably when the issue
> > > > was raised during the previous attempt to implement the functionality
> > > > in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> > > > that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> > > > changing to an identifier that meets the spec is problematic because
> > > > of backwards compatibility with Windows guests on Hyper-V that
> > > > consume the current identifier.  There's no practical way to have Hyper-V
> > > > provide a conformant identifier AND fix all the Windows guests out in
> > > > the wild to consume the new identifier.   As a result, at this point Hyper-V
> > > > is not planning to change anything.
> > > >
> > > > It's a lousy state-of-affairs, but as mentioned previously in this thread,
> > > > it seems to be one that we will have to live with.
> > > >
> > >
> > > Thanks for chiming in.
> > >
> > > Why not do something like
> > >
> > > Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )
> > >
> > > ?
> > >
> > > That way, older clients can match on the existing _CID and new clients
> > > can match on the spec compliant one.
> >
> > I'll run this by the Hyper-V guys.  I don't have the ACPI expertise to disagree
> > with them when they say they can't change it. :-(
> >
> 
> Yes, please, even if it makes no difference for this particular patch.

The Hyper-V guys pass along their thanks for your suggestion.  They have
created an internal build with the change and verified that it preserves
compatibility with Windows guests.  I've tested with Linux guests and
Jason's new driver (modified to look for "VMGENCTR"), and it all looks good.
It will take a little while to wend its way through the Windows/Hyper-V
release system, but they are planning to take the change.

Michael

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Michael,

On 3/22/22, Michael Kelley (LINUX) <mikelley@microsoft.com> wrote:
> From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:47
> PM
>>
>> On Mon, 28 Feb 2022 at 23:38, Michael Kelley (LINUX)
>> <mikelley@microsoft.com> wrote:
>> >
>> > From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022
>> > 2:22 PM
>> > >
>> > > On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
>> > > <mikelley@microsoft.com> wrote:
>> > > >
>> > > > From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February
>> > > > 28, 2022
>> > > 1:55 PM
>> > > > >
>> > > > > Hi Andy,
>> > > > >
>> > > > > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
>> > > > > <andy.shevchenko@gmail.com> wrote:
>> > > > > > My point is that this is clear abuse of the spec and:
>> > > > > > 1) we have to enable the broken, because it is already in the
>> > > > > > wild with
>> > > > > >    the comment that this is an issue
>> > > > > >
>> > > > > > AND
>> > > > > >
>> > > > > > 2) issue an ECR / work with MS to make sure they understand the
>> > > > > > problem.
>> > > > > >
>> > > > > > This can be done in parallel. What I meant as a prerequisite is
>> > > > > > to start doing
>> > > > > > 2) while we have 1) on table.
>> > > > >
>> > > > > Oh, okay, that makes sense. If you want to get (2) going, by all
>> > > > > means
>> > > > > go for it. I have no idea how to do this myself; Ard said
>> > > > > something
>> > > > > about joining the UEFI forum as an individual something or another
>> > > > > but
>> > > > > I don't think I'm the man for the job there. Is this something
>> > > > > that
>> > > > > Intel can do with their existing membership (is that the right
>> > > > > term?)
>> > > > > at the UEFI forum? Or maybe a Microsoft engineer on the list?
>> > > >
>> > > > My team at Microsoft, which works on Linux, filed a bug on this
>> > > > issue against the Hyper-V team about a year ago, probably when the
>> > > > issue
>> > > > was raised during the previous attempt to implement the
>> > > > functionality
>> > > > in Linux.  I've talked with the Hyper-V dev manager, and they
>> > > > acknowledge
>> > > > that the ACPI entry Hyper-V provides to guest VMs violates the spec.
>> > > >  But
>> > > > changing to an identifier that meets the spec is problematic
>> > > > because
>> > > > of backwards compatibility with Windows guests on Hyper-V that
>> > > > consume the current identifier.  There's no practical way to have
>> > > > Hyper-V
>> > > > provide a conformant identifier AND fix all the Windows guests out
>> > > > in
>> > > > the wild to consume the new identifier.   As a result, at this point
>> > > > Hyper-V
>> > > > is not planning to change anything.
>> > > >
>> > > > It's a lousy state-of-affairs, but as mentioned previously in this
>> > > > thread,
>> > > > it seems to be one that we will have to live with.
>> > > >
>> > >
>> > > Thanks for chiming in.
>> > >
>> > > Why not do something like
>> > >
>> > > Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )
>> > >
>> > > ?
>> > >
>> > > That way, older clients can match on the existing _CID and new
>> > > clients
>> > > can match on the spec compliant one.
>> >
>> > I'll run this by the Hyper-V guys.  I don't have the ACPI expertise to
>> > disagree
>> > with them when they say they can't change it. :-(
>> >
>>
>> Yes, please, even if it makes no difference for this particular patch.
>
> The Hyper-V guys pass along their thanks for your suggestion.  They have
> created an internal build with the change and verified that it preserves
> compatibility with Windows guests.  I've tested with Linux guests and
> Jason's new driver (modified to look for "VMGENCTR"), and it all looks
> good.
> It will take a little while to wend its way through the Windows/Hyper-V
> release system, but they are planning to take the change.
>
> Michael
>

Do you want to send a patch against the crng/random.git tree adding that new id?

Jason

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Tuesday, March 22, 2022 1:12 PM
> 
> Hi Michael,
> 
> >
> > The Hyper-V guys pass along their thanks for your suggestion.  They have
> > created an internal build with the change and verified that it preserves
> > compatibility with Windows guests.  I've tested with Linux guests and
> > Jason's new driver (modified to look for "VMGENCTR"), and it all looks
> > good.
> > It will take a little while to wend its way through the Windows/Hyper-V
> > release system, but they are planning to take the change.
> >
> > Michael
> >
> 
> Do you want to send a patch against the crng/random.git tree adding that new id?

I've sent a patch.

Michael

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Tue, 22 Mar 2022 at 20:59, Michael Kelley (LINUX)
<mikelley@microsoft.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:47 PM
> >
> > On Mon, 28 Feb 2022 at 23:38, Michael Kelley (LINUX)
> > <mikelley@microsoft.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org> Sent: Monday, February 28, 2022 2:22 PM
> > > >
> > > > On Mon, 28 Feb 2022 at 23:14, Michael Kelley (LINUX)
> > > > <mikelley@microsoft.com> wrote:
> > > > >
> > > > > From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022
> > > > 1:55 PM
> > > > > >
> > > > > > Hi Andy,
> > > > > >
> > > > > > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > > My point is that this is clear abuse of the spec and:
> > > > > > > 1) we have to enable the broken, because it is already in the wild with
> > > > > > >    the comment that this is an issue
> > > > > > >
> > > > > > > AND
> > > > > > >
> > > > > > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > > > > > >
> > > > > > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > > > > > 2) while we have 1) on table.
> > > > > >
> > > > > > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > > > > > go for it. I have no idea how to do this myself; Ard said something
> > > > > > about joining the UEFI forum as an individual something or another but
> > > > > > I don't think I'm the man for the job there. Is this something that
> > > > > > Intel can do with their existing membership (is that the right term?)
> > > > > > at the UEFI forum? Or maybe a Microsoft engineer on the list?
> > > > >
> > > > > My team at Microsoft, which works on Linux, filed a bug on this
> > > > > issue against the Hyper-V team about a year ago, probably when the issue
> > > > > was raised during the previous attempt to implement the functionality
> > > > > in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> > > > > that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> > > > > changing to an identifier that meets the spec is problematic because
> > > > > of backwards compatibility with Windows guests on Hyper-V that
> > > > > consume the current identifier.  There's no practical way to have Hyper-V
> > > > > provide a conformant identifier AND fix all the Windows guests out in
> > > > > the wild to consume the new identifier.   As a result, at this point Hyper-V
> > > > > is not planning to change anything.
> > > > >
> > > > > It's a lousy state-of-affairs, but as mentioned previously in this thread,
> > > > > it seems to be one that we will have to live with.
> > > > >
> > > >
> > > > Thanks for chiming in.
> > > >
> > > > Why not do something like
> > > >
> > > > Name (_CID, Package (2) { "VM_GEN_COUNTER", "VMGENCTR" } )
> > > >
> > > > ?
> > > >
> > > > That way, older clients can match on the existing _CID and new clients
> > > > can match on the spec compliant one.
> > >
> > > I'll run this by the Hyper-V guys.  I don't have the ACPI expertise to disagree
> > > with them when they say they can't change it. :-(
> > >
> >
> > Yes, please, even if it makes no difference for this particular patch.
>
> The Hyper-V guys pass along their thanks for your suggestion.  They have
> created an internal build with the change and verified that it preserves
> compatibility with Windows guests.  I've tested with Linux guests and
> Jason's new driver (modified to look for "VMGENCTR"), and it all looks good.
> It will take a little while to wend its way through the Windows/Hyper-V
> release system, but they are planning to take the change.
>

Thanks for reporting back.

Will the spec be updated accordingly?

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Ard Biesheuvel <ardb@kernel.org> Sent: Tuesday, March 22, 2022 3:07 PM
> 
> On Tue, 22 Mar 2022 at 20:59, Michael Kelley (LINUX)
> <mikelley@microsoft.com> wrote:
> >
> > The Hyper-V guys pass along their thanks for your suggestion.  They have
> > created an internal build with the change and verified that it preserves
> > compatibility with Windows guests.  I've tested with Linux guests and
> > Jason's new driver (modified to look for "VMGENCTR"), and it all looks good.
> > It will take a little while to wend its way through the Windows/Hyper-V
> > release system, but they are planning to take the change.
> >
> 
> Thanks for reporting back.
> 
> Will the spec be updated accordingly?

The Hyper-V team is looking into updating the spec.  The document
is 10 years old, so they need to find the original source for the PDF.

Michael

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

On 3/24/22, Michael Kelley (LINUX) <mikelley@microsoft.com> wrote:
> From: Ard Biesheuvel <ardb@kernel.org> Sent: Tuesday, March 22, 2022 3:07
> PM
>>
>> On Tue, 22 Mar 2022 at 20:59, Michael Kelley (LINUX)
>> <mikelley@microsoft.com> wrote:
>> >
>> > The Hyper-V guys pass along their thanks for your suggestion.  They
>> > have
>> > created an internal build with the change and verified that it
>> > preserves
>> > compatibility with Windows guests.  I've tested with Linux guests and
>> > Jason's new driver (modified to look for "VMGENCTR"), and it all looks
>> > good.
>> > It will take a little while to wend its way through the Windows/Hyper-V
>> > release system, but they are planning to take the change.
>> >
>>
>> Thanks for reporting back.
>>
>> Will the spec be updated accordingly?
>
> The Hyper-V team is looking into updating the spec.  The document
> is 10 years old, so they need to find the original source for the PDF.
>

Lol, here's the docx:
https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx

RE: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Michael Kelley (LINUX)]

From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Thursday, March 24, 2022 12:45 PM
> 
> On 3/24/22, Michael Kelley (LINUX) <mikelley@microsoft.com> wrote:
> > From: Ard Biesheuvel <ardb@kernel.org> Sent: Tuesday, March 22, 2022 3:07
> > PM
> >>
> >> On Tue, 22 Mar 2022 at 20:59, Michael Kelley (LINUX)
> >> <mikelley@microsoft.com> wrote:
> >> >
> >> > The Hyper-V guys pass along their thanks for your suggestion.  They
> >> > have
> >> > created an internal build with the change and verified that it
> >> > preserves
> >> > compatibility with Windows guests.  I've tested with Linux guests and
> >> > Jason's new driver (modified to look for "VMGENCTR"), and it all looks
> >> > good.
> >> > It will take a little while to wend its way through the Windows/Hyper-V
> >> > release system, but they are planning to take the change.
> >> >
> >>
> >> Thanks for reporting back.
> >>
> >> Will the spec be updated accordingly?
> >
> > The Hyper-V team is looking into updating the spec.  The document
> > is 10 years old, so they need to find the original source for the PDF.
> >
> 
> Lol, here's the docx:
> https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx

Indeed!  My mistake.  I just assumed it was a PDF without even looking at it. :-(
Somebody internally here also just commented that it was weird to have a .docx
file posted for download.

Regardless, that removes one hurdle to making updates!

Michael

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Andy Shevchenko]

On Tue, Mar 1, 2022 at 12:14 AM Michael Kelley (LINUX)
<mikelley@microsoft.com> wrote:
> From: Jason A. Donenfeld <Jason@zx2c4.com> Sent: Monday, February 28, 2022 1:55 PM
> > On Mon, Feb 28, 2022 at 10:28 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > My point is that this is clear abuse of the spec and:
> > > 1) we have to enable the broken, because it is already in the wild with
> > >    the comment that this is an issue
> > >
> > > AND
> > >
> > > 2) issue an ECR / work with MS to make sure they understand the problem.
> > >
> > > This can be done in parallel. What I meant as a prerequisite is to start doing
> > > 2) while we have 1) on table.
> >
> > Oh, okay, that makes sense. If you want to get (2) going, by all means
> > go for it. I have no idea how to do this myself; Ard said something
> > about joining the UEFI forum as an individual something or another but
> > I don't think I'm the man for the job there. Is this something that
> > Intel can do with their existing membership (is that the right term?)
> > at the UEFI forum? Or maybe a Microsoft engineer on the list?
>
> My team at Microsoft, which works on Linux, filed a bug on this
> issue against the Hyper-V team about a year ago, probably when the issue
> was raised during the previous attempt to implement the functionality
> in Linux.  I've talked with the Hyper-V dev manager, and they acknowledge
> that the ACPI entry Hyper-V provides to guest VMs violates the spec.  But
> changing to an identifier that meets the spec is problematic because
> of backwards compatibility with Windows guests on Hyper-V that
> consume the current identifier.  There's no practical way to have Hyper-V
> provide a conformant identifier AND fix all the Windows guests out in
> the wild to consume the new identifier.   As a result, at this point Hyper-V
> is not planning to change anything.
>
> It's a lousy state-of-affairs, but as mentioned previously in this thread,
> it seems to be one that we will have to live with.

Yes, my point of 2) is targeting the following:
a) MS should be notified
b) MS must try very hard to avoid similar problems in the future, they
very well may discuss the matters in ASWG with other companies
c) the spec will be fixed for the future versions, while the current
one will live for the backward compatibility only

Frankly I'm a bit frustrated that it's not the first time MS violates
the ACPI spec, while being a member of ASWG.

-- 
With Best Regards,
Andy Shevchenko

Re: Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Alexander Graf]

Hi Andy,

On 28.02.22 22:27, Andy Shevchenko wrote:
> On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
>> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>> From: Alexander Graf <graf@amazon.com>
>>>>
>>>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
>>>> entries of the respective devices. However, when making structs for
>>>> matching, we squeeze those IDs into acpi_device_id, which only has 9
>>>> bytes space to store the identifier. The subsystem actually captures the
>>>> full length of the IDs, and the modalias has the full length, but this
>>>> struct we use for matching is limited. It originally had 16 bytes, but
>>>> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
>>>> modalias generation safe for cross compiling"), presumably on the theory
>>>> that it would match the ACPI spec so it didn't matter.
>>>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
>>>> decided that its VM Generation Counter device [1] should only be
>>>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
>>>> than 9 characters.
>>> Then why do we not see the ECR from somebody to update the spec or to
>>> fix MS' abuse of it?
>>> I believe _this_ should be the prerequisite to the proposed change.
>> What exactly are you suggesting here? That the contributor of this
>> patch joins the UEFI forum as an individual adopter in order to get
>> the ACPI spec updated before we can advance with this patch? Or that
>> he works with Microsoft to get them to refrain from violating it?
>>
>> I don't think that is reasonable or realistic. The kernel is already
>> riddled with UEFI and ACPI quirks that are only there because some
>> teams at MS don't take the ACPI spec too literally (which is why they
>> have their own AML compiler, for one), and PC vendors only care about
>> the Windows sticker, so they don't care about the ACPI spec either.
>>
>> So I don't think this is the right time to get pedantic about this.
>> Our ACPI subsystem already deals with CIDs that are longer than 8
>> characters (which are btw permitted by the ACPI spec for bus topology
>> related metadata), the only thing being changed here is the ability to
>> actually match against such identifiers.
> My point is that this is clear abuse of the spec and:
> 1) we have to enable the broken, because it is already in the wild with
>     the comment that this is an issue
>
> AND
>
> 2) issue an ECR / work with MS to make sure they understand the problem.
>
> This can be done in parallel. What I meant as a prerequisite is to start doing
> 2) while we have 1) on table.


While trying to revalidate whether this really is breaking the spec, 
I've tried to reread the respective section in it and I'm afraid that it 
may be valid use of the _CID identifier:


"""

6.1.2 _CID (Compatible ID)

This optional object is used to supply OSPM with a device’s Plug and 
Play-Compatible Device ID. Use _CID objects when a device has no other 
defined hardware standard method to report its compatible IDs. The _CID 
object is valid only within a Full Device Descriptor. An _HID object 
must also be present.

Arguments:

None

Return Value:
An Integer or String containing a single CID or a Package containing a 
list of CIDs A _CID object evaluates to either:

  *

    A single Compatible Device ID

  *

    A package of Compatible Device IDs for the device – in the order of
    preference, highest preference first.

Each Compatible Device ID must be either:

  *

    A valid HID value (a 32-bit compressed EISA type ID or a string such
    as “ACPI0004”).

  *

    A string that uses a bus-specific nomenclature. For example, _CID
    can be used to specify the PCI ID. The format of a PCI ID string is
    one of the following:

"PCI\CC_ccss" "PCI\CC_ccsspp" 
"PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss&REV_rr" 
"PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss" "PCI\VEN_vvvv&DEV_dddd&REV_rr" 
"PCI\VEN_vvvv&DEV_dddd"

"""

In this case, you could interpret things as looking at "bus-specific 
nomenclature" case which even in the examples mentioned in the spec 
exceeds the 8 character limit we impose on the matching logic today.

There still is spec violation in Hyper-V's VMGenID device's _HID value 
which doesn't follow the PNP format, but that's not relevant here. _CID 
doesn't seem to have the same restrictions?


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: Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Ard Biesheuvel]

On Mon, 28 Feb 2022 at 23:01, Alexander Graf <graf@amazon.com> wrote:
>
> Hi Andy,
>
> On 28.02.22 22:27, Andy Shevchenko wrote:
> > On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
> >> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>> On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>> From: Alexander Graf <graf@amazon.com>
> >>>>
> >>>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> >>>> entries of the respective devices. However, when making structs for
> >>>> matching, we squeeze those IDs into acpi_device_id, which only has 9
> >>>> bytes space to store the identifier. The subsystem actually captures the
> >>>> full length of the IDs, and the modalias has the full length, but this
> >>>> struct we use for matching is limited. It originally had 16 bytes, but
> >>>> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> >>>> modalias generation safe for cross compiling"), presumably on the theory
> >>>> that it would match the ACPI spec so it didn't matter.
> >>>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> >>>> decided that its VM Generation Counter device [1] should only be
> >>>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> >>>> than 9 characters.
> >>> Then why do we not see the ECR from somebody to update the spec or to
> >>> fix MS' abuse of it?
> >>> I believe _this_ should be the prerequisite to the proposed change.
> >> What exactly are you suggesting here? That the contributor of this
> >> patch joins the UEFI forum as an individual adopter in order to get
> >> the ACPI spec updated before we can advance with this patch? Or that
> >> he works with Microsoft to get them to refrain from violating it?
> >>
> >> I don't think that is reasonable or realistic. The kernel is already
> >> riddled with UEFI and ACPI quirks that are only there because some
> >> teams at MS don't take the ACPI spec too literally (which is why they
> >> have their own AML compiler, for one), and PC vendors only care about
> >> the Windows sticker, so they don't care about the ACPI spec either.
> >>
> >> So I don't think this is the right time to get pedantic about this.
> >> Our ACPI subsystem already deals with CIDs that are longer than 8
> >> characters (which are btw permitted by the ACPI spec for bus topology
> >> related metadata), the only thing being changed here is the ability to
> >> actually match against such identifiers.
> > My point is that this is clear abuse of the spec and:
> > 1) we have to enable the broken, because it is already in the wild with
> >     the comment that this is an issue
> >
> > AND
> >
> > 2) issue an ECR / work with MS to make sure they understand the problem.
> >
> > This can be done in parallel. What I meant as a prerequisite is to start doing
> > 2) while we have 1) on table.
>
>
> While trying to revalidate whether this really is breaking the spec,
> I've tried to reread the respective section in it and I'm afraid that it
> may be valid use of the _CID identifier:
>
>
> """
>
> 6.1.2 _CID (Compatible ID)
>
> This optional object is used to supply OSPM with a device’s Plug and
> Play-Compatible Device ID. Use _CID objects when a device has no other
> defined hardware standard method to report its compatible IDs. The _CID
> object is valid only within a Full Device Descriptor. An _HID object
> must also be present.
>
> Arguments:
>
> None
>
> Return Value:
> An Integer or String containing a single CID or a Package containing a
> list of CIDs A _CID object evaluates to either:
>
>   *
>
>     A single Compatible Device ID
>
>   *
>
>     A package of Compatible Device IDs for the device – in the order of
>     preference, highest preference first.
>
> Each Compatible Device ID must be either:
>
>   *
>
>     A valid HID value (a 32-bit compressed EISA type ID or a string such
>     as “ACPI0004”).
>
>   *
>
>     A string that uses a bus-specific nomenclature. For example, _CID
>     can be used to specify the PCI ID. The format of a PCI ID string is
>     one of the following:
>
> "PCI\CC_ccss" "PCI\CC_ccsspp"
> "PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss&REV_rr"
> "PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss" "PCI\VEN_vvvv&DEV_dddd&REV_rr"
> "PCI\VEN_vvvv&DEV_dddd"
>
> """
>
> In this case, you could interpret things as looking at "bus-specific
> nomenclature" case which even in the examples mentioned in the spec
> exceeds the 8 character limit we impose on the matching logic today.
>

This is what I was referring to when I mentioned bus topology related metadata.

This is why those uses of ACPI_ID_LEN outside of struct acpi_device_id
may potentially be dangerous, given that _CIDs are apparently
effectively unbounded in size. But relying on this to justify the
"VM_GEN_COUNTER" CID is a bit of a stretch, IMO.

> There still is spec violation in Hyper-V's VMGenID device's _HID value
> which doesn't follow the PNP format, but that's not relevant here. _CID
> doesn't seem to have the same restrictions?
>

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Hans de Goede]

Hi,

On 2/28/22 19:33, Jason A. Donenfeld wrote:
> From: Alexander Graf <graf@amazon.com>
> 
> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> entries of the respective devices. However, when making structs for
> matching, we squeeze those IDs into acpi_device_id, which only has 9
> bytes space to store the identifier. The subsystem actually captures the
> full length of the IDs, and the modalias has the full length, but this
> struct we use for matching is limited. It originally had 16 bytes, but
> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> modalias generation safe for cross compiling"), presumably on the theory
> that it would match the ACPI spec so it didn't matter.
> 
> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> decided that its VM Generation Counter device [1] should only be
> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> than 9 characters.
> 
> To allow device drivers to match identifiers that exceed the 9 byte
> limit, this simply ups the length to 16, just like it was before the
> aforementioned commit. Empirical testing indicates that this
> doesn't actually increase vmlinux size on 64-bit, because the ulong in
> the same struct caused there to be 7 bytes of padding anyway, and when
> doing a s/M/Y/g i386_defconfig build, the bzImage only increased by
> 0.0055%, so negligible.
> 
> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
> 
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
> 
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> [Jason: reworked commit message a bit, went with len=16 approach.]
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

The need for this is a bit meh, but I have no objections against
solving it this way:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
> Hi Rafael,
> 
> This patch is directed toward you specifically. The first and last patch
> of the series this is part of have been through the ringer of review a
> bit already and do not specifically require your attention, but we wound
> up getting hung up on an ACPI ID matching API limitation. This patch
> fixes that limitation with this patch that you see here, with a trivial
> one line fix, which does require your attention.
> 
> The other patches will go through my random.git tree naturally, but
> because those patches depend on this one here, in order to compile
> without warnings (and be functional at all), it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
> 
> This v6 updates the commit message.
> 
> Please have a look and let me know what you think.
> 
> Thanks,
> Jason
> 
>  include/linux/mod_devicetable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> -#define ACPI_ID_LEN	9
> +#define ACPI_ID_LEN	16
>  
>  struct acpi_device_id {
>  	__u8 id[ACPI_ID_LEN];

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

Hi Hans,

On Tue, Mar 1, 2022 at 11:35 AM Hans de Goede <hdegoede@redhat.com> wrote:
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks for the Ack. I still need Rafael's Ack to take this through my
random.git tree, right? Or are you two one in the same when it comes
to that? Trying not to step on toes if possible.

Jason

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Hans de Goede]

Hi,

On 3/1/22 11:38, Jason A. Donenfeld wrote:
> Hi Hans,
> 
> On Tue, Mar 1, 2022 at 11:35 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks for the Ack. I still need Rafael's Ack to take this through my
> random.git tree, right?

Right.

> Or are you two one in the same when it comes
> to that? Trying not to step on toes if possible.

No, I'm the drivers/platform/x86 subsys maintainer and as such do a lot
with ACPI too, but Rafael is the ACPI subsys maintainer.

Regards,

Hans

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Andy Shevchenko]

On Tue, Mar 01, 2022 at 11:35:12AM +0100, Hans de Goede wrote:
> On 2/28/22 19:33, Jason A. Donenfeld wrote:

> > Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>

Official tag is Co-developed-by

> > [Jason: reworked commit message a bit, went with len=16 approach.]
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Jason A. Donenfeld]

On Tue, Mar 1, 2022 at 3:26 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Mar 01, 2022 at 11:35:12AM +0100, Hans de Goede wrote:
> > On 2/28/22 19:33, Jason A. Donenfeld wrote:
>
> > > Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Official tag is Co-developed-by

Thanks, will adjust.

Re: [PATCH 2/3 v6] ACPI: allow longer device IDs

[Rafael J. Wysocki]

On Mon, Feb 28, 2022 at 7:34 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> From: Alexander Graf <graf@amazon.com>
>
> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> entries of the respective devices. However, when making structs for
> matching, we squeeze those IDs into acpi_device_id, which only has 9
> bytes space to store the identifier. The subsystem actually captures the
> full length of the IDs, and the modalias has the full length, but this
> struct we use for matching is limited. It originally had 16 bytes, but
> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> modalias generation safe for cross compiling"), presumably on the theory
> that it would match the ACPI spec so it didn't matter.
>
> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> decided that its VM Generation Counter device [1] should only be
> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> than 9 characters.
>
> To allow device drivers to match identifiers that exceed the 9 byte
> limit, this simply ups the length to 16, just like it was before the
> aforementioned commit. Empirical testing indicates that this
> doesn't actually increase vmlinux size on 64-bit, because the ulong in
> the same struct caused there to be 7 bytes of padding anyway, and when
> doing a s/M/Y/g i386_defconfig build, the bzImage only increased by
> 0.0055%, so negligible.
>
> This patch is a prerequisite to add support for VMGenID in Linux, the
> subsequent patch in this series. It has been confirmed to also work on
> the udev/modalias side in userspace.
>
> [1] https://download.microsoft.com/download/3/1/C/31CFC307-98CA-4CA5-914C-D9772691E214/VirtualMachineGenerationID.docx
>
> Signed-off-by: Alexander Graf <graf@amazon.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Co-authored-by: Jason A. Donenfeld <Jason@zx2c4.com>
> [Jason: reworked commit message a bit, went with len=16 approach.]
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Hi Rafael,
>
> This patch is directed toward you specifically. The first and last patch
> of the series this is part of have been through the ringer of review a
> bit already and do not specifically require your attention, but we wound
> up getting hung up on an ACPI ID matching API limitation. This patch
> fixes that limitation with this patch that you see here, with a trivial
> one line fix, which does require your attention.
>
> The other patches will go through my random.git tree naturally, but
> because those patches depend on this one here, in order to compile
> without warnings (and be functional at all), it would be nice if you
> would provide an "Acked-by" on it and permit me to /also/ take it
> through my random.git tree (if it looks like a correct patch to you, of
> course). This would make the merge logistics a lot easier. Plus it's a
> small +1/-1 line change.
>
> This v6 updates the commit message.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> Please have a look and let me know what you think.
>
> Thanks,
> Jason
>
>  include/linux/mod_devicetable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 4bb71979a8fd..5da5d990ff58 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -211,7 +211,7 @@ struct css_device_id {
>         kernel_ulong_t driver_data;
>  };
>
> -#define ACPI_ID_LEN    9
> +#define ACPI_ID_LEN    16
>
>  struct acpi_device_id {
>         __u8 id[ACPI_ID_LEN];
> --
> 2.35.1
>

Re: [PATCH v5 2/3] ACPI: allow longer device IDs

[Hans de Goede]

Hi,

On 2/28/22 19:19, Rafael J. Wysocki wrote:
> +Mika, Andy and Hans in case they have something to add

Thanks, I don't really have anything to add to the discussion
on v6 of this patch.

Regards,

Hans



> 
> On Mon, Feb 28, 2022 at 12:27 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> Hey again,
>>
>> On Sun, Feb 27, 2022 at 1:43 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>
>>> Hi Alex,
>>>
>>> On Sun, Feb 27, 2022 at 12:42:03PM +0100, Alexander Graf wrote:
>>>>> To allow device drivers to match identifiers that exceed the 9 byte
>>>>> limit, this simply ups the length to 16, just like it was before the
>>>>> aforementioned commit. Empirical testing indicates that this
>>>>
>>>>
>>>> This is only true for 64bit systems where padding automatically bloated
>>>> to 9 byte array to 16. I still believe the patch is fine as it is, but
>>>> there will be minor .rodata overhead on 32bit targets which you may want
>>>> to quantify in the patch description.
>>>
>>> Good point. So I just tried this out with a 32-bit i686 kernel and the
>>> results are the same again for the size of vmlinux. I then ran `objdump
>>> --headers` and looked at the size of the .rodata section, where it's
>>> also the same. I'm not quite sure what to make of this, as it's not what
>>> I was expecting, but I think I tested it right. So maybe we're lucky
>>> here?
>>
>> I tried a little harder to get _some_ difference on 32-bit, and
>> managed to get one by doing i386_defconfig and then switching off
>> modules to make all M into Y, and then compared sizes:
>>
>> vmlinux: 25590780 -> 25598972, so a 0.032% increase.
>> bzImage: 8698944 -> 8699424, so a 0.0055% increase.
>>
>> So it does increase, ever so slightly, but a) on 32-bit, and b) a
>> super, super tiny amount.
>>
>> In other words, I still think this patch is very much a-okay. But very
>> eager to hear from Rafael on the approach.
> 
> Increasing the ACPI_ID_LEN value is fine with me, but the patch
> changelog is not entirely accurate.
> 
> The ACPI subsystem uses struct acpi_device_id mostly (if not only) for
> device ID matching and it is generally used for creating lists of ACPI
> device IDs in drivers (and allow/deny lists etc).  The device IDs
> extracted from the ACPI tables can be longer than ACPI_ID_LEN.
> 
> This means that drivers cannot match device IDs longer than 8
> characters (excluding the terminating 0), because the IDs in the lists
> used by them for ID matching cannot be longer than this and not
> because the ACPI subsystem is limited by that value.
> 

[PATCH v5 3/3] 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: Alexander Graf <graf@amazon.com>
Cc: Adrian Catangiu <adrian@parity.io>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v4->v5:
- [Greg] Use module_acpi_driver instead of writing my own code.
- [Alex] Match on _CID instead of _HID.
- Prefer Y over M but still allow M, to handle initramfs reseeds.
- [Wei] Use IS_ERR instead of NULL check with devm_memremap.

 MAINTAINERS            |   1 +
 drivers/virt/Kconfig   |  11 +++++
 drivers/virt/Makefile  |   1 +
 drivers/virt/vmgenid.c | 100 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 113 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..121b9293c737 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -13,6 +13,17 @@ 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.
+
+	  Prefer Y to M so that this protection is activated very early.
+
 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..0ae1a39f2e28
--- /dev/null
+++ b/drivers/virt/vmgenid.c
@@ -0,0 +1,100 @@
+// 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_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 (IS_ERR(state->next_id)) {
+		ret = PTR_ERR(state->next_id);
+		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_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[] = {
+	{ "VM_GEN_COUNTER", 0 },
+	{ }
+};
+
+static struct acpi_driver vmgenid_driver = {
+	.name = "vmgenid",
+	.ids = vmgenid_ids,
+	.owner = THIS_MODULE,
+	.ops = {
+		.add = vmgenid_add,
+		.notify = vmgenid_notify
+	}
+};
+
+module_acpi_driver(vmgenid_driver);
+
+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 v5 3/3] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Michael Kelley (LINUX)]

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

From: Souradeep Chakrabarti <souradch.linux@gmail.com>

Jason A. Donenfeld <Jason@zx2c4.com> Sent: Saturday, February 26, 2022 2:07 PM
> 
> 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: Alexander Graf <graf@amazon.com>
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Wei Yongjun <weiyongjun1@huawei.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

The patch has been successfully verified in Hyper-V. I have used checkpoint
to create snapshot and restarted the VM multiple times from the same snapshot
by applying it each time. To confirm the unique VM generation id, printk has
been used to print the value each time.

Tested-by: Souradeep Chakrabarti <souradch.linux@gmail.com>

> ---
> Changes v4->v5:
> - [Greg] Use module_acpi_driver instead of writing my own code.
> - [Alex] Match on _CID instead of _HID.
> - Prefer Y over M but still allow M, to handle initramfs reseeds.
> - [Wei] Use IS_ERR instead of NULL check with devm_memremap.
> 
>  MAINTAINERS            |   1 +
>  drivers/virt/Kconfig   |  11 +++++
>  drivers/virt/Makefile  |   1 +
>  drivers/virt/vmgenid.c | 100 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 113 insertions(+)
>  create mode 100644 drivers/virt/vmgenid.c
> 

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 18737 bytes --]

RE: [PATCH v5 3/3] virt: vmgenid: introduce driver for reinitializing RNG on VM fork

[Michael Kelley (LINUX)]

From: Souradeep Chakrabarti <souradch.linux@gmail.com>

Jason A. Donenfeld <Jason@zx2c4.com> Sent: Saturday, February 26, 2022 2:07 PM
> 
> 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: Alexander Graf <graf@amazon.com>
> Cc: Adrian Catangiu <adrian@parity.io>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Wei Yongjun <weiyongjun1@huawei.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

The patch has been successfully verified in Hyper-V. I have used checkpoint
to create snapshot and restarted the VM multiple times from the same snapshot
by applying it each time. To confirm the unique VM generation id, printk has
been used to print the value each time.

Tested-by: Souradeep Chakrabarti <souradch.linux@gmail.com>

> ---
> Changes v4->v5:
> - [Greg] Use module_acpi_driver instead of writing my own code.
> - [Alex] Match on _CID instead of _HID.
> - Prefer Y over M but still allow M, to handle initramfs reseeds.
> - [Wei] Use IS_ERR instead of NULL check with devm_memremap.
> 
>  MAINTAINERS            |   1 +
>  drivers/virt/Kconfig   |  11 +++++
>  drivers/virt/Makefile  |   1 +
>  drivers/virt/vmgenid.c | 100 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 113 insertions(+)
>  create mode 100644 drivers/virt/vmgenid.c
>