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

Ard Biesheuvel posted 3 patches 4 weeks ago
Failed in applying to current master (apply log)
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(-)
[edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by Ard Biesheuvel 4 weeks ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by Jason A. Donenfeld via groups.io 3 weeks, 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by Ard Biesheuvel 3 weeks, 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by Jason A. Donenfeld via groups.io 3 weeks, 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by Gerd Hoffmann 3 weeks, 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] 回复: [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng
Posted by gaoliming via groups.io 3 weeks, 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-