All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/arm/virt: dt: add rng-seed property
@ 2022-06-27 16:07 Jason A. Donenfeld
  2022-06-27 16:12 ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-27 16:07 UTC (permalink / raw)
  To: peter.maydell, qemu-devel; +Cc: Jason A. Donenfeld

In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
kaslr-seed property was added, but the equally as important rng-seed
property was forgotten about, which has identical semantics for a
similar purpose. This commit implements it in exactly the same way as
kaslr-seed.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 hw/arm/virt.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 2 files changed, 41 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 097238faa7..8a3436a370 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -221,6 +221,16 @@ static bool cpu_type_valid(const char *cpu)
     return false;
 }
 
+static void create_rng_seed(MachineState *ms, const char *node)
+{
+    uint8_t seed[32];
+
+    if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
+        return;
+    }
+    qemu_fdt_setprop(ms->fdt, node, "rng-seed", seed, sizeof(seed));
+}
+
 static void create_kaslr_seed(MachineState *ms, const char *node)
 {
     uint64_t seed;
@@ -251,6 +261,9 @@ static void create_fdt(VirtMachineState *vms)
 
     /* /chosen must exist for load_dtb to fill in necessary properties later */
     qemu_fdt_add_subnode(fdt, "/chosen");
+    if (vms->dtb_rng_seed) {
+        create_rng_seed(ms, "/chosen");
+    }
     if (vms->dtb_kaslr_seed) {
         create_kaslr_seed(ms, "/chosen");
     }
@@ -260,6 +273,9 @@ static void create_fdt(VirtMachineState *vms)
         if (vms->dtb_kaslr_seed) {
             create_kaslr_seed(ms, "/secure-chosen");
         }
+        if (vms->dtb_rng_seed) {
+            create_rng_seed(ms, "/secure-chosen");
+        }
     }
 
     /* Clock node, for the benefit of the UART. The kernel device tree
@@ -2348,6 +2364,20 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
     vms->its = value;
 }
 
+static bool virt_get_dtb_rng_seed(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->dtb_rng_seed;
+}
+
+static void virt_set_dtb_rng_seed(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->dtb_rng_seed = value;
+}
+
 static bool virt_get_dtb_kaslr_seed(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -2988,6 +3018,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                                           "Set on/off to enable/disable "
                                           "ITS instantiation");
 
+    object_class_property_add_bool(oc, "dtb-rng-seed",
+                                   virt_get_dtb_rng_seed,
+                                   virt_set_dtb_rng_seed);
+    object_class_property_set_description(oc, "dtb-rng-seed",
+                                          "Set off to disable passing of rng-seed "
+                                          "dtb node to guest");
+
     object_class_property_add_bool(oc, "dtb-kaslr-seed",
                                    virt_get_dtb_kaslr_seed,
                                    virt_set_dtb_kaslr_seed);
@@ -3061,6 +3098,9 @@ static void virt_instance_init(Object *obj)
     /* MTE is disabled by default.  */
     vms->mte = false;
 
