Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if it
exposes a virtio-rng device. This means that generic EFI apps or
loaders have no access to an entropy source if this device is
unavailable, unless they implement their own arch-specific handling to
figure out whether any CPU instructions or monitor calls can be used
instead.

So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
using the existing drivers and libraries.

First patch is a bugfix - Liming, mind if I merge that right away?
Thanks.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>

Ard Biesheuvel (3):
  ArmPkg/ArmTrngLib: Fix incorrect GUID reference in DEBUG() output
  ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if
    implemented
  OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

 ArmPkg/Library/ArmTrngLib/ArmTrngLib.c |  2 +-
 ArmVirtPkg/ArmVirtQemu.dsc             | 11 +++++++++++
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc   |  5 +++++
 ArmVirtPkg/ArmVirtQemuKernel.dsc       | 11 +++++++++++
 OvmfPkg/OvmfPkgIa32.dsc                |  1 +
 OvmfPkg/OvmfPkgIa32.fdf                |  1 +
 OvmfPkg/OvmfPkgIa32X64.dsc             |  1 +
 OvmfPkg/OvmfPkgIa32X64.fdf             |  1 +
 OvmfPkg/OvmfPkgX64.dsc                 |  1 +
 OvmfPkg/OvmfPkgX64.fdf                 |  1 +
 10 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.35.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96188): https://edk2.groups.io/g/devel/message/96188
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Ard,

On Thu, Nov 10, 2022 at 2:48 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if it
> exposes a virtio-rng device. This means that generic EFI apps or
> loaders have no access to an entropy source if this device is
> unavailable, unless they implement their own arch-specific handling to
> figure out whether any CPU instructions or monitor calls can be used
> instead.
>
> So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
> using the existing drivers and libraries.

I tested this series on x86 and it appears to work as expected. Thanks
for putting this together. So,

    Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>

On very brief inspection, this also looks good, though I'm not really
an EDK2 expert and my review isn't very thorough. But in case it
helps, which you can take or leave,

    Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

My only question is how it chooses which RNG source to use in the
event that multiple are available. I would think preferring virtio-rng
if available is the right thing there. If it's based on the order of
the items in the .dsc file, then it looks like this series is doing
the right thing.

Jason


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96308): https://edk2.groups.io/g/devel/message/96308
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, 11 Nov 2022 at 03:41, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Thu, Nov 10, 2022 at 2:48 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if it
> > exposes a virtio-rng device. This means that generic EFI apps or
> > loaders have no access to an entropy source if this device is
> > unavailable, unless they implement their own arch-specific handling to
> > figure out whether any CPU instructions or monitor calls can be used
> > instead.
> >
> > So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
> > using the existing drivers and libraries.
>
> I tested this series on x86 and it appears to work as expected. Thanks
> for putting this together. So,
>
>     Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> On very brief inspection, this also looks good, though I'm not really
> an EDK2 expert and my review isn't very thorough. But in case it
> helps, which you can take or leave,
>
>     Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
>

Thanks.

> My only question is how it chooses which RNG source to use in the
> event that multiple are available. I would think preferring virtio-rng
> if available is the right thing there. If it's based on the order of
> the items in the .dsc file, then it looks like this series is doing
> the right thing.
>

No, it is essentially arbitrary (but not random :-))

We already have special handling for the virtio RNG device in the BDS
code, because normally, EFI only dispatches drivers for devices that
it needs to boot (i..e, it walks the device path of the boot entry and
only connects a device to its driver at each stage if it needs to do
so to get to the next one)

So connecting the virtio-rng device to its driver needs to be done
explicitly, or it may not be connected at all. We handle this in
ConnectVirtioPciRng() for x86 and some similar code exists in
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c

On ARM, the RngDxe wired up by this patch is backed by a hypervisor or
secure world firmware service, rather than by the VMM, so in the ARM
case, I think this one is the preferred one given that the VMM is
generally less trusted (although that distinction really only matters
for confidential compute).

