| Message ID | 20220627160734.749861-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: dt: add rng-seed property | expand |
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
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
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
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
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
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.
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
"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.
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
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
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
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;
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(+)