+    /* Supply a rng-seed by default */
+    vms->dtb_rng_seed = true;
+
     /* Supply a kaslr-seed by default */
     vms->dtb_kaslr_seed = true;
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 15feabac63..cf652f1f3d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -152,6 +152,7 @@ struct VirtMachineState {
     bool virt;
     bool ras;
     bool mte;
+    bool dtb_rng_seed;
     bool dtb_kaslr_seed;
     OnOffAuto acpi;
     VirtGICType gic_version;
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-27 16:07 [PATCH] hw/arm/virt: dt: add rng-seed property Jason A. Donenfeld
@ 2022-06-27 16:12 ` Peter Maydell
  2022-06-27 16:36   ` Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-06-27 16:12 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: qemu-devel

On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
> kaslr-seed property was added, but the equally as important rng-seed
> property was forgotten about, which has identical semantics for a
> similar purpose. This commit implements it in exactly the same way as
> kaslr-seed.

Not an objection, since if this is what the dtb spec says we need
to provide then I guess we need to provide it, but:
Why do we need to give the kernel two separate random seeds?
Isn't one sufficient for the kernel to seed its RNG and generate
whatever randomness it needs for whatever purposes it wants it?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-27 16:12 ` Peter Maydell
@ 2022-06-27 16:36   ` Jason A. Donenfeld
  2022-06-28 18:45     ` Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-27 16:36 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
>> kaslr-seed property was added, but the equally as important rng-seed
>> property was forgotten about, which has identical semantics for a
>> similar purpose. This commit implements it in exactly the same way as
>> kaslr-seed.
>
> Not an objection, since if this is what the dtb spec says we need
> to provide then I guess we need to provide it, but:
> Why do we need to give the kernel two separate random seeds?
> Isn't one sufficient for the kernel to seed its RNG and generate
> whatever randomness it needs for whatever purposes it wants it?
>

Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
After the kernel calls add_bootloader_randomness() on it,
get_random_long() can be used for kaslr'ing and everything else too.
So I'm not sure what's up, but here we are. Maybe down the line I'll
look into the details and formulate a plan to remove `kaslr-seed` if
my supposition is correct.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-27 16:36   ` Jason A. Donenfeld
@ 2022-06-28 18:45     ` Jason A. Donenfeld
  2022-06-29  9:37       ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-28 18:45 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>
>>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
>>> kaslr-seed property was added, but the equally as important rng-seed
>>> property was forgotten about, which has identical semantics for a
>>> similar purpose. This commit implements it in exactly the same way as
>>> kaslr-seed.
>>
>> Not an objection, since if this is what the dtb spec says we need
>> to provide then I guess we need to provide it, but:
>> Why do we need to give the kernel two separate random seeds?
>> Isn't one sufficient for the kernel to seed its RNG and generate
>> whatever randomness it needs for whatever purposes it wants it?
>>
>
> Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
> After the kernel calls add_bootloader_randomness() on it,
> get_random_long() can be used for kaslr'ing and everything else too.
> So I'm not sure what's up, but here we are. Maybe down the line I'll
> look into the details and formulate a plan to remove `kaslr-seed` if
> my supposition is correct.
>
> Jason
>

Was wondering if you planned to queue this up?

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-28 18:45     ` Jason A. Donenfeld
@ 2022-06-29  9:37       ` Peter Maydell
  2022-06-29 10:15         ` Alex Bennée
  2022-06-29 10:18         ` Alex Bennée
  0 siblings, 2 replies; 15+ messages in thread
From: Peter Maydell @ 2022-06-29  9:37 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: qemu-devel

On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>
> >>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
> >>> kaslr-seed property was added, but the equally as important rng-seed
> >>> property was forgotten about, which has identical semantics for a
> >>> similar purpose. This commit implements it in exactly the same way as
> >>> kaslr-seed.
> >>
> >> Not an objection, since if this is what the dtb spec says we need
> >> to provide then I guess we need to provide it, but:
> >> Why do we need to give the kernel two separate random seeds?
> >> Isn't one sufficient for the kernel to seed its RNG and generate
> >> whatever randomness it needs for whatever purposes it wants it?
> >>
> >
> > Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
> > After the kernel calls add_bootloader_randomness() on it,
> > get_random_long() can be used for kaslr'ing and everything else too.
> > So I'm not sure what's up, but here we are. Maybe down the line I'll
> > look into the details and formulate a plan to remove `kaslr-seed` if
> > my supposition is correct.
> >
> > Jason
> >
>
> Was wondering if you planned to queue this up?

It's on my todo list to review...

-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29  9:37       ` Peter Maydell
@ 2022-06-29 10:15         ` Alex Bennée
  2022-06-29 10:18         ` Alex Bennée
  1 sibling, 0 replies; 15+ messages in thread
From: Alex Bennée @ 2022-06-29 10:15 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Jason A. Donenfeld, qemu-devel


Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >>>
>> >>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
>> >>> kaslr-seed property was added, but the equally as important rng-seed
>> >>> property was forgotten about, which has identical semantics for a
>> >>> similar purpose. This commit implements it in exactly the same way as
>> >>> kaslr-seed.
>> >>
>> >> Not an objection, since if this is what the dtb spec says we need
>> >> to provide then I guess we need to provide it, but:
>> >> Why do we need to give the kernel two separate random seeds?
>> >> Isn't one sufficient for the kernel to seed its RNG and generate
>> >> whatever randomness it needs for whatever purposes it wants it?
>> >>
>> >
>> > Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
>> > After the kernel calls add_bootloader_randomness() on it,
>> > get_random_long() can be used for kaslr'ing and everything else too.
>> > So I'm not sure what's up, but here we are. Maybe down the line I'll
>> > look into the details and formulate a plan to remove `kaslr-seed` if
>> > my supposition is correct.
>> >
>> > Jason
>> >
>>
>> Was wondering if you planned to queue this up?
>
> It's on my todo list to review...

Erm isn't this replicating kaslr-seed?

  static void create_kaslr_seed(MachineState *ms, const char *node)
  {
      uint64_t seed;

      if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
          return;
      }
      qemu_fdt_setprop_u64(ms->fdt, node, "kaslr-seed", seed);
  }