On x86, we use the RdRand instruction, which is also independent from
the VMM, so I'd assume this is the preferred choice, no? Or do you
have concerns about broken implementations?

Another distinction is that the ARM version only implements
EFI_RNG_ALGORITHM_RAW, whereas x86 also implements
EFI_RNG_ALGORITHM_SP800_90_CTR_256_GUID. (virtio-rng also only
implements EFI_RNG_ALGORITHM_RAW). This likely does not matter at all,
but it is nevertheless good to call out while we decide which driver
to give precedence.

Another thing to note is that we generally try very hard to do as
little as possible at boot time (although you might get a different
impression when looking at the code :-)). So simply skipping the
virtio-rng driver dispatch if some implementation of EFI_RNG_PROTOCOL
is already available seems appropriate from that angle as well.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96251): https://edk2.groups.io/g/devel/message/96251
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Ard,

On Fri, Nov 11, 2022 at 8:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 11 Nov 2022 at 03:41, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Ard,
> >
> > On Thu, Nov 10, 2022 at 2:48 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if it
> > > exposes a virtio-rng device. This means that generic EFI apps or
> > > loaders have no access to an entropy source if this device is
> > > unavailable, unless they implement their own arch-specific handling to
> > > figure out whether any CPU instructions or monitor calls can be used
> > > instead.
> > >
> > > So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
> > > using the existing drivers and libraries.
> >
> > I tested this series on x86 and it appears to work as expected. Thanks
> > for putting this together. So,
> >
> >     Tested-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
> > On very brief inspection, this also looks good, though I'm not really
> > an EDK2 expert and my review isn't very thorough. But in case it
> > helps, which you can take or leave,
> >
> >     Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
>
> Thanks.
>
> > My only question is how it chooses which RNG source to use in the
> > event that multiple are available. I would think preferring virtio-rng
> > if available is the right thing there. If it's based on the order of
> > the items in the .dsc file, then it looks like this series is doing
> > the right thing.
> >
>
> No, it is essentially arbitrary (but not random :-))
>
> We already have special handling for the virtio RNG device in the BDS
> code, because normally, EFI only dispatches drivers for devices that
> it needs to boot (i..e, it walks the device path of the boot entry and
> only connects a device to its driver at each stage if it needs to do
> so to get to the next one)
>
> So connecting the virtio-rng device to its driver needs to be done
> explicitly, or it may not be connected at all. We handle this in
> ConnectVirtioPciRng() for x86 and some similar code exists in
> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
>
> On ARM, the RngDxe wired up by this patch is backed by a hypervisor or
> secure world firmware service, rather than by the VMM, so in the ARM
> case, I think this one is the preferred one given that the VMM is
> generally less trusted (although that distinction really only matters
> for confidential compute).
>
> On x86, we use the RdRand instruction, which is also independent from
> the VMM, so I'd assume this is the preferred choice, no? Or do you
> have concerns about broken implementations?
>
> Another distinction is that the ARM version only implements
> EFI_RNG_ALGORITHM_RAW, whereas x86 also implements
> EFI_RNG_ALGORITHM_SP800_90_CTR_256_GUID. (virtio-rng also only
> implements EFI_RNG_ALGORITHM_RAW). This likely does not matter at all,
> but it is nevertheless good to call out while we decide which driver
> to give precedence.
>
> Another thing to note is that we generally try very hard to do as
> little as possible at boot time (although you might get a different
> impression when looking at the code :-)). So simply skipping the
> virtio-rng driver dispatch if some implementation of EFI_RNG_PROTOCOL
> is already available seems appropriate from that angle as well.

That all seems reasonable to me, your arguments about secure world
etc. So in case there's another rng dxe available, we can just skip
the (more expensive) virtio initialization, and this will make things
generally simpler too. Sounds alright.

