Re: [edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng

["Laszlo Ersek" <lersek@redhat.com>]

On 1/11/23 17:05, Ard Biesheuvel wrote:
> On Wed, 11 Jan 2023 at 17:03, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 11 Jan 2023 at 16:23, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> On 1/10/23 19:19, Jason A. Donenfeld via groups.io wrote:
>>>> Could we get this merged?
>>>
>>> Sorry to barge in -- I have *zero* complaints regarding this particular
>>> series, so whatever I'm about to say regards *further* BDS
>>> customizations. Please feel free to go ahead with merging this one, as
>>> far as I'm concerned.
>>>
>>
>> Thanks.
>>
>>> So, picking up the thread at
>>> <https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055607.html>.
>>> The argument in that thread was made that "RDRAND-based protocol is
>>> better than nothing". However, the most recent idea, favoring the
>>> RDRAND-based protocol implementation over the virtio-rng-based one,
>>> seems to enable a degradation too, of EFI-time randomness.
>>>
>>> Most commonly, virtio-rng is fed on the host side from /dev/urandom,
>>> which *I think* means that the EFI_RNG_PROTOCOL from VirtioRngDxe will
>>> expose all the "good quality entropy", pre-boot, that the host-side
>>> Linux kernel collects from *multiple* sources. If the consumer of
>>> EFI_RNG_PROTOCOL in the guest doesn't do its own mixing, it sill gets
>>> the good stuff. That could potentially be degraded by relying on RDRAND
>>> only, in the guest.
>>>
>>
>> Indeed.
>>
>>> I can't propose any particular priority ordering mechanism for the
>>> platform firmware to produce exactly one EFI_RNG_PROTOCOL.
>>>
>>> Normally I'd suggest any viable mechanism for the platform to block or
>>> to delay "SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf" --
>>> introducing a new dynamic PCD for early exit, adding a new protocol
>>> dependency to its DEPEX, postponing its protocol installation to an
>>> event group notification function or a protocol installation
>>> notification. Note that RngDxe.inf is a DXE_DRIVER, so it produces its
>>> protocol in its entry point function, so for blocking it or
>>> short-circuiting it, one of these measures would be needed. It could
>>> even be turned into a UEFI_DRIVER, one that would bind a synthetic VenHw
>>> device path.
>>>
>>> But, I'm not proposing any of those right now, because I imagine there
>>> are advantages to having EFI_RNG_PROTOCOL in the DXE phase, that is,
>>> *before* the BDS phase.
>>>
>>> VirtioRngDxe is a UEFI_DRIVER module that follows the UEFI driver model;
>>> in other words, it won't do anything beyond exposing the
>>> EFI_DRIVER_BINDING_PROTOCOL until BDS connects it. I think that should
>>> be sufficient for most cases, even (for example) possibly providing
>>> randomness for TLS in UEFI HTTPS Boot. But I vaguely remember we had
>>> wished for randomness being available earlier than BDS.
>>> "SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf" can fill that
>>> role, VirtioRngDxe can't.
>>>
>>> So best would be if both could coexist, and VirtioRngDxe took effect
>>> *whenever* it were available. Of course the UEFI spec allows for a
>>> client to collect all instances of EFI_RNG_PROTOCOL, and then to call
>>> GetInfo() on each, but that's hardly enough for a client to pick the one
>>> it thinks is "more secure". So one way or another we might want to
>>> control this still at the platform level, where we can form ideas about
>>> both protocol providers, *and* perhaps even determine if we *actually
>>> need* pre-BDS randomness.
>>>
>>> BDS could try connecting the virtio-rng device. If that failed, it could
>>> try "unblocking" RngDxe. If RngDxe were a UEFI driver following the UEFI
>>> driver model (see the VenHw option above), this would not be hard to do,
>>> with a "fallback" gBS->ConnectController() call.
>>>
>>> (Regarding VenHw vs. VenMedia vs. VenMsg -- RngDxe uses an RNG that's
>>> built into the processor, wich is arguably "inside the resource domain"
>>> of the system. So VenHw seems the right choice.)
>>>
>>> RngDxe could perhaps be restructured for the addition of a new entry
>>> point (new INF file and new entry point C file), so that it remain
>>> compatible with existent platforms that already consume it (and want it
>>> to remain a DXE_DRIVER).
>>>
>>> BDS could also signal an event group or install a synthetic protocol, so
>>> that the notification function in RngDxe expose EFI_RNG_PROTOCOL in
>>> response.
>>>
>>> Unblocking a DXE_DRIVER's DEPEX from BDS seems more cumbersome, by
>>> installing a dependend-upon synthetic protocol; I believe we might have
>>> to call gDS->Dispatch() manually then.
>>>
>>> And if a dynamic PCD caused RngDxe to exit early, we couldn't undo that
>>> from BDS at all.
>>>
>>
>> One option that might be feasible would be to modify VIrtioRngDxe so it:
>> - installs a RNG protocol implementation solely based on [Base]RngLib
>> when it is dispatched
>> - uninstalls it again when it binds to the first virtio-rng device
>> - reinstalls it when it unbinds from the last virtio-rng device it was bound to
>> - installs the virtio-rng backed flavor of the RNG protocol when
>> binding to a device
> 
> (Un)installing the protocol is a bit problematic, as a caller may hold
> a reference. Probably better to expose a single implementation from
> VirtioRngDxe, and back it with whatever is available at the time of
> the call.

Probably so, yes.

(But that shouldn't block this series from being merged -- let me
confirm that again.)

Thanks!
Laszlo

> 
>> (- mixes the output of the latter with the RngLIb based implementation)
>>
>> I think this would address all of these concerns, assuming that the
>> mixing is done correctly.
>>
>> *However*, I am not convinced that any of this is worth the hassle,
>> tbh. If you don't trust your CPU, all bets are off anyway - the only
>> thing we'd need to cater for is an explicit opt-out for known broken
>> implementations of RdRand.
>