which BTW we provide a knob (dtb-kaslr-seed) to suppress because it can
get in the way of secure instrumentation/checksums done by secure boot.

>
> -- PMM


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29  9:37       ` Peter Maydell
  2022-06-29 10:15         ` Alex Bennée
@ 2022-06-29 10:18         ` Alex Bennée
  2022-06-29 11:26           ` Jason A. Donenfeld
  1 sibling, 1 reply; 15+ messages in thread
From: Alex Bennée @ 2022-06-29 10:18 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Jason A. Donenfeld, qemu-devel


Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >>>
>> >>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
>> >>> kaslr-seed property was added, but the equally as important rng-seed
>> >>> property was forgotten about, which has identical semantics for a
>> >>> similar purpose. This commit implements it in exactly the same way as
>> >>> kaslr-seed.
>> >>
>> >> Not an objection, since if this is what the dtb spec says we need
>> >> to provide then I guess we need to provide it, but:
>> >> Why do we need to give the kernel two separate random seeds?
>> >> Isn't one sufficient for the kernel to seed its RNG and generate
>> >> whatever randomness it needs for whatever purposes it wants it?
>> >>
>> >
>> > Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
>> > After the kernel calls add_bootloader_randomness() on it,
>> > get_random_long() can be used for kaslr'ing and everything else too.
>> > So I'm not sure what's up, but here we are. Maybe down the line I'll
>> > look into the details and formulate a plan to remove `kaslr-seed` if
>> > my supposition is correct.

Sorry now I've had my coffee and read properly I see you are already
aware of kaslr-seed. However my point about suppression would still
stand because for the secure boot flow you need checksum-able DTBs.

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29 10:18         ` Alex Bennée
@ 2022-06-29 11:26           ` Jason A. Donenfeld
  2022-06-29 15:24             ` Alex Bennée
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-29 11:26 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Peter Maydell, qemu-devel

On Wed, Jun 29, 2022 at 11:18:23AM +0100, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>
> >> On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> > On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> >>>
> >> >>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
> >> >>> kaslr-seed property was added, but the equally as important rng-seed
> >> >>> property was forgotten about, which has identical semantics for a
> >> >>> similar purpose. This commit implements it in exactly the same way as
> >> >>> kaslr-seed.
> >> >>
> >> >> Not an objection, since if this is what the dtb spec says we need
> >> >> to provide then I guess we need to provide it, but:
> >> >> Why do we need to give the kernel two separate random seeds?
> >> >> Isn't one sufficient for the kernel to seed its RNG and generate
> >> >> whatever randomness it needs for whatever purposes it wants it?
> >> >>
> >> >
> >> > Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
> >> > After the kernel calls add_bootloader_randomness() on it,
> >> > get_random_long() can be used for kaslr'ing and everything else too.
> >> > So I'm not sure what's up, but here we are. Maybe down the line I'll
> >> > look into the details and formulate a plan to remove `kaslr-seed` if
> >> > my supposition is correct.
> 
> Sorry now I've had my coffee and read properly I see you are already
> aware of kaslr-seed. However my point about suppression would still
> stand because for the secure boot flow you need checksum-able DTBs.

Please read the patch. Maybe take a sip of coffee first. There's a knob
for this too.

The code is exactly the same for kaslr-seed and rng-seed. Everytime
there's some kaslr-seed thing, there is now the same rng-seed thing.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29 11:26           ` Jason A. Donenfeld
@ 2022-06-29 15:24             ` Alex Bennée
  2022-06-29 15:55               ` Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Alex Bennée @ 2022-06-29 15:24 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Peter Maydell, qemu-devel