Jason


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96309): https://edk2.groups.io/g/devel/message/96309
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Nov 10, 2022 at 02:47:35PM +0100, Ard Biesheuvel wrote:
> Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if it
> exposes a virtio-rng device. This means that generic EFI apps or
> loaders have no access to an entropy source if this device is
> unavailable, unless they implement their own arch-specific handling to
> figure out whether any CPU instructions or monitor calls can be used
> instead.
> 
> So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
> using the existing drivers and libraries.
> 
> First patch is a bugfix - Liming, mind if I merge that right away?
> Thanks.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Ard Biesheuvel (3):
>   ArmPkg/ArmTrngLib: Fix incorrect GUID reference in DEBUG() output
>   ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if
>     implemented
>   OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

Series looks good to me (not tested though).

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96253): https://edk2.groups.io/g/devel/message/96253
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Could we get this merged?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98254): https://edk2.groups.io/g/devel/message/98254
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

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.

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.

Thanks for considering,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98297): https://edk2.groups.io/g/devel/message/98297
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
(- 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98299): https://edk2.groups.io/g/devel/message/98299
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

> (- 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98300): https://edk2.groups.io/g/devel/message/98300
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98339): https://edk2.groups.io/g/devel/message/98339
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 10 Jan 2023 at 19:19, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Could we get this merged?

Thanks for the ping. I hope to get back to this shortly.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98294): https://edk2.groups.io/g/devel/message/98294
Mute This Topic: https://groups.io/mt/94935839/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Ard:
  The first patch is the bug fix. I agree to merge it for this stable tag. I
will help merge it today. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2022年11月10日 21:48
> 收件人: devel@edk2.groups.io
> 抄送: Ard Biesheuvel <ardb@kernel.org>; Liming Gao
> <gaoliming@byosoft.com.cn>; Rebecca Cran <rebecca@bsdio.com>; Pierre
> Gondois <pierre.gondois@arm.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Jason A . Donenfeld
> <Jason@zx2c4.com>
> 主题: [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
> 
> Currently, we only expose EFI_RNG_PROTOCOL when running under QEMU if
> it
> 
> exposes a virtio-rng device. This means that generic EFI apps or
> 
> loaders have no access to an entropy source if this device is
> 
> unavailable, unless they implement their own arch-specific handling to
> 
> figure out whether any CPU instructions or monitor calls can be used
> 
> instead.
> 
> 
> 
> So let's wire those up as EFI_RNG_PROTOCOL implementations as well,
> 
> using the existing drivers and libraries.
> 
> 
> 
> First patch is a bugfix - Liming, mind if I merge that right away?
> 
> Thanks.
> 
> 
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Cc: Rebecca Cran <rebecca@bsdio.com>
> 
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> 
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> 
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> 
> 
> Ard Biesheuvel (3):
> 
>   ArmPkg/ArmTrngLib: Fix incorrect GUID reference in DEBUG() output
> 
>   ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if
> 
>     implemented
> 
>   OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL
> implementation
> 
> 
> 
>  ArmPkg/Library/ArmTrngLib/ArmTrngLib.c |  2 +-
> 
>  ArmVirtPkg/ArmVirtQemu.dsc             | 11 +++++++++++
> 
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc   |  5 +++++
> 
>  ArmVirtPkg/ArmVirtQemuKernel.dsc       | 11 +++++++++++
> 
>  OvmfPkg/OvmfPkgIa32.dsc                |  1 +
> 
>  OvmfPkg/OvmfPkgIa32.fdf                |  1 +
> 
>  OvmfPkg/OvmfPkgIa32X64.dsc             |  1 +
> 
>  OvmfPkg/OvmfPkgIa32X64.fdf             |  1 +
> 
>  OvmfPkg/OvmfPkgX64.dsc                 |  1 +
> 
>  OvmfPkg/OvmfPkgX64.fdf                 |  1 +
> 
>  10 files changed, 34 insertions(+), 1 deletion(-)
> 
> 
> 
> --
> 
> 2.35.1
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96212): https://edk2.groups.io/g/devel/message/96212
Mute This Topic: https://groups.io/mt/94949625/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-