Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On Thu, Jun 30, 2022 at 10:15:29AM +0100, Peter Maydell wrote: > On Wed, 29 Jun 2022 at 16:56, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On Wed, 29 Jun 2022 at 16:56, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
"Jason A. Donenfeld" writes:
> On Wed, Jun 29, 2022 at 11:18:23AM +0100, Alex Bennée wrote:
>>
>> Peter Maydell writes:
>>
>> > On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld wrote:
>> >>
>> >> On 6/27/22, Jason A. Donenfeld wrote:
>> >> > On 6/27/22, Peter Maydell wrote:
>> >> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld
>> >> >> 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On Wed, Jun 29, 2022 at 11:18:23AM +0100, Alex Bennée wrote:
>
> Peter Maydell writes:
>
> > On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld wrote:
> >>
> >> On 6/27/22, Jason A. Donenfeld wrote:
> >> > On 6/27/22, Peter Maydell wrote:
> >> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld
> >> >> 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
Peter Maydell writes:
> On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld wrote:
>>
>> On 6/27/22, Jason A. Donenfeld wrote:
>> > On 6/27/22, Peter Maydell wrote:
>> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
Peter Maydell writes:
> On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld wrote:
>>
>> On 6/27/22, Jason A. Donenfeld wrote:
>> > On 6/27/22, Peter Maydell wrote:
>> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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(, 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On Tue, 28 Jun 2022 at 19:45, Jason A. Donenfeld wrote:
>
> On 6/27/22, Jason A. Donenfeld wrote:
> > On 6/27/22, Peter Maydell wrote:
> >> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On 6/27/22, Jason A. Donenfeld wrote:
> On 6/27/22, Peter Maydell wrote:
>> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On 6/27/22, Peter Maydell wrote:
> On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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
Re: [PATCH] hw/arm/virt: dt: add rng-seed property
On Mon, 27 Jun 2022 at 17:07, Jason A. Donenfeld 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