"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> On Wed, Jun 29, 2022 at 11:18:23AM +0100, Alex Bennée wrote:
>> 
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>> > On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >>
>> >> On 6/27/22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >> > On 6/27/22, Peter Maydell <peter.maydell@linaro.org> wrote:
>> >> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> >> >>>
>> >> >>> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
>> >> >>> kaslr-seed property was added, but the equally as important rng-seed
>> >> >>> property was forgotten about, which has identical semantics for a
>> >> >>> similar purpose. This commit implements it in exactly the same way as
>> >> >>> kaslr-seed.
>> >> >>
>> >> >> Not an objection, since if this is what the dtb spec says we need
>> >> >> to provide then I guess we need to provide it, but:
>> >> >> Why do we need to give the kernel two separate random seeds?
>> >> >> Isn't one sufficient for the kernel to seed its RNG and generate
>> >> >> whatever randomness it needs for whatever purposes it wants it?
>> >> >>
>> >> >
>> >> > Seems a bit silly to me too. `rng-seed` alone ought to be sufficient.
>> >> > After the kernel calls add_bootloader_randomness() on it,
>> >> > get_random_long() can be used for kaslr'ing and everything else too.
>> >> > So I'm not sure what's up, but here we are. Maybe down the line I'll
>> >> > look into the details and formulate a plan to remove `kaslr-seed` if
>> >> > my supposition is correct.
>> 
>> Sorry now I've had my coffee and read properly I see you are already
>> aware of kaslr-seed. However my point about suppression would still
>> stand because for the secure boot flow you need checksum-able DTBs.
>
> Please read the patch. Maybe take a sip of coffee first. There's a knob
> for this too.

I was obviously not paying enough attention this morning. Sorry about that.

> The code is exactly the same for kaslr-seed and rng-seed. Everytime
> there's some kaslr-seed thing, there is now the same rng-seed thing.

The duplication is annoying but specs are specs - where is this written
by the way?

Given the use case for the dtb-kaslr-seed knob I wonder if we should
have a common property and deprecate the kaslr one? As of this patch
existing workflows will break until command lines are updated to suppress
the second source of randomness.

Maybe it would be better to have a single a new property
(dtb-rng-seeds?) which suppresses both dtb entries and make
dtb-kaslr-seed an alias and mark it as deprecated.

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29 15:24             ` Alex Bennée
@ 2022-06-29 15:55               ` Jason A. Donenfeld
  2022-06-30  9:15                 ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-29 15:55 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Peter Maydell, qemu-devel

Hi Alex,

On Wed, Jun 29, 2022 at 04:24:20PM +0100, Alex Bennée wrote:
> > The code is exactly the same for kaslr-seed and rng-seed. Everytime
> > there's some kaslr-seed thing, there is now the same rng-seed thing.
> 
> The duplication is annoying but specs are specs - where is this written
> by the way?

The same place as all the ordinary specs:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml

> Given the use case for the dtb-kaslr-seed knob I wonder if we should
> have a common property and deprecate the kaslr one? As of this patch
> existing workflows will break until command lines are updated to suppress
> the second source of randomness.
> 
> Maybe it would be better to have a single a new property
> (dtb-rng-seeds?) which suppresses both dtb entries and make
> dtb-kaslr-seed an alias and mark it as deprecated.

No, I don't think so. If anything, I'll try to get rid of kaslr-seed
upstream at some point if that makes sense. But until that happens --
that is, until I have the conversations with people who added these and
care about their semantics -- assume that there's granularity for some
good reason. No need to put the cart before the horse.

This is a simple patch doing a simple thing in exactly the way that
things are already being done. I really don't want to do much more than
that here. If you want to bikeshed it further, send a follow up patch.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-29 15:55               ` Jason A. Donenfeld
@ 2022-06-30  9:15                 ` Peter Maydell
  2022-06-30 10:22                   ` Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-06-30  9:15 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Alex Bennée, qemu-devel

On Wed, 29 Jun 2022 at 16:56, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Wed, Jun 29, 2022 at 04:24:20PM +0100, Alex Bennée wrote:
> > Given the use case for the dtb-kaslr-seed knob I wonder if we should
> > have a common property and deprecate the kaslr one? As of this patch
> > existing workflows will break until command lines are updated to suppress
> > the second source of randomness.
> >
> > Maybe it would be better to have a single a new property
> > (dtb-rng-seeds?) which suppresses both dtb entries and make
> > dtb-kaslr-seed an alias and mark it as deprecated.
>
> No, I don't think so. If anything, I'll try to get rid of kaslr-seed
> upstream at some point if that makes sense. But until that happens --
> that is, until I have the conversations with people who added these and
> care about their semantics -- assume that there's granularity for some
> good reason. No need to put the cart before the horse.
>
> This is a simple patch doing a simple thing in exactly the way that
> things are already being done. I really don't want to do much more than
> that here. If you want to bikeshed it further, send a follow up patch.

It's adding a command line option, though. Those we have to get
right the first time, because for QEMU they're kind of like ABI
to our users. We *can* clean them up if we find we've made a mistake,
but we have to go through a multi-release deprecation process to do it,
so it's much less effort overall to make sure we have the command line
syntax right to start with.

If there's a good use case for the two seeds to be separately
controllable, that's fine. But I'd rather we find that out for
certain before we put a second control knob and make all our
users with workflows where they want non-random dtb blobs find
out about it and flip it.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] hw/arm/virt: dt: add rng-seed property
  2022-06-30  9:15                 ` Peter Maydell
@ 2022-06-30 10:22                   ` Jason A. Donenfeld
  2022-06-30 10:37                     ` [PATCH v2] " Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-30 10:22 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Alex Bennée, qemu-devel

On Thu, Jun 30, 2022 at 10:15:29AM +0100, Peter Maydell wrote:
> On Wed, 29 Jun 2022 at 16:56, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Wed, Jun 29, 2022 at 04:24:20PM +0100, Alex Bennée wrote:
> > > Given the use case for the dtb-kaslr-seed knob I wonder if we should
> > > have a common property and deprecate the kaslr one? As of this patch
> > > existing workflows will break until command lines are updated to suppress
> > > the second source of randomness.
> > >
> > > Maybe it would be better to have a single a new property
> > > (dtb-rng-seeds?) which suppresses both dtb entries and make
> > > dtb-kaslr-seed an alias and mark it as deprecated.
> >
> > No, I don't think so. If anything, I'll try to get rid of kaslr-seed
> > upstream at some point if that makes sense. But until that happens --
> > that is, until I have the conversations with people who added these and
> > care about their semantics -- assume that there's granularity for some
> > good reason. No need to put the cart before the horse.
> >
> > This is a simple patch doing a simple thing in exactly the way that
> > things are already being done. I really don't want to do much more than
> > that here. If you want to bikeshed it further, send a follow up patch.
> 
> It's adding a command line option, though. Those we have to get
> right the first time, because for QEMU they're kind of like ABI
> to our users. We *can* clean them up if we find we've made a mistake,
> but we have to go through a multi-release deprecation process to do it,
> so it's much less effort overall to make sure we have the command line
> syntax right to start with.
> 
> If there's a good use case for the two seeds to be separately
> controllable, that's fine. But I'd rather we find that out for
> certain before we put a second control knob and make all our
> users with workflows where they want non-random dtb blobs find
> out about it and flip it.

