Page MenuHomeFreeBSD

Add support for getting early entropy from the UEFI RNG protocol
ClosedPublic

Authored by val_packett.cool on Jun 27 2019, 2:39 PM.
Referenced Files
Unknown Object (File)
Sun, Dec 22, 7:11 PM
Unknown Object (File)
Sun, Dec 8, 12:36 AM
Unknown Object (File)
Sun, Dec 8, 12:36 AM
Unknown Object (File)
Sun, Dec 8, 12:35 AM
Unknown Object (File)
Fri, Dec 6, 4:46 AM
Unknown Object (File)
Fri, Dec 6, 4:39 AM
Unknown Object (File)
Fri, Dec 6, 4:39 AM
Unknown Object (File)
Fri, Dec 6, 4:39 AM

Details

Summary

UEFI provides a protocol for accessing randomness. This is a good way to gather early entropy, especially when there's no driver for the RNG on the platform (as is the case on the Marvell Armada8k (MACCHIATObin) for now).

FreeBSD already has a way to pass entropy from the loader to the kernel: via loading a module with a special type. (I was searching for where the entropy_cache_name etc. variables are used for quite some time before realizing it works dynamically together with regular modules :D)

With a little modification to module.c, we can allow other parts of the code to load modules from anywhere (not just the FS), and use this in the EFI-specific code to load a synthetic module with the entropy.


references:

Test Plan

Tested on: Marvell MACCHIATObin, Lenovo Thinkpad X240

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40691
Build 37580: arc lint + arc unit

Event Timeline

cperciva added inline comments.
stand/defaults/loader.conf
51

The syntax "disable X instead of doing Y" seems confusing to me.

Also, why would we not want to just read both sources of entropy by default?

Presuming all the testing works :-)

This revision is now accepted and ready to land.Jun 27 2019, 6:26 PM
stand/defaults/loader.conf
51

Because random_harvestq_prime currently looks for just one loaded file and I don't really want to modify that code.

stand/defaults/loader.conf
51

I don't think this should be enabled by default if it replaces the other, more trustworthy source. Early seeding is sensitive and nothing I've seen about EFI or BIOSes suggests that we should trust some implementation of the EFI RNG protocol as entropy.

stand/defaults/loader.conf
51

I took a look at the EDK2 implementation, and for x86 it uses RDRAND, while for aarch64 it uses the machine specific functionality, such as TRNG on Marvell Aramada devices.

delphij added inline comments.
stand/defaults/loader.conf
51

Stored entropy cache has to be used if entropy_cache_load="YES" and when they are available. Please amend random_harvestq_prime to support it.

This revision now requires changes to proceed.Jun 27 2019, 10:22 PM
stand/defaults/loader.conf
51

I agree with cem and delphij -- EFI implementations do not have enough of a reputation for correctness that I would want to trust our security to their ability to implement an RNG. It would not be the first time that a RNG produced non-random numbers.

Changed to use the file_addbuf function — no more modifications to stand/common/module \o/

Changed the loader.conf setting and disabled by default. It should probably be enabled in e.g. aarch64 EC2 images.

stand/defaults/loader.conf
53

Is there a reason why we can't just fix this and support multiple entropy files?

stand/defaults/loader.conf
51

I agree with cem and delphij -- EFI implementations do not have enough of a reputation for correctness that I would want to trust our security to their ability to implement an RNG. It would not be the first time that a RNG produced non-random numbers.

Agreed, but this "just" means we shouldn't rely on EFI entropy alone. It's OK to use it *in addition*.

53

No reason, in my opinion. Just A Small Matter of Coding™.

stand/defaults/loader.conf
51

Linux now unconditionally uses this interface on x86 (been using it on arm64 for a while):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0d95981438c3bdb53cc99b0fb656d24d7a80e1f3

