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.