Okay. Do you want me to just make this controllable by dtb-kaslr-seed
for now, then, and we can rename that in a follow-up commit? I'll send a
patch for that.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] hw/arm/virt: dt: add rng-seed property
  2022-06-30 10:22                   ` Jason A. Donenfeld
@ 2022-06-30 10:37                     ` Jason A. Donenfeld
  2022-07-04 14:42                       ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-06-30 10:37 UTC (permalink / raw)
  To: Peter Maydell, Alex Bennée, qemu-devel; +Cc: Jason A. Donenfeld

In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
kaslr-seed property was added, but the equally as important rng-seed
property was forgotten about, which has identical semantics for a
similar purpose. This commit implements it in exactly the same way as
kaslr-seed. It then changes the name of the disabling option to reflect
that this has more to do with randomness vs determinism, rather than
something particular about kaslr.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 docs/system/arm/virt.rst | 17 ++++++++++------
 hw/arm/virt.c            | 44 ++++++++++++++++++++++++----------------
 include/hw/arm/virt.h    |  2 +-
 3 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 3d1058a80c..3b6ba69a9a 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -126,13 +126,18 @@ ras
   Set ``on``/``off`` to enable/disable reporting host memory errors to a guest
   using ACPI and guest external abort exceptions. The default is off.
 
+dtb-randomness
+  Set ``on``/``off`` to pass random seeds via the guest DTB
+  rng-seed and kaslr-seed nodes (in both "/chosen" and
+  "/secure-chosen") to use for features like the random number
+  generator and address space randomisation. The default is
+  ``on``. You will want to disable it if your trusted boot chain
+  will verify the DTB it is passed, since this option causes the
+  DTB to be non-deterministic. It would be the responsibility of
+  the firmware to come up with a seed and pass it on if it wants to.
+
 dtb-kaslr-seed
-  Set ``on``/``off`` to pass a random seed via the guest dtb
-  kaslr-seed node (in both "/chosen" and /secure-chosen) to use
-  for features like address space randomisation. The default is
-  ``on``. You will want to disable it if your trusted boot chain will
-  verify the DTB it is passed. It would be the responsibility of the
-  firmware to come up with a seed and pass it on if it wants to.
+  A deprecated synonym for dtb-randomness.
 
 Linux guest kernel configuration
 """"""""""""""""""""""""""""""""
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 097238faa7..924ded7f85 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -221,14 +221,18 @@ static bool cpu_type_valid(const char *cpu)
     return false;
 }
 
-static void create_kaslr_seed(MachineState *ms, const char *node)
+static void create_randomness(MachineState *ms, const char *node)
 {
-    uint64_t seed;
+    struct {
+        uint64_t kaslr;
+        uint8_t rng[32];
+    } seed;
 
     if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
         return;
     }
-    qemu_fdt_setprop_u64(ms->fdt, node, "kaslr-seed", seed);
+    qemu_fdt_setprop_u64(ms->fdt, node, "kaslr-seed", seed.kaslr);
+    qemu_fdt_setprop(ms->fdt, node, "rng-seed", seed.rng, sizeof(seed.rng));
 }
 
 static void create_fdt(VirtMachineState *vms)
@@ -251,14 +255,14 @@ static void create_fdt(VirtMachineState *vms)
 
     /* /chosen must exist for load_dtb to fill in necessary properties later */
     qemu_fdt_add_subnode(fdt, "/chosen");
-    if (vms->dtb_kaslr_seed) {
-        create_kaslr_seed(ms, "/chosen");
+    if (vms->dtb_randomness) {
+        create_randomness(ms, "/chosen");
     }
 
     if (vms->secure) {
         qemu_fdt_add_subnode(fdt, "/secure-chosen");
-        if (vms->dtb_kaslr_seed) {
-            create_kaslr_seed(ms, "/secure-chosen");
+        if (vms->dtb_randomness) {
+            create_randomness(ms, "/secure-chosen");
         }
     }
 
@@ -2348,18 +2352,18 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
     vms->its = value;
 }
 
-static bool virt_get_dtb_kaslr_seed(Object *obj, Error **errp)
+static bool virt_get_dtb_randomness(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
-    return vms->dtb_kaslr_seed;
+    return vms->dtb_randomness;
 }
 
-static void virt_set_dtb_kaslr_seed(Object *obj, bool value, Error **errp)
+static void virt_set_dtb_randomness(Object *obj, bool value, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
-    vms->dtb_kaslr_seed = value;
+    vms->dtb_randomness = value;
 }
 
 static char *virt_get_oem_id(Object *obj, Error **errp)
@@ -2988,12 +2992,18 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                                           "Set on/off to enable/disable "
                                           "ITS instantiation");
 