They seem to have a build time flag CONFIG_RANDOM_TRUST_BOOTLOADER that decides whether to *count* this towards their entropy count (but it's *used* either way)

53

oh, actually it is small. I thought it was hard at first, but looking more closely, there's 2 possible solutions:

  • turn random_prime_loader_file into random_prime_loader_files, looping over all modules (found an example of that in the codebase already) — funnily enough would make its comment more true, as it already says "loader-loaded files" plural
  • add another type next to RANDOM_CACHED_BOOT_ENTROPY_MODULE ("boot_entropy_cache"), call it "platform" instead of "cache" I guess, and just random_prime_loader_file again with that type
    • this would allow more accurate bootverbose messages (not "read %zu bytes from preloaded cache" anymore)
    • and potentially treating it as a different RANDOM_ kind than CACHED in the future if there's ever a reason to

I think I'll go with the second one, as it is more semantically correct in some ways.

Now loading with a separate preload type

Testing this on an RPi4 would be nice, but it's not a dealbreaker.

stand/efi/include/efirng.h
1

This file needs a copyright / license at the top.

stand/efi/include/efirng.h
1

Which copyright should it be? Most of these interface definitions seem to have Intel…

stand/lua/config.lua
732

hm, can I call core.isUEFIBoot() here? just requiring core in config.lua should be fine, right?

stand/lua/config.lua
732

I believe so for the first part, and definitely for the second part.

Oops, didn't meant to block after the amendment.

(I'd probably fold the logic to detect/complain the no entropy can be loaded from loader into random_prime_loader_file, but that can always be done in a separate change).

This revision is now accepted and ready to land.Sep 6 2021, 12:58 AM
kevans added inline comments.
stand/lua/config.lua
732

No, we can't do circular dependencies like that. IMO we should probably split some core stuff out into a system.lua (e.g. isI386, isUEFIBoot) with no dependencies.

Is this waiting for anything else before it gets committed?

Is this waiting for anything else before it gets committed?

A committer? For some reason, I thought this was committated many moons ago.

I'll do it tomorrow, Sunday, 16 Jan 2022.

Thanks! Can you also MFC it to stable/13 after a week?

err, I have not addressed the "isUEFIBoot" thing and the "This file needs a copyright / license at the top" thing…

In D20780#766778, @greg_unrelenting.technology wrote:

err, I have not addressed the "isUEFIBoot" thing and the "This file needs a copyright / license at the top" thing…

OK - Whenever you are ready!

So seems like it's easier to just do it all in core.lua, which is where lots of config accesses are anyway.

This revision now requires review to proceed.Jan 26 2022, 9:30 PM
stand/efi/include/efirng.h
1

Which copyright should it be? Most of these interface definitions seem to have Intel…

BSD 2-clause here, maybe. Assign to BSD Foundation if you don't want it for yourself?

delphij added inline comments.
stand/efi/include/efirng.h
1

My $0.02 -- IANAL but I think these (constants and interface data structures) are "fact"s and is probably not copyrightable. To err on the safe side, however, I'd probably use Intel copyright as the file appears to be substantially identical to edk2's [RandomNumber.h](https://github.com/tianocore/edk2-test/blame/master/uefi-sct/SctPkg/UEFI/Protocol/RandomNumber.h)

This revision is now accepted and ready to land.Jan 27 2022, 5:52 PM

Yep, I've had basically the exact same opinion as @delphij about the copyright. Let's go with Intel.

Anyway, I guess this can go in now unless there's more feedback about either the core.lua thing or the C parts?

format-patch against upstream main with no extra context: P536

This revision now requires review to proceed.Jan 28 2022, 11:59 AM
stand/efi/include/efirng.h
1

IIRC, there is a problem with copyright-less files, as lawyers get touchy about this in large corporations.

If it falls through to the top-level copyright, then I guess it's OK.

This revision is now accepted and ready to land.Jan 29 2022, 10:17 AM

Is this ready to be committed? I'm happy to do it myself but markm said he was going to commit (prior to the latest round of changes) -- don't want to commit prematurely if you're still waiting for something.

stand/lua/core.lua
383 ↗(On Diff #102047)

I think we want both of these to be called only in the same block just above with loadelf(); if the kernel is loaded already, we specifically avoid trying to load anything else in case the user deliberately avoided it. e.g., disaster scenario and for some reason kernel-side of processing this entropy is hosing us, we give the user an easy path out.

Last nit can be done pre-commit or I can whack it post-commit; ok from lua perspective.

Who is going to do the actual commit? I'm happy to do it if no-one else wants to? Whoever does it has csprng@ green-light.

I'll do the commit. Thanks to Greg for writing this and to everyone who helped to review it!