* [PATCH v2 0/2] Use RNG to get random behaviour
@ 2020-12-16 16:28 matthias.bgg at kernel.org
2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: matthias.bgg at kernel.org @ 2020-12-16 16:28 UTC (permalink / raw)
To: u-boot
From: Matthias Brugger <mbrugger@suse.com>
For now bootp and uuid code use a weak seed for generating random data.
U-Boot as support for RNG devices now, so we should change to code to
use them if they are present. This will help mitigate issues like seen
in CVE-2019-11690.
Changes in v2:
- fix dm_rng_read() parameters
- add missing include
- fix dm_rng_read() parameters
- add missing include file
Matthias Brugger (2):
lib: uuid: use RNG device if present
net: Use NDRNG device in srand_mac()
lib/uuid.c | 21 ++++++++++++++++++---
net/net_rand.h | 19 ++++++++++++++++++-
2 files changed, 36 insertions(+), 4 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-16 16:28 [PATCH v2 0/2] Use RNG to get random behaviour matthias.bgg at kernel.org @ 2020-12-16 16:28 ` matthias.bgg at kernel.org 2020-12-18 8:37 ` Torsten Duwe 2020-12-19 2:29 ` Simon Glass 2020-12-16 16:28 ` [PATCH v2 2/2] net: Use NDRNG device in srand_mac() matthias.bgg at kernel.org 2020-12-18 8:31 ` [PATCH v2 0/2] Use RNG to get random behaviour Torsten Duwe 2 siblings, 2 replies; 16+ messages in thread From: matthias.bgg at kernel.org @ 2020-12-16 16:28 UTC (permalink / raw) To: u-boot From: Matthias Brugger <mbrugger@suse.com> When calculating a random UUID we use a weak seed. Use a RNG device if present to increase entropy. Signed-off-by: Matthias Brugger <mbrugger@suse.com> --- Changes in v2: - fix dm_rng_read() parameters - add missing include lib/uuid.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/uuid.c b/lib/uuid.c index e62d5ca264..e3487380c3 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -15,6 +15,8 @@ #include <asm/io.h> #include <part_efi.h> #include <malloc.h> +#include <dm/uclass.h> +#include <rng.h> /* * UUID - Universally Unique IDentifier - 128 bits unique number. @@ -249,9 +251,22 @@ void gen_rand_uuid(unsigned char *uuid_bin) { u32 ptr[4]; struct uuid *uuid = (struct uuid *)ptr; - int i; - - srand(get_ticks() + rand()); + int i, ret; + struct udevice *devp; + u8 randv = 0; + +#if defined(CONFIG_DM_RNG) + ret = uclass_get_device(UCLASS_RNG, 0, &devp); + if (ret) { + ret = dm_rng_read(devp, &randv, sizeof(randv)); + if (ret < 0) + randv = 0; + } + if (randv) + srand(randv); + else +#endif + srand(get_ticks() + rand()); /* Set all fields randomly */ for (i = 0; i < 4; i++) -- 2.29.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org @ 2020-12-18 8:37 ` Torsten Duwe 2020-12-18 9:10 ` mbrugger 2020-12-19 2:29 ` Simon Glass 1 sibling, 1 reply; 16+ messages in thread From: Torsten Duwe @ 2020-12-18 8:37 UTC (permalink / raw) To: u-boot On Wed, 16 Dec 2020 17:28:06 +0100 matthias.bgg at kernel.org wrote: > @@ -249,9 +251,22 @@ void gen_rand_uuid(unsigned char *uuid_bin) > { > u32 ptr[4]; > struct uuid *uuid = (struct uuid *)ptr; > - int i; > - > - srand(get_ticks() + rand()); > + int i, ret; > + struct udevice *devp; > + u8 randv = 0; ^^ Only 1 byte? The UUID could use some more, and, having a HW RNG, it doesn't hurt to seed it with more entropy here. I suggest using u32 as well. > + > +#if defined(CONFIG_DM_RNG) > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > + if (ret) { > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > + if (ret < 0) > + randv = 0; > + } See my reply to the cover letter. I'd suggest to wrap this with if (IS_ENABLED(CONFIG_DM_RNG)) instead, likewise for the MAC. Torsten ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-18 8:37 ` Torsten Duwe @ 2020-12-18 9:10 ` mbrugger 0 siblings, 0 replies; 16+ messages in thread From: mbrugger @ 2020-12-18 9:10 UTC (permalink / raw) To: u-boot On Fri, Dec 18, 2020 at 09:37:28AM +0100, Torsten Duwe wrote: > On Wed, 16 Dec 2020 17:28:06 +0100 > matthias.bgg at kernel.org wrote: > > > @@ -249,9 +251,22 @@ void gen_rand_uuid(unsigned char *uuid_bin) > > { > > u32 ptr[4]; > > struct uuid *uuid = (struct uuid *)ptr; > > - int i; > > - > > - srand(get_ticks() + rand()); > > + int i, ret; > > + struct udevice *devp; > > + u8 randv = 0; > ^^ > Only 1 byte? The UUID could use some more, and, having a HW RNG, it > doesn't hurt to seed it with more entropy here. I suggest using u32 as > well. > > > + > > +#if defined(CONFIG_DM_RNG) > > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > > + if (ret) { > > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > > + if (ret < 0) > > + randv = 0; > > + } > > See my reply to the cover letter. I'd suggest to wrap this with > if (IS_ENABLED(CONFIG_DM_RNG)) > instead, likewise for the MAC. Sounds good, I'll send a new version addressing both issues. Thanks for the review. Matthias ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org 2020-12-18 8:37 ` Torsten Duwe @ 2020-12-19 2:29 ` Simon Glass 2020-12-20 16:59 ` Torsten Duwe 2020-12-23 10:09 ` Matthias Brugger 1 sibling, 2 replies; 16+ messages in thread From: Simon Glass @ 2020-12-19 2:29 UTC (permalink / raw) To: u-boot Hi Mattias, On Wed, 16 Dec 2020 at 09:28, <matthias.bgg@kernel.org> wrote: > > From: Matthias Brugger <mbrugger@suse.com> > > When calculating a random UUID we use a weak seed. > Use a RNG device if present to increase entropy. > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > --- > > Changes in v2: > - fix dm_rng_read() parameters > - add missing include > > lib/uuid.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/lib/uuid.c b/lib/uuid.c > index e62d5ca264..e3487380c3 100644 > --- a/lib/uuid.c > +++ b/lib/uuid.c > @@ -15,6 +15,8 @@ > #include <asm/io.h> > #include <part_efi.h> > #include <malloc.h> > +#include <dm/uclass.h> > +#include <rng.h> > > /* > * UUID - Universally Unique IDentifier - 128 bits unique number. > @@ -249,9 +251,22 @@ void gen_rand_uuid(unsigned char *uuid_bin) > { > u32 ptr[4]; > struct uuid *uuid = (struct uuid *)ptr; > - int i; > - > - srand(get_ticks() + rand()); > + int i, ret; > + struct udevice *devp; > + u8 randv = 0; > + > +#if defined(CONFIG_DM_RNG) This seems a little backwards to me. The caller should request a RNG device, getting either a hardware one or a software one, and then call the uclass method to get the uuid. > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > + if (ret) { > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > + if (ret < 0) > + randv = 0; > + } > + if (randv) > + srand(randv); > + else > +#endif > + srand(get_ticks() + rand()); > > /* Set all fields randomly */ > for (i = 0; i < 4; i++) > -- > 2.29.2 > Regards, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-19 2:29 ` Simon Glass @ 2020-12-20 16:59 ` Torsten Duwe 2020-12-20 18:17 ` Simon Glass 2020-12-23 10:09 ` Matthias Brugger 1 sibling, 1 reply; 16+ messages in thread From: Torsten Duwe @ 2020-12-20 16:59 UTC (permalink / raw) To: u-boot On Fri, 18 Dec 2020 19:29:12 -0700 Simon Glass <sjg@chromium.org> wrote: > > - int i; > > - > > - srand(get_ticks() + rand()); > > + int i, ret; > > + struct udevice *devp; > > + u8 randv = 0; > > + > > +#if defined(CONFIG_DM_RNG) > > This seems a little backwards to me. The caller should request a RNG > device, getting either a hardware one or a software one, and then call > the uclass method to get the uuid. Strictly speaking, there's no such thing as a "software RNG". The term "DRBG" was coined for accurateness, "deterministic random bit generator". The oxymoron "deterministic random" pretty much nails it. Alternatively, it can be called "pseudo" RNG. rand() and srand() exactly implement such a mechanism already, with low coding overhead. U-Boot runs fine with them most of the time, but there are rare cases where real entropy would be needed. This is what these two patches are about. In case there's more, I already speculated about a centralised entity in my response to the v1 cover letter, but for now these two changes should do. > > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > > + if (ret) { > > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > > + if (ret < 0) > > + randv = 0; > > + } > > + if (randv) > > + srand(randv); > > + else > > +#endif > > + srand(get_ticks() + rand()); > > > > /* Set all fields randomly */ > > for (i = 0; i < 4; i++) [ rand() usage following here ^ ] The #ifdefs are already gone in v3. Torsten ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-20 16:59 ` Torsten Duwe @ 2020-12-20 18:17 ` Simon Glass 2020-12-23 10:24 ` Torsten Duwe 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2020-12-20 18:17 UTC (permalink / raw) To: u-boot Hi Torsten, On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: > > On Fri, 18 Dec 2020 19:29:12 -0700 > Simon Glass <sjg@chromium.org> wrote: > > > > - int i; > > > - > > > - srand(get_ticks() + rand()); > > > + int i, ret; > > > + struct udevice *devp; > > > + u8 randv = 0; > > > + > > > +#if defined(CONFIG_DM_RNG) > > > > This seems a little backwards to me. The caller should request a RNG > > device, getting either a hardware one or a software one, and then call > > the uclass method to get the uuid. > > Strictly speaking, there's no such thing as a "software RNG". The term > "DRBG" was coined for accurateness, "deterministic random bit > generator". The oxymoron "deterministic random" pretty much nails it. > Alternatively, it can be called "pseudo" RNG. > rand() and srand() exactly implement such a mechanism already, with low > coding overhead. U-Boot runs fine with them most of the time, but there > are rare cases where real entropy would be needed. This is what these > two patches are about. In case there's more, I already speculated about > a centralised entity in my response to the v1 cover letter, but for now > these two changes should do. I am used to the term pseudo-random, but it doesn't much matter what kind of random number is used. It is still covered by the RNG class. You are currently burying device access in a utility function. That really isn't the right way to do it. See my comment above. There is no way to control which RNG device is used and no visibility that this is happening at all, outside this function. > > > > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > > > + if (ret) { > > > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > > > + if (ret < 0) > > > + randv = 0; > > > + } > > > + if (randv) > > > + srand(randv); > > > + else > > > +#endif > > > + srand(get_ticks() + rand()); > > > > > > /* Set all fields randomly */ > > > for (i = 0; i < 4; i++) > [ rand() usage following here ^ ] > > The #ifdefs are already gone in v3. OK good. Regards, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-20 18:17 ` Simon Glass @ 2020-12-23 10:24 ` Torsten Duwe 2020-12-29 3:32 ` Simon Glass 0 siblings, 1 reply; 16+ messages in thread From: Torsten Duwe @ 2020-12-23 10:24 UTC (permalink / raw) To: u-boot On Sun, 20 Dec 2020 11:17:50 -0700 Simon Glass <sjg@chromium.org> wrote: > Hi Torsten, > > On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: > > > > On Fri, 18 Dec 2020 19:29:12 -0700 > > Simon Glass <sjg@chromium.org> wrote: > > > > > > - int i; > > > > - > > > > - srand(get_ticks() + rand()); > > > > + int i, ret; > > > > + struct udevice *devp; > > > > + u8 randv = 0; > > > > + > > > > +#if defined(CONFIG_DM_RNG) > > > > > > This seems a little backwards to me. The caller should request a > > > RNG device, getting either a hardware one or a software one, and > > > then call the uclass method to get the uuid. > > > > Strictly speaking, there's no such thing as a "software RNG". The > > term "DRBG" was coined for accurateness, "deterministic random bit > > generator". The oxymoron "deterministic random" pretty much nails > > it. Alternatively, it can be called "pseudo" RNG. > > rand() and srand() exactly implement such a mechanism already, with > > low coding overhead. U-Boot runs fine with them most of the time, > > but there are rare cases where real entropy would be needed. This > > is what these two patches are about. In case there's more, I > > already speculated about a centralised entity in my response to the > > v1 cover letter, but for now these two changes should do. > > I am used to the term pseudo-random, but it doesn't much matter what > kind of random number is used. It is still covered by the RNG class. Well, in these 2 cases, it _does_ matter. And besides, as I wrote above, pseudo randomness is produced by the rand() function, and RNG devices provide _real_ entropy to a system. So, while every other entity in U-Boot is happy with a DRBG, a UUID and a BOOTP delay need this real entropy, hence the different code, for a start. > You are currently burying device access in a utility function. That > really isn't the right way to do it. See my comment above. There is no > way to control which RNG device is used and no visibility that this is > happening at all, outside this function. The code looks a bit odd to me, too, as I mentioned. I imagined something not so full blown as in the Linux kernel, but still some central mechanism to get entropy from, for those who really need it (in the current situation, just these 2 cases). This alternative would result in a real /dev/random in U-Boot, which would yield a cleaner structure, but would require more code to be produced and more code needing change. That given, I'd agree to these 2 hacks, especially because there are security implications. What's your opinion, how would you like to create really unique UUIDs? How should BOOTP clients wait randomly (esp. in a large group)? Shall we create some U-Boot version of /dev/random and haveged? I'm really open to suggestions. Torsten ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-23 10:24 ` Torsten Duwe @ 2020-12-29 3:32 ` Simon Glass 2021-01-20 11:17 ` Matthias Brugger 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2020-12-29 3:32 UTC (permalink / raw) To: u-boot Hi guys, On Wed, 23 Dec 2020 at 03:24, Torsten Duwe <duwe@lst.de> wrote: > > On Sun, 20 Dec 2020 11:17:50 -0700 > Simon Glass <sjg@chromium.org> wrote: > > > Hi Torsten, > > > > On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: > > > > > > On Fri, 18 Dec 2020 19:29:12 -0700 > > > Simon Glass <sjg@chromium.org> wrote: > > > > > > > > - int i; > > > > > - > > > > > - srand(get_ticks() + rand()); > > > > > + int i, ret; > > > > > + struct udevice *devp; > > > > > + u8 randv = 0; > > > > > + > > > > > +#if defined(CONFIG_DM_RNG) > > > > > > > > This seems a little backwards to me. The caller should request a > > > > RNG device, getting either a hardware one or a software one, and > > > > then call the uclass method to get the uuid. > > > > > > Strictly speaking, there's no such thing as a "software RNG". The > > > term "DRBG" was coined for accurateness, "deterministic random bit > > > generator". The oxymoron "deterministic random" pretty much nails > > > it. Alternatively, it can be called "pseudo" RNG. > > > rand() and srand() exactly implement such a mechanism already, with > > > low coding overhead. U-Boot runs fine with them most of the time, > > > but there are rare cases where real entropy would be needed. This > > > is what these two patches are about. In case there's more, I > > > already speculated about a centralised entity in my response to the > > > v1 cover letter, but for now these two changes should do. > > > > I am used to the term pseudo-random, but it doesn't much matter what > > kind of random number is used. It is still covered by the RNG class. > > Well, in these 2 cases, it _does_ matter. And besides, as I wrote > above, pseudo randomness is produced by the rand() function, and RNG > devices provide _real_ entropy to a system. > > So, while every other entity in U-Boot is happy with a DRBG, a UUID and > a BOOTP delay need this real entropy, hence the different code, for a > start. > > > You are currently burying device access in a utility function. That > > really isn't the right way to do it. See my comment above. There is no > > way to control which RNG device is used and no visibility that this is > > happening at all, outside this function. > > The code looks a bit odd to me, too, as I mentioned. I imagined > something not so full blown as in the Linux kernel, but still some > central mechanism to get entropy from, for those who really need it (in > the current situation, just these 2 cases). This alternative would > result in a real /dev/random in U-Boot, which would yield a cleaner > structure, but would require more code to be produced and more code > needing change. That given, I'd agree to these 2 hacks, especially > because there are security implications. > > What's your opinion, how would you like to create really unique UUIDs? > How should BOOTP clients wait randomly (esp. in a large group)? > Shall we create some U-Boot version of /dev/random and haveged? > > I'm really open to suggestions. > I really don't mind too much about that side of it. But I do worry when I see code that buries hard-coded DM access in a lib/ function. If boot wants a random device it should get one, at the top level, not in the bowels of U-Boot. It can pass that device down to other functions as needed. To my reading, the current definition of the RNG uclass looks similar to a /dev/random Let me know if I am missing something. Regards, Simon ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-29 3:32 ` Simon Glass @ 2021-01-20 11:17 ` Matthias Brugger 2021-01-20 19:50 ` Simon Glass 0 siblings, 1 reply; 16+ messages in thread From: Matthias Brugger @ 2021-01-20 11:17 UTC (permalink / raw) To: u-boot On 29/12/2020 04:32, Simon Glass wrote: > Hi guys, > > On Wed, 23 Dec 2020 at 03:24, Torsten Duwe <duwe@lst.de> wrote: >> >> On Sun, 20 Dec 2020 11:17:50 -0700 >> Simon Glass <sjg@chromium.org> wrote: >> >>> Hi Torsten, >>> >>> On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: >>>> >>>> On Fri, 18 Dec 2020 19:29:12 -0700 >>>> Simon Glass <sjg@chromium.org> wrote: >>>> >>>>>> - int i; >>>>>> - >>>>>> - srand(get_ticks() + rand()); >>>>>> + int i, ret; >>>>>> + struct udevice *devp; >>>>>> + u8 randv = 0; >>>>>> + >>>>>> +#if defined(CONFIG_DM_RNG) >>>>> >>>>> This seems a little backwards to me. The caller should request a >>>>> RNG device, getting either a hardware one or a software one, and >>>>> then call the uclass method to get the uuid. >>>> >>>> Strictly speaking, there's no such thing as a "software RNG". The >>>> term "DRBG" was coined for accurateness, "deterministic random bit >>>> generator". The oxymoron "deterministic random" pretty much nails >>>> it. Alternatively, it can be called "pseudo" RNG. >>>> rand() and srand() exactly implement such a mechanism already, with >>>> low coding overhead. U-Boot runs fine with them most of the time, >>>> but there are rare cases where real entropy would be needed. This >>>> is what these two patches are about. In case there's more, I >>>> already speculated about a centralised entity in my response to the >>>> v1 cover letter, but for now these two changes should do. >>> >>> I am used to the term pseudo-random, but it doesn't much matter what >>> kind of random number is used. It is still covered by the RNG class. >> >> Well, in these 2 cases, it _does_ matter. And besides, as I wrote >> above, pseudo randomness is produced by the rand() function, and RNG >> devices provide _real_ entropy to a system. >> >> So, while every other entity in U-Boot is happy with a DRBG, a UUID and >> a BOOTP delay need this real entropy, hence the different code, for a >> start. >> >>> You are currently burying device access in a utility function. That >>> really isn't the right way to do it. See my comment above. There is no >>> way to control which RNG device is used and no visibility that this is >>> happening at all, outside this function. >> >> The code looks a bit odd to me, too, as I mentioned. I imagined >> something not so full blown as in the Linux kernel, but still some >> central mechanism to get entropy from, for those who really need it (in >> the current situation, just these 2 cases). This alternative would >> result in a real /dev/random in U-Boot, which would yield a cleaner >> structure, but would require more code to be produced and more code >> needing change. That given, I'd agree to these 2 hacks, especially >> because there are security implications. >> >> What's your opinion, how would you like to create really unique UUIDs? >> How should BOOTP clients wait randomly (esp. in a large group)? >> Shall we create some U-Boot version of /dev/random and haveged? >> >> I'm really open to suggestions. >> > > I really don't mind too much about that side of it. But I do worry > when I see code that buries hard-coded DM access in a lib/ function. > v3 of this series got merged, which should not stop us from making the code better. > If boot wants a random device it should get one, at the top level, not > in the bowels of U-Boot. It can pass that device down to other > functions as needed. > So you mean we should get the RNG udevice at the caller of gen_rand_uuid_str and then pass it down to gen_rand_uuid? And if no device has been found, we will call srand(get_ticks() + rand())? I'm not quite sure how we would need to implement this in the case of srand_mac() though. Regards, Matthias > To my reading, the current definition of the RNG uclass looks similar > to a /dev/random > > Let me know if I am missing something. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2021-01-20 11:17 ` Matthias Brugger @ 2021-01-20 19:50 ` Simon Glass 2021-01-20 19:56 ` Matthias Brugger 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2021-01-20 19:50 UTC (permalink / raw) To: u-boot Hi Matthias, On Wed, 20 Jan 2021 at 04:17, Matthias Brugger <mbrugger@suse.com> wrote: > > > > On 29/12/2020 04:32, Simon Glass wrote: > > Hi guys, > > > > On Wed, 23 Dec 2020 at 03:24, Torsten Duwe <duwe@lst.de> wrote: > >> > >> On Sun, 20 Dec 2020 11:17:50 -0700 > >> Simon Glass <sjg@chromium.org> wrote: > >> > >>> Hi Torsten, > >>> > >>> On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: > >>>> > >>>> On Fri, 18 Dec 2020 19:29:12 -0700 > >>>> Simon Glass <sjg@chromium.org> wrote: > >>>> > >>>>>> - int i; > >>>>>> - > >>>>>> - srand(get_ticks() + rand()); > >>>>>> + int i, ret; > >>>>>> + struct udevice *devp; > >>>>>> + u8 randv = 0; > >>>>>> + > >>>>>> +#if defined(CONFIG_DM_RNG) > >>>>> > >>>>> This seems a little backwards to me. The caller should request a > >>>>> RNG device, getting either a hardware one or a software one, and > >>>>> then call the uclass method to get the uuid. > >>>> > >>>> Strictly speaking, there's no such thing as a "software RNG". The > >>>> term "DRBG" was coined for accurateness, "deterministic random bit > >>>> generator". The oxymoron "deterministic random" pretty much nails > >>>> it. Alternatively, it can be called "pseudo" RNG. > >>>> rand() and srand() exactly implement such a mechanism already, with > >>>> low coding overhead. U-Boot runs fine with them most of the time, > >>>> but there are rare cases where real entropy would be needed. This > >>>> is what these two patches are about. In case there's more, I > >>>> already speculated about a centralised entity in my response to the > >>>> v1 cover letter, but for now these two changes should do. > >>> > >>> I am used to the term pseudo-random, but it doesn't much matter what > >>> kind of random number is used. It is still covered by the RNG class. > >> > >> Well, in these 2 cases, it _does_ matter. And besides, as I wrote > >> above, pseudo randomness is produced by the rand() function, and RNG > >> devices provide _real_ entropy to a system. > >> > >> So, while every other entity in U-Boot is happy with a DRBG, a UUID and > >> a BOOTP delay need this real entropy, hence the different code, for a > >> start. > >> > >>> You are currently burying device access in a utility function. That > >>> really isn't the right way to do it. See my comment above. There is no > >>> way to control which RNG device is used and no visibility that this is > >>> happening at all, outside this function. > >> > >> The code looks a bit odd to me, too, as I mentioned. I imagined > >> something not so full blown as in the Linux kernel, but still some > >> central mechanism to get entropy from, for those who really need it (in > >> the current situation, just these 2 cases). This alternative would > >> result in a real /dev/random in U-Boot, which would yield a cleaner > >> structure, but would require more code to be produced and more code > >> needing change. That given, I'd agree to these 2 hacks, especially > >> because there are security implications. > >> > >> What's your opinion, how would you like to create really unique UUIDs? > >> How should BOOTP clients wait randomly (esp. in a large group)? > >> Shall we create some U-Boot version of /dev/random and haveged? > >> > >> I'm really open to suggestions. > >> > > > > I really don't mind too much about that side of it. But I do worry > > when I see code that buries hard-coded DM access in a lib/ function. > > > > v3 of this series got merged, which should not stop us from making the code better. > > > > If boot wants a random device it should get one, at the top level, not > > in the bowels of U-Boot. It can pass that device down to other > > functions as needed. > > > > So you mean we should get the RNG udevice at the caller of gen_rand_uuid_str and > then pass it down to gen_rand_uuid? And if no device has been found, we will > call srand(get_ticks() + rand())? Yes. I suppose you could call it with NULL if there is no device, but I think it would be much better to have a software RNG device that can be enabled on platforms that don't have hardware support. Then your get_ticks() + rand() can be in that. Also the command stuff in uuid.c should move to cmd/ > > I'm not quite sure how we would need to implement this in the case of > srand_mac() though. You can put it in struct eth_uclass_priv and provide a function in eth-uclass.c to return it? Then call that from bootp.c At least then the caller is in control. > > Regards, > Matthias > > > To my reading, the current definition of the RNG uclass looks similar > > to a /dev/random > > > > Let me know if I am missing something. > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2021-01-20 19:50 ` Simon Glass @ 2021-01-20 19:56 ` Matthias Brugger 0 siblings, 0 replies; 16+ messages in thread From: Matthias Brugger @ 2021-01-20 19:56 UTC (permalink / raw) To: u-boot On Wed, Jan 20, 2021 at 12:50:56PM -0700, Simon Glass wrote: > Hi Matthias, > > On Wed, 20 Jan 2021 at 04:17, Matthias Brugger <mbrugger@suse.com> wrote: > > > > > > > > On 29/12/2020 04:32, Simon Glass wrote: > > > Hi guys, > > > > > > On Wed, 23 Dec 2020 at 03:24, Torsten Duwe <duwe@lst.de> wrote: > > >> > > >> On Sun, 20 Dec 2020 11:17:50 -0700 > > >> Simon Glass <sjg@chromium.org> wrote: > > >> > > >>> Hi Torsten, > > >>> > > >>> On Sun, 20 Dec 2020 at 10:00, Torsten Duwe <duwe@lst.de> wrote: > > >>>> > > >>>> On Fri, 18 Dec 2020 19:29:12 -0700 > > >>>> Simon Glass <sjg@chromium.org> wrote: > > >>>> > > >>>>>> - int i; > > >>>>>> - > > >>>>>> - srand(get_ticks() + rand()); > > >>>>>> + int i, ret; > > >>>>>> + struct udevice *devp; > > >>>>>> + u8 randv = 0; > > >>>>>> + > > >>>>>> +#if defined(CONFIG_DM_RNG) > > >>>>> > > >>>>> This seems a little backwards to me. The caller should request a > > >>>>> RNG device, getting either a hardware one or a software one, and > > >>>>> then call the uclass method to get the uuid. > > >>>> > > >>>> Strictly speaking, there's no such thing as a "software RNG". The > > >>>> term "DRBG" was coined for accurateness, "deterministic random bit > > >>>> generator". The oxymoron "deterministic random" pretty much nails > > >>>> it. Alternatively, it can be called "pseudo" RNG. > > >>>> rand() and srand() exactly implement such a mechanism already, with > > >>>> low coding overhead. U-Boot runs fine with them most of the time, > > >>>> but there are rare cases where real entropy would be needed. This > > >>>> is what these two patches are about. In case there's more, I > > >>>> already speculated about a centralised entity in my response to the > > >>>> v1 cover letter, but for now these two changes should do. > > >>> > > >>> I am used to the term pseudo-random, but it doesn't much matter what > > >>> kind of random number is used. It is still covered by the RNG class. > > >> > > >> Well, in these 2 cases, it _does_ matter. And besides, as I wrote > > >> above, pseudo randomness is produced by the rand() function, and RNG > > >> devices provide _real_ entropy to a system. > > >> > > >> So, while every other entity in U-Boot is happy with a DRBG, a UUID and > > >> a BOOTP delay need this real entropy, hence the different code, for a > > >> start. > > >> > > >>> You are currently burying device access in a utility function. That > > >>> really isn't the right way to do it. See my comment above. There is no > > >>> way to control which RNG device is used and no visibility that this is > > >>> happening at all, outside this function. > > >> > > >> The code looks a bit odd to me, too, as I mentioned. I imagined > > >> something not so full blown as in the Linux kernel, but still some > > >> central mechanism to get entropy from, for those who really need it (in > > >> the current situation, just these 2 cases). This alternative would > > >> result in a real /dev/random in U-Boot, which would yield a cleaner > > >> structure, but would require more code to be produced and more code > > >> needing change. That given, I'd agree to these 2 hacks, especially > > >> because there are security implications. > > >> > > >> What's your opinion, how would you like to create really unique UUIDs? > > >> How should BOOTP clients wait randomly (esp. in a large group)? > > >> Shall we create some U-Boot version of /dev/random and haveged? > > >> > > >> I'm really open to suggestions. > > >> > > > > > > I really don't mind too much about that side of it. But I do worry > > > when I see code that buries hard-coded DM access in a lib/ function. > > > > > > > v3 of this series got merged, which should not stop us from making the code better. > > > > > > > If boot wants a random device it should get one, at the top level, not > > > in the bowels of U-Boot. It can pass that device down to other > > > functions as needed. > > > > > > > So you mean we should get the RNG udevice at the caller of gen_rand_uuid_str and > > then pass it down to gen_rand_uuid? And if no device has been found, we will > > call srand(get_ticks() + rand())? > > Yes. I suppose you could call it with NULL if there is no device, but > I think it would be much better to have a software RNG device that can > be enabled on platforms that don't have hardware support. Then your > get_ticks() + rand() can be in that. Well the thing is that up to now the SW RNG implementation uses different ways in uuid and bootp to create the seeds. > > Also the command stuff in uuid.c should move to cmd/ > Yes, realized that as well :) > > > > I'm not quite sure how we would need to implement this in the case of > > srand_mac() though. > > You can put it in struct eth_uclass_priv and provide a function in > eth-uclass.c to return it? Then call that from bootp.c > > At least then the caller is in control. > I'll have a look on that. Regards, Matthias > > > > Regards, > > Matthias > > > > > To my reading, the current definition of the RNG uclass looks similar > > > to a /dev/random > > > > > > Let me know if I am missing something. > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] lib: uuid: use RNG device if present 2020-12-19 2:29 ` Simon Glass 2020-12-20 16:59 ` Torsten Duwe @ 2020-12-23 10:09 ` Matthias Brugger 1 sibling, 0 replies; 16+ messages in thread From: Matthias Brugger @ 2020-12-23 10:09 UTC (permalink / raw) To: u-boot Hi Simon, On 19/12/2020 03:29, Simon Glass wrote: > Hi Mattias, > > On Wed, 16 Dec 2020 at 09:28, <matthias.bgg@kernel.org> wrote: >> >> From: Matthias Brugger <mbrugger@suse.com> >> >> When calculating a random UUID we use a weak seed. >> Use a RNG device if present to increase entropy. >> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> >> --- >> >> Changes in v2: >> - fix dm_rng_read() parameters >> - add missing include >> >> lib/uuid.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/lib/uuid.c b/lib/uuid.c >> index e62d5ca264..e3487380c3 100644 >> --- a/lib/uuid.c >> +++ b/lib/uuid.c >> @@ -15,6 +15,8 @@ >> #include <asm/io.h> >> #include <part_efi.h> >> #include <malloc.h> >> +#include <dm/uclass.h> >> +#include <rng.h> >> >> /* >> * UUID - Universally Unique IDentifier - 128 bits unique number. >> @@ -249,9 +251,22 @@ void gen_rand_uuid(unsigned char *uuid_bin) >> { >> u32 ptr[4]; >> struct uuid *uuid = (struct uuid *)ptr; >> - int i; >> - >> - srand(get_ticks() + rand()); >> + int i, ret; >> + struct udevice *devp; >> + u8 randv = 0; >> + >> +#if defined(CONFIG_DM_RNG) > > This seems a little backwards to me. The caller should request a RNG > device, getting either a hardware one or a software one, and then call > the uclass method to get the uuid. > The problem I see here is, that in case no DM_RNG is present the seed used is different for uuid (get_ticks() + rand()) and bootp (seed_mac() uses the mac address) So we would need to pass this alternatives to the generic DM code, which makes it a bit ugly. Apart from that beware that the seed used for srand() can vary depending on the caller and the entropy it needs. Regards, Matthias >> + ret = uclass_get_device(UCLASS_RNG, 0, &devp); >> + if (ret) { >> + ret = dm_rng_read(devp, &randv, sizeof(randv)); >> + if (ret < 0) >> + randv = 0; >> + } >> + if (randv) >> + srand(randv); >> + else >> +#endif >> + srand(get_ticks() + rand()); >> >> /* Set all fields randomly */ >> for (i = 0; i < 4; i++) >> -- >> 2.29.2 >> > > Regards, > Simon > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] net: Use NDRNG device in srand_mac() 2020-12-16 16:28 [PATCH v2 0/2] Use RNG to get random behaviour matthias.bgg at kernel.org 2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org @ 2020-12-16 16:28 ` matthias.bgg at kernel.org 2020-12-19 2:29 ` Simon Glass 2020-12-18 8:31 ` [PATCH v2 0/2] Use RNG to get random behaviour Torsten Duwe 2 siblings, 1 reply; 16+ messages in thread From: matthias.bgg at kernel.org @ 2020-12-16 16:28 UTC (permalink / raw) To: u-boot From: Matthias Brugger <mbrugger@suse.com> When calling srand_mac we use a weak seed dependent on the mac address. If present, use a RNG device instead to incerase entropy. Signed-off-by: Matthias Brugger <mbrugger@suse.com> --- Changes in v2: - fix dm_rng_read() parameters - add missing include file net/net_rand.h | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/net/net_rand.h b/net/net_rand.h index 4bf9bd817e..e7299973a3 100644 --- a/net/net_rand.h +++ b/net/net_rand.h @@ -10,6 +10,8 @@ #define __NET_RAND_H__ #include <common.h> +#include <dm/uclass.h> +#include <rng.h> /* * Return a seed for the PRNG derived from the eth0 MAC address. @@ -37,7 +39,22 @@ static inline unsigned int seed_mac(void) */ static inline void srand_mac(void) { - srand(seed_mac()); +#if defined(CONFIG_DM_RNG) + int ret; + struct udevice *devp; + u32 randv = 0; + + ret = uclass_get_device(UCLASS_RNG, 0, &devp); + if (ret) { + ret = dm_rng_read(devp, &randv, sizeof(randv)); + if (ret < 0) + randv = 0; + } + if (randv) + srand(randv); + else +#endif + srand(seed_mac()); } #endif /* __NET_RAND_H__ */ -- 2.29.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] net: Use NDRNG device in srand_mac() 2020-12-16 16:28 ` [PATCH v2 2/2] net: Use NDRNG device in srand_mac() matthias.bgg at kernel.org @ 2020-12-19 2:29 ` Simon Glass 0 siblings, 0 replies; 16+ messages in thread From: Simon Glass @ 2020-12-19 2:29 UTC (permalink / raw) To: u-boot Hi Mattias, On Wed, 16 Dec 2020 at 09:28, <matthias.bgg@kernel.org> wrote: > > From: Matthias Brugger <mbrugger@suse.com> > > When calling srand_mac we use a weak seed dependent on the > mac address. If present, use a RNG device instead to incerase entropy. > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > --- > > Changes in v2: > - fix dm_rng_read() parameters > - add missing include file > > net/net_rand.h | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/net/net_rand.h b/net/net_rand.h > index 4bf9bd817e..e7299973a3 100644 > --- a/net/net_rand.h > +++ b/net/net_rand.h > @@ -10,6 +10,8 @@ > #define __NET_RAND_H__ > > #include <common.h> > +#include <dm/uclass.h> > +#include <rng.h> > > /* > * Return a seed for the PRNG derived from the eth0 MAC address. > @@ -37,7 +39,22 @@ static inline unsigned int seed_mac(void) > */ > static inline void srand_mac(void) > { > - srand(seed_mac()); > +#if defined(CONFIG_DM_RNG) if (IS_ENABLED()) Similar comment to the other patch > + int ret; > + struct udevice *devp; > + u32 randv = 0; > + > + ret = uclass_get_device(UCLASS_RNG, 0, &devp); > + if (ret) { > + ret = dm_rng_read(devp, &randv, sizeof(randv)); > + if (ret < 0) > + randv = 0; > + } > + if (randv) > + srand(randv); > + else > +#endif > + srand(seed_mac()); > } > > #endif /* __NET_RAND_H__ */ > -- > 2.29.2 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] Use RNG to get random behaviour 2020-12-16 16:28 [PATCH v2 0/2] Use RNG to get random behaviour matthias.bgg at kernel.org 2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org 2020-12-16 16:28 ` [PATCH v2 2/2] net: Use NDRNG device in srand_mac() matthias.bgg at kernel.org @ 2020-12-18 8:31 ` Torsten Duwe 2 siblings, 0 replies; 16+ messages in thread From: Torsten Duwe @ 2020-12-18 8:31 UTC (permalink / raw) To: u-boot On Wed, 16 Dec 2020 17:28:05 +0100 matthias.bgg at kernel.org wrote: > From: Matthias Brugger <mbrugger@suse.com> > > > For now bootp and uuid code use a weak seed for generating random > data. U-Boot as support for RNG devices now, so we should change to > code to use them if they are present. This will help mitigate issues > like seen in CVE-2019-11690. > > Changes in v2: > - fix dm_rng_read() parameters > - add missing include > - fix dm_rng_read() parameters > - add missing include file One more thing for both patches: https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation If U-Boot also follows these, it should be if (IS_ENABLED(CONFIG_DM_RNG) ) { ... } Instead of #if defined(...). AFAICS, it's easy to see for the compiler that randv is constantly 0 if only the devp usage is made conditional. That would also be more readable. Torsten ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-20 19:56 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-16 16:28 [PATCH v2 0/2] Use RNG to get random behaviour matthias.bgg at kernel.org 2020-12-16 16:28 ` [PATCH v2 1/2] lib: uuid: use RNG device if present matthias.bgg at kernel.org 2020-12-18 8:37 ` Torsten Duwe 2020-12-18 9:10 ` mbrugger 2020-12-19 2:29 ` Simon Glass 2020-12-20 16:59 ` Torsten Duwe 2020-12-20 18:17 ` Simon Glass 2020-12-23 10:24 ` Torsten Duwe 2020-12-29 3:32 ` Simon Glass 2021-01-20 11:17 ` Matthias Brugger 2021-01-20 19:50 ` Simon Glass 2021-01-20 19:56 ` Matthias Brugger 2020-12-23 10:09 ` Matthias Brugger 2020-12-16 16:28 ` [PATCH v2 2/2] net: Use NDRNG device in srand_mac() matthias.bgg at kernel.org 2020-12-19 2:29 ` Simon Glass 2020-12-18 8:31 ` [PATCH v2 0/2] Use RNG to get random behaviour Torsten Duwe
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.