+    object_class_property_add_bool(oc, "dtb-randomness",
+                                   virt_get_dtb_randomness,
+                                   virt_set_dtb_randomness);
+    object_class_property_set_description(oc, "dtb-randomness",
+                                          "Set off to disable passing random or "
+                                          "non-deterministic dtb nodes to guest");
+
     object_class_property_add_bool(oc, "dtb-kaslr-seed",
-                                   virt_get_dtb_kaslr_seed,
-                                   virt_set_dtb_kaslr_seed);
+                                   virt_get_dtb_randomness,
+                                   virt_set_dtb_randomness);
     object_class_property_set_description(oc, "dtb-kaslr-seed",
-                                          "Set off to disable passing of kaslr-seed "
-                                          "dtb node to guest");
+                                          "Deprecated synonym of dtb-randomness");
 
     object_class_property_add_str(oc, "x-oem-id",
                                   virt_get_oem_id,
@@ -3061,8 +3071,8 @@ static void virt_instance_init(Object *obj)
     /* MTE is disabled by default.  */
     vms->mte = false;
 
-    /* Supply a kaslr-seed by default */
-    vms->dtb_kaslr_seed = true;
+    /* Supply kaslr-seed and rng-seed by default */
+    vms->dtb_randomness = true;
 
     vms->irqmap = a15irqmap;
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 15feabac63..6ec479ca2b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -152,7 +152,7 @@ struct VirtMachineState {
     bool virt;
     bool ras;
     bool mte;
-    bool dtb_kaslr_seed;
+    bool dtb_randomness;
     OnOffAuto acpi;
     VirtGICType gic_version;
     VirtIOMMUType iommu;
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] hw/arm/virt: dt: add rng-seed property
  2022-06-30 10:37                     ` [PATCH v2] " Jason A. Donenfeld
@ 2022-07-04 14:42                       ` Peter Maydell
  2022-07-05  0:45                         ` Jason A. Donenfeld
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2022-07-04 14:42 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Alex Bennée, QEMU Developers

On Thu, 30 Jun 2022 at 11:39, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> In 60592cfed2 ("hw/arm/virt: dt: add kaslr-seed property"), the
> kaslr-seed property was added, but the equally as important rng-seed
> property was forgotten about, which has identical semantics for a
> similar purpose. This commit implements it in exactly the same way as
> kaslr-seed. It then changes the name of the disabling option to reflect
> that this has more to do with randomness vs determinism, rather than
> something particular about kaslr.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks for this respin; I think this is the right way to go.

> ---
>  docs/system/arm/virt.rst | 17 ++++++++++------
>  hw/arm/virt.c            | 44 ++++++++++++++++++++++++----------------
>  include/hw/arm/virt.h    |  2 +-
>  3 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> index 3d1058a80c..3b6ba69a9a 100644
> --- a/docs/system/arm/virt.rst
> +++ b/docs/system/arm/virt.rst
> @@ -126,13 +126,18 @@ ras
>    Set ``on``/``off`` to enable/disable reporting host memory errors to a guest
>    using ACPI and guest external abort exceptions. The default is off.
>
> +dtb-randomness
> +  Set ``on``/``off`` to pass random seeds via the guest DTB
> +  rng-seed and kaslr-seed nodes (in both "/chosen" and
> +  "/secure-chosen") to use for features like the random number
> +  generator and address space randomisation. The default is
> +  ``on``. You will want to disable it if your trusted boot chain
> +  will verify the DTB it is passed, since this option causes the
> +  DTB to be non-deterministic. It would be the responsibility of
> +  the firmware to come up with a seed and pass it on if it wants to.
> +
>  dtb-kaslr-seed
> -  Set ``on``/``off`` to pass a random seed via the guest dtb
> -  kaslr-seed node (in both "/chosen" and /secure-chosen) to use
> -  for features like address space randomisation. The default is
> -  ``on``. You will want to disable it if your trusted boot chain will
> -  verify the DTB it is passed. It would be the responsibility of the
> -  firmware to come up with a seed and pass it on if it wants to.
> +  A deprecated synonym for dtb-randomness.

We should also add a section to docs/about/deprecated.rst
noting that the old name is deprecated in favour of the new one.
I'll fold that in when I add the patch to target-arm.next, to
save you doing a respin:

--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -225,6 +225,14 @@ Use the more generic event
``DEVICE_UNPLUG_GUEST_ERROR`` instead.
 System emulator machines
 ------------------------

+Arm ``virt`` machine ``dtb-kaslr-seed`` property
+''''''''''''''''''''''''''''''''''''''''''''''''
+
+The ``dtb-kaslr-seed`` property on the ``virt`` board has been
+deprecated; use the new name ``dtb-randomness`` instead. The new name
+better reflects the way this property affects all random data within
+the device tree blob, not just the ``kaslr-seed`` node.
+

> +    object_class_property_add_bool(oc, "dtb-randomness",
> +                                   virt_get_dtb_randomness,
> +                                   virt_set_dtb_randomness);
> +    object_class_property_set_description(oc, "dtb-randomness",
> +                                          "Set off to disable passing random or "
> +                                          "non-deterministic dtb nodes to guest");
> +
>      object_class_property_add_bool(oc, "dtb-kaslr-seed",
> -                                   virt_get_dtb_kaslr_seed,
> -                                   virt_set_dtb_kaslr_seed);
> +                                   virt_get_dtb_randomness,
> +                                   virt_set_dtb_randomness);
>      object_class_property_set_description(oc, "dtb-kaslr-seed",
> -                                          "Set off to disable passing of kaslr-seed "
> -                                          "dtb node to guest");
> +                                          "Deprecated synonym of dtb-randomness");

I was going to suggest object_property_add_alias() but that
works at the object level, not the class level, so never mind.
This will work, and eventually we'll delete the code when it
hits the deprecation-to-removal horizon. So

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

and applied to target-arm.next.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] hw/arm/virt: dt: add rng-seed property
  2022-07-04 14:42                       ` Peter Maydell
@ 2022-07-05  0:45                         ` Jason A. Donenfeld
  0 siblings, 0 replies; 15+ messages in thread
From: Jason A. Donenfeld @ 2022-07-05  0:45 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Alex Bennée, QEMU Developers

Hi Peter,

On Mon, Jul 04, 2022 at 03:42:55PM +0100, Peter Maydell wrote:
> We should also add a section to docs/about/deprecated.rst
> noting that the old name is deprecated in favour of the new one.
> I'll fold that in when I add the patch to target-arm.next, to
> save you doing a respin:

Thanks for doing that. Your text looks good to me.

Jason


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-07-05  0:47 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 16:07 [PATCH] hw/arm/virt: dt: add rng-seed property Jason A. Donenfeld
2022-06-27 16:12 ` Peter Maydell
2022-06-27 16:36   ` Jason A. Donenfeld
2022-06-28 18:45     ` Jason A. Donenfeld
2022-06-29  9:37       ` Peter Maydell
2022-06-29 10:15         ` Alex Bennée
2022-06-29 10:18         ` Alex Bennée
2022-06-29 11:26           ` Jason A. Donenfeld
2022-06-29 15:24             ` Alex Bennée
2022-06-29 15:55               ` Jason A. Donenfeld
2022-06-30  9:15                 ` Peter Maydell
2022-06-30 10:22                   ` Jason A. Donenfeld
2022-06-30 10:37                     ` [PATCH v2] " Jason A. Donenfeld
2022-07-04 14:42                       ` Peter Maydell
2022-07-05  0:45                         ` Jason A. Donenfeld

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.