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

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

[Ard Biesheuvel]

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

[PATCH 1/3] ArmPkg/ArmTrngLib: Fix incorrect GUID reference in DEBUG() output

[Ard Biesheuvel]

ArmTrngLib crashes when run in DEBUG mode due to the fact that it passed
the [truncated] GUID value to a DEBUG() print statement instead of a
pointer to the GUID which is what the %g conversion expects.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmTrngLib/ArmTrngLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
index b974a9423880..fdabc02cd39c 100644
--- a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
+++ b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
@@ -375,7 +375,7 @@ ArmTrngLibConstructor (
     "FW-TRNG: Version %d.%d, GUID {%g}\n",
     MajorRev,
     MinorRev,
-    Guid
+    &Guid
     ));
 
   DEBUG_CODE_END ();
-- 
2.35.1

[PATCH 2/3] ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if implemented

[Ard Biesheuvel]

Currently, we only expose the EFI_RNG_PROTOCOL in ArmVirtQemu if QEMU
provides a virtio-rng device, and it doesn't do so by default.

Given that KVM exposes the ARM architected TRNG service (and has done so
for a while now), let's incorporate the RngDxe driver which has recently
grown support for the ARM firmware/hypervisor service.

If both the service and the virtio device are available, two
implementations of the RNG protocol will be exposed, but this is fine:
callers that don't care about the distinction will grab the first one
available.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc           | 11 +++++++++++
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
 ArmVirtPkg/ArmVirtQemuKernel.dsc     | 11 +++++++++++
 3 files changed, 27 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index f77443229e8e..1771ad562225 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -140,6 +140,8 @@ [PcdsFeatureFlag.common]
 
   gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
 
+  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
@@ -442,6 +444,15 @@ [Components.common]
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
   OvmfPkg/VirtioRngDxe/VirtioRng.inf
 
+  #
+  # Rng Support
+  #
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {
+    <LibraryClasses>
+      ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
+      ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
+  }
+
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
   #
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index e06ca7424476..75c75a2d9a17 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -99,6 +99,11 @@ [FV.FvMain]
   INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
   INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
 
+  #
+  # Rng Support
+  #
+  INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
+
   INF ShellPkg/Application/Shell/Shell.inf
   INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
   INF ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index f5db3ac432f3..abe0cbab8295 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -114,6 +114,8 @@ [PcdsFeatureFlag.common]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
 
+  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
@@ -350,6 +352,15 @@ [Components.common]
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
   OvmfPkg/VirtioRngDxe/VirtioRng.inf
 
+  #
+  # Rng Support
+  #
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {
+    <LibraryClasses>
+      ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
+      ArmTrngLib|ArmPkg/Library/ArmTrngLib/ArmTrngLib.inf
+  }
+
   #
   # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
   #
-- 
2.35.1

[PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Ard Biesheuvel]

Expose the EFI_RNG_PROTOCOL based on RdRand, so that we don't have to
rely on QEMU providing a virtio-rng device in order to implement this
protocol.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 1 +
 OvmfPkg/OvmfPkgIa32.fdf    | 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
 OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
 OvmfPkg/OvmfPkgX64.dsc     | 1 +
 OvmfPkg/OvmfPkgX64.fdf     | 1 +
 6 files changed, 6 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index e9ba491237ae..18c1e7255812 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -941,6 +941,7 @@ [Components]
   }
 !endif
 
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
   OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 7023ade8cebe..34f27ca832bc 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -248,6 +248,7 @@ [FV.DXEFV]
 INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
 !endif
 
+  INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index af566b953f36..e9a199c9f490 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -955,6 +955,7 @@ [Components.X64]
   }
 !endif
 
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
   OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 80de4fa2c0df..33cc163e596e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -249,6 +249,7 @@ [FV.DXEFV]
 INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
 !endif
 
+  INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f39d9cd117e6..5572cb82998f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -1023,6 +1023,7 @@ [Components]
   }
 !endif
 
+  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
   OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index c0f5a1ef3c30..d42deebe3f8f 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -274,6 +274,7 @@ [FV.DXEFV]
 INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
 !endif
 
+INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
 !endif
-- 
2.35.1

Re: [PATCH 1/3] ArmPkg/ArmTrngLib: Fix incorrect GUID reference in DEBUG() output

[Sami Mujawar]

Hi Ard,

Thank you for this fix.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 10/11/2022, 13:48, "Ard Biesheuvel" <ardb@kernel.org> wrote:

    ArmTrngLib crashes when run in DEBUG mode due to the fact that it passed
    the [truncated] GUID value to a DEBUG() print statement instead of a
    pointer to the GUID which is what the %g conversion expects.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
    ---
     ArmPkg/Library/ArmTrngLib/ArmTrngLib.c | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
    index b974a9423880..fdabc02cd39c 100644
    --- a/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
    +++ b/ArmPkg/Library/ArmTrngLib/ArmTrngLib.c
    @@ -375,7 +375,7 @@ ArmTrngLibConstructor (
         "FW-TRNG: Version %d.%d, GUID {%g}\n",

         MajorRev,

         MinorRev,

    -    Guid

    +    &Guid

         ));



       DEBUG_CODE_END ();

    -- 
    2.35.1

回复: [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL without virtio-rng

[gaoliming]

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

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

[Jason A. Donenfeld]

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

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

[gaoliming]

It has been merged at c8fb7240469b5753773da4fa5710b870b790c363

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 gaoliming via
> groups.io
> 发送时间: 2022年11月11日 8:42
> 收件人: 'Ard Biesheuvel' <ardb@kernel.org>; devel@edk2.groups.io
> 抄送: '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>
> 主题: [edk2-devel] 回复: [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL
> without virtio-rng
> 
> 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
> >
> >
> 
> 
> 
> 
> 
> 
> 

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

[Ard Biesheuvel]

On Fri, 11 Nov 2022 at 04:53, gaoliming via groups.io
<gaoliming=byosoft.com.cn@groups.io> wrote:
>
> It has been merged at c8fb7240469b5753773da4fa5710b870b790c363
>
> Thanks
> Liming

Thank you.

I'll merge the remaining ones after the stable tag has been created.


> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 gaoliming via
> > groups.io
> > 发送时间: 2022年11月11日 8:42
> > 收件人: 'Ard Biesheuvel' <ardb@kernel.org>; devel@edk2.groups.io
> > 抄送: '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>
> > 主题: [edk2-devel] 回复: [PATCH 0/3] OVMF: support EFI_RNG_PROTOCOL
> > without virtio-rng
> >
> > 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
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
>
>
>
>
>
> 
>
>

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

[Ard Biesheuvel]

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.

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

[Gerd Hoffmann]

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

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

[Jason A. Donenfeld]

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

Re: [PATCH 2/3] ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if implemented

[PierreGondois]

Hello Ard,

On 11/10/22 14:47, Ard Biesheuvel wrote:
> Currently, we only expose the EFI_RNG_PROTOCOL in ArmVirtQemu if QEMU
> provides a virtio-rng device, and it doesn't do so by default.
> 
> Given that KVM exposes the ARM architected TRNG service (and has done so
> for a while now), let's incorporate the RngDxe driver which has recently
> grown support for the ARM firmware/hypervisor service.
> 
> If both the service and the virtio device are available, two
> implementations of the RNG protocol will be exposed, but this is fine:
> callers that don't care about the distinction will grab the first one
> available.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   ArmVirtPkg/ArmVirtQemu.dsc           | 11 +++++++++++
>   ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
>   ArmVirtPkg/ArmVirtQemuKernel.dsc     | 11 +++++++++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index f77443229e8e..1771ad562225 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -140,6 +140,8 @@ [PcdsFeatureFlag.common]
>   
>     gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
>   
> +  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
> +

It seems that the PSCI conduit needs to be dynamically set.
The psci conduit that should be used is configured by qemu depending on the
virtualization=[on|off] parameter. When off, HVC must be used (SMC otherwise).
Cf:
https://github.com/qemu/qemu/blob/master/hw/arm/virt.c#L2052

If using the wrong conduit, qemu traps the instruction and stops.
For KvmTool, the conduit is always HVC.

Command used:
[PATH_TO]/qemu/build/qemu-system-aarch64 \
	-serial stdio -M virt,highmem=on,virtualization=off \
	-cpu cortex-a57 -smp 4 -m 4096 \
	-drive file=pflash0.img,format=raw,if=pflash,readonly=on \
	-drive file=pflash1.img,format=raw,if=pflash

Regards,
Pierre

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 4195 bytes --]

Hi Ard,

Given this patch plus the corresponding linux-efi patches wrt RNG, I'm
mildly concerned about buggy RDRAND implementations compromising the
kernel's RNG. Is this not a concern?

It's also worth noting that MdePkg/Library/BaseRngLib skips the CPUID bit
check in ArchIsRngSupported for $REASON, which I assume will crash
pre-RDRAND VMs.
We should probably also test for stupidly broken rdrand implementations
like the notorious Zen 3 which always return 0xFFFFFFFF (per xkcd 221 ;)).

Thanks,
Pedro

On Thu, Nov 10, 2022 at 1:48 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> Expose the EFI_RNG_PROTOCOL based on RdRand, so that we don't have to
> rely on QEMU providing a virtio-rng device in order to implement this
> protocol.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 1 +
>  OvmfPkg/OvmfPkgIa32.fdf    | 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
>  OvmfPkg/OvmfPkgX64.dsc     | 1 +
>  OvmfPkg/OvmfPkgX64.fdf     | 1 +
>  6 files changed, 6 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index e9ba491237ae..18c1e7255812 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -941,6 +941,7 @@ [Components]
>    }
>  !endif
>
> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>    OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 7023ade8cebe..34f27ca832bc 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -248,6 +248,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
>  !endif
>
> +  INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    INF
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index af566b953f36..e9a199c9f490 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -955,6 +955,7 @@ [Components.X64]
>    }
>  !endif
>
> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>    OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 80de4fa2c0df..33cc163e596e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -249,6 +249,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
>  !endif
>
> +  INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    INF
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index f39d9cd117e6..5572cb82998f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -1023,6 +1023,7 @@ [Components]
>    }
>  !endif
>
> +  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>    OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index c0f5a1ef3c30..d42deebe3f8f 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -274,6 +274,7 @@ [FV.DXEFV]
>  INF  OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf
>  !endif
>
> +INF  SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    INF
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>  !endif
> --
> 2.35.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96191): https://edk2.groups.io/g/devel/message/96191
> Mute This Topic: https://groups.io/mt/94935843/5946980
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [pedro.falcato@gmail.com
> ]
> ------------
>
>
>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 5506 bytes --]

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Jason A. Donenfeld]

Hi Pedro,

On Tue, Nov 22, 2022 at 12:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> Given this patch plus the corresponding linux-efi patches wrt RNG, I'm
> mildly concerned about buggy RDRAND implementations compromising the
> kernel's RNG. Is this not a concern?

Speaking with my kernel RNG maintainer hat on, no, this is not really a
concern, for several reasons:

- The kernel's RNG takes input from multiple sources, continuously, and
  tries to mix in new inputs rather quickly, especially at early boot.

- The kernel will use RDRAND on its own, even in the case that EFI
  doesn't provide it, so it's not gaining anything here.

- EFI on actual baremetal firmware, as opposed to OVMF, already provides
  EFI, so this is par for the course.

- Most of those RDRAND bugs have concerned coming in and out of various
  sleep states, which doesn't really apply to early boot EFI.

- And again, just to reinforce the first point, the kernel takes inputs
  from many sources. Having EFI provide its own thing -- via RDRAND or
  any other mechanism -- is complementary and will only help.

Regarding your "corresponding linux-efi patches wrt RNG", I'm not quite
sure what you're referring to. If anything, recent work during this
cycle has been aimed around shuffling more sources of randomness in from
elsewhere. The EFI RNG protocol stuff has been in there already for a
long time. So maybe you misunderstood those? Or I'm misunderstanding
what you're referring to?

As a general point, this question of "do we have enough entropy?" or
"are we initialized yet?" is an impossible proposition. Entropy
estimation is impossible, and is only ever a guess, and that guess can
be sometimes wrong, even wildly wrong. So the kernel is increasingly
moving away from /relying/ on that, and is more focused on getting more
sources faster -- incorporating anything it can find, and mixing it into
the output stream more continuously. To that end, if EFI's got a DXE to
do something or another, please hook it up.

Lastly, I think the concern you raised is inappropriate for Ard's
patchset, as it actually doesn't apply to it at all. This patchset is
about hooking an existing DXE up to OVMF, one that is hooked up
elsewhere, but wasn't for OVMF. This alone has nothing to do with the
concern. Rather, the concerns you raised are about the DXE itself. So to
that end, perhaps you should start a new thread or send some patches or
do something to the DXE that you're concerned about (e.g. a basic boring
power-on selftest like what the kernel has or something, if you're extra
worried). Or maybe not, for the reasons I listed above of things being
basically fine.

Jason

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Jason A. Donenfeld]

Hi again,

On Tue, Nov 22, 2022 at 11:35:06AM +0000, Pedro Falcato wrote:
> We should probably also test for stupidly broken rdrand implementations
> like the notorious Zen 3 which always return 0xFFFFFFFF (per xkcd 221 ;)).

On this topic, if you did want to improve this part of that DXE, the
kernel's test for this is super dumb and basic but I think basically
gets the job done for the most pathological scenarios.

>From arch/x86/kernel/cpu/rdrand.c:

  void x86_init_rdrand(struct cpuinfo_x86 *c)
  {
    enum { SAMPLES = 8, MIN_CHANGE = 5 };
    unsigned long sample, prev;
    bool failure = false;
    size_t i, changed;
  
    if (!cpu_has(c, X86_FEATURE_RDRAND))
      return;
  
    for (changed = 0, i = 0; i < SAMPLES; ++i) {
      if (!rdrand_long(&sample)) {
        failure = true;
        break;
      }
      changed += i && sample != prev;
      prev = sample;
    }
    if (changed < MIN_CHANGE)
      failure = true;
  
    if (failure) {
      clear_cpu_cap(c, X86_FEATURE_RDRAND);
      clear_cpu_cap(c, X86_FEATURE_RDSEED);
      pr_emerg("RDRAND is not reliable on this platform; disabling.\n");
    }
  }

Feel free to lift that into the DXE if you want. That should probably be
a different patch to this series, though, as I mentioned in my last
email. The CPUID check that you mentioned, however, seems like an
important prerequisite to this series.

Jason

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]

On Tue, Nov 22, 2022 at 12:20 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> Hi Pedro,
>
> On Tue, Nov 22, 2022 at 12:35 PM Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
> > Given this patch plus the corresponding linux-efi patches wrt RNG, I'm
> > mildly concerned about buggy RDRAND implementations compromising the
> > kernel's RNG. Is this not a concern?
>

Hi,

Thank you for your thorough response, glad to have you in this thread.

Speaking with my kernel RNG maintainer hat on, no, this is not really a
> concern, for several reasons:
>
> - The kernel's RNG takes input from multiple sources, continuously, and
>   tries to mix in new inputs rather quickly, especially at early boot.
>

I am aware, but I'm more scared when it comes to very early boot (think
linux's EFI stub or some other bootloader) I can see how
an ill-advised RNG_PROTOCOL user can try to exclusively rely on it (if it's
available, which I don't believe it is atm on non-virtio-rng OVMF) vs
mixing in the few sources you can get at that point, making important
things like KASLR addresses or possibly even a stack canary 100% guessable.

- The kernel will use RDRAND on its own, even in the case that EFI
>   doesn't provide it, so it's not gaining anything here.
>

Yes, but as you said, the kernel mixes RDRAND with a lot of other entropy
sources and also does proper sanity checking on it.

- EFI on actual baremetal firmware, as opposed to OVMF, already provides
>   EFI, so this is par for the course.
>

Hm? What do you mean?

> - Most of those RDRAND bugs have concerned coming in and out of various
>   sleep states, which doesn't really apply to early boot EFI.
>
> - And again, just to reinforce the first point, the kernel takes inputs
>   from many sources. Having EFI provide its own thing -- via RDRAND or
>   any other mechanism -- is complementary and will only help.
>
> Regarding your "corresponding linux-efi patches wrt RNG", I'm not quite
> sure what you're referring to. If anything, recent work during this
> cycle has been aimed around shuffling more sources of randomness in from
> elsewhere. The EFI RNG protocol stuff has been in there already for a
> long time. So maybe you misunderstood those? Or I'm misunderstanding
> what you're referring to?
>

Ah yes, I haven't been paying much close attention to the RNG patches
themselves but now that I took a closer look I can see you're right.

As a general point, this question of "do we have enough entropy?" or
> "are we initialized yet?" is an impossible proposition. Entropy
> estimation is impossible, and is only ever a guess, and that guess can
> be sometimes wrong, even wildly wrong. So the kernel is increasingly
> moving away from /relying/ on that, and is more focused on getting more
> sources faster -- incorporating anything it can find, and mixing it into
> the output stream more continuously. To that end, if EFI's got a DXE to
> do something or another, please hook it up.
>
> Lastly, I think the concern you raised is inappropriate for Ard's
> patchset, as it actually doesn't apply to it at all. This patchset is
> about hooking an existing DXE up to OVMF, one that is hooked up
> elsewhere, but wasn't for OVMF. This alone has nothing to do with the
> concern. Rather, the concerns you raised are about the DXE itself. So to
> that end, perhaps you should start a new thread or send some patches or
> do something to the DXE that you're concerned about (e.g. a basic boring
> power-on selftest like what the kernel has or something, if you're extra
> worried). Or maybe not, for the reasons I listed above of things being
> basically fine.
>

I know, I'm not yelling at Ard for the (questionable?) choices done in the
BaseRngLib code, but I'm concerned this patch may negatively influence any
sort of early boot RNG,
particularly for the more naive users of RNG_PROTOCOL, by providing the
possibly flawed RDRAND code. If the efi subsystem/EFISTUB code handles this
case well by still mixing
in whatever sources it can get before using this entropy, then that's
great, but providing things like a non-random RNG_PROTOCOL sounds very
broken and very unsafe to me (again invoking
that possible KASLR at very early boot example).

Also the CPUID check seems like an important step towards
not-breaking-old-CPUs.
All I'm saying is that we shouldn't just hook up the RNG DXE driver without
carefully considering what the code is doing.

Thanks,
Pedro

[-- Attachment #2: Type: text/html, Size: 6069 bytes --]

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Jason A. Donenfeld]

Hi Pedro,

On 11/22/22, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> I am aware, but I'm more scared when it comes to very early boot (think
> linux's EFI stub or some other bootloader) I can see how
> an ill-advised RNG_PROTOCOL user can try to exclusively rely on it (if it's
> available, which I don't believe it is atm on non-virtio-rng OVMF) vs
> mixing in the few sources you can get at that point, making important
> things like KASLR addresses or possibly even a stack canary 100% guessable.

The thing is, in low-entropy early boot scenarios, the alternative
would be just having nothing. The kernel doesn't pause boot to wait
for a good source....it just uses a bad one. So adding RDRAND in EFI
helps. Really, more entropy from more sources as early as possible and
as fast as possible only ever helps here.

> also does proper sanity checking on it.

No, there's nothing "proper" about it. It's a dumb basic thing.


>
> - EFI on actual baremetal firmware, as opposed to OVMF, already provides
>>   EFI, so this is par for the course.
>>
>
> Hm? What do you mean?

I meant already provides RDRAND, sorry. All x86 hardware with EFI has
this enabled.

> I know, I'm not yelling at Ard for the (questionable?) choices done in the
> BaseRngLib code, but I'm concerned this patch may negatively influence any
> sort of early boot RNG,
> particularly for the more naive users of RNG_PROTOCOL, by providing the
> possibly flawed RDRAND code. If the efi subsystem/EFISTUB code handles this
> case well by still mixing
> in whatever sources it can get before using this entropy, then that's
> great, but providing things like a non-random RNG_PROTOCOL sounds very
> broken and very unsafe to me (again invoking
> that possible KASLR at very early boot example).

Except as related several times now, it will only help and won't hurt.
If you want to improve the RDRAND DXE, do it, but aside from the CPUID
issue you raised, I don't think your "concerns" warrant holding up
this patch.

>
> Also the CPUID check seems like an important step towards
> not-breaking-old-CPUs.

Yes. If what you say is true, this should be fixed asap. Do you intend
to send a patch?

> All I'm saying is that we shouldn't just hook up the RNG DXE driver without
> carefully considering what the code is doing.

To point out again, this is already hooked up to baremetal firmware.
So if you see issues that are worth fixing, fix them, but it shouldn't
hold up Ard's patchset.

Jason

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On Tue, Nov 22, 2022 at 1:10 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> Yes. If what you say is true, this should be fixed asap. Do you intend
> to send a patch?
>

I have sent out a patch (https://edk2.groups.io/g/devel/message/96552)
fixing the CPUID checks with a naive attempt to sniff out RDRAND issues.
Your Linux snippet is probably better but I couldn't look at it due to
licensing concerns.

To point out again, this is already hooked up to baremetal firmware.
> So if you see issues that are worth fixing, fix them, but it shouldn't
> hold up Ard's patchset.


"Real firmware" is frequently not a shining example of quality :) They can
also more or less know what CPUs the fw will run on, but OVMF runs mostly
everywhere.

In any case, I've sent out a patch (hopefully) fixing these issues. It
should be trivial enough to review and apply so that Ard's patch doesn't
get held up.
I think it's best to apply this only after my patch gets merged (in order
to avoid weird breakage between commits) but that's up to the OVMF
maintainers to decide.

Pedro

[-- Attachment #2: Type: text/html, Size: 1858 bytes --]

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

[Jason A. Donenfeld]

Hi,

On Tue, Nov 22, 2022 at 3:17 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> I have sent out a patch (https://edk2.groups.io/g/devel/message/96552) fixing the CPUID checks with a naive attempt to sniff out RDRAND issues.
> Your Linux snippet is probably better but I couldn't look at it due to licensing concerns.

I (re)wrote that function in Linux. I hereby relicense it as MIT, and
also place it into public domain. Do with it what you will now.

Jason

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

[Jason A. Donenfeld]

Could we get this merged?

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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.

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

[Ard Biesheuvel]

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.

Re: [edk2-devel] [PATCH 2/3] ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if implemented

[Ard Biesheuvel]

On Fri, 18 Nov 2022 at 17:48, PierreGondois <pierre.gondois@arm.com> wrote:
>
> Hello Ard,
>
> On 11/10/22 14:47, Ard Biesheuvel wrote:
> > Currently, we only expose the EFI_RNG_PROTOCOL in ArmVirtQemu if QEMU
> > provides a virtio-rng device, and it doesn't do so by default.
> >
> > Given that KVM exposes the ARM architected TRNG service (and has done so
> > for a while now), let's incorporate the RngDxe driver which has recently
> > grown support for the ARM firmware/hypervisor service.
> >
> > If both the service and the virtio device are available, two
> > implementations of the RNG protocol will be exposed, but this is fine:
> > callers that don't care about the distinction will grab the first one
> > available.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   ArmVirtPkg/ArmVirtQemu.dsc           | 11 +++++++++++
> >   ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
> >   ArmVirtPkg/ArmVirtQemuKernel.dsc     | 11 +++++++++++
> >   3 files changed, 27 insertions(+)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index f77443229e8e..1771ad562225 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -140,6 +140,8 @@ [PcdsFeatureFlag.common]
> >
> >     gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> >
> > +  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
> > +
>
> It seems that the PSCI conduit needs to be dynamically set.

Why? And how is this different from PSCI for resetting the system?

Note that ArmVIrtQemu was never intended to run at EL2, even if it
seems to work to some extent.


> The psci conduit that should be used is configured by qemu depending on the
> virtualization=[on|off] parameter. When off, HVC must be used (SMC otherwise).
> Cf:
> https://github.com/qemu/qemu/blob/master/hw/arm/virt.c#L2052
>
> If using the wrong conduit, qemu traps the instruction and stops.
> For KvmTool, the conduit is always HVC.
>
> Command used:
> [PATH_TO]/qemu/build/qemu-system-aarch64 \
>         -serial stdio -M virt,highmem=on,virtualization=off \
>         -cpu cortex-a57 -smp 4 -m 4096 \
>         -drive file=pflash0.img,format=raw,if=pflash,readonly=on \
>         -drive file=pflash1.img,format=raw,if=pflash
>
> Regards,
> Pierre
>
>
> 
>
>

Re: [edk2-devel] [PATCH 2/3] ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if implemented

[PierreGondois]

On 1/11/23 17:49, Ard Biesheuvel wrote:
> On Fri, 18 Nov 2022 at 17:48, PierreGondois <pierre.gondois@arm.com> wrote:
>>
>> Hello Ard,
>>
>> On 11/10/22 14:47, Ard Biesheuvel wrote:
>>> Currently, we only expose the EFI_RNG_PROTOCOL in ArmVirtQemu if QEMU
>>> provides a virtio-rng device, and it doesn't do so by default.
>>>
>>> Given that KVM exposes the ARM architected TRNG service (and has done so
>>> for a while now), let's incorporate the RngDxe driver which has recently
>>> grown support for the ARM firmware/hypervisor service.
>>>
>>> If both the service and the virtio device are available, two
>>> implementations of the RNG protocol will be exposed, but this is fine:
>>> callers that don't care about the distinction will grab the first one
>>> available.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    ArmVirtPkg/ArmVirtQemu.dsc           | 11 +++++++++++
>>>    ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
>>>    ArmVirtPkg/ArmVirtQemuKernel.dsc     | 11 +++++++++++
>>>    3 files changed, 27 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> index f77443229e8e..1771ad562225 100644
>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> @@ -140,6 +140,8 @@ [PcdsFeatureFlag.common]
>>>
>>>      gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
>>>
>>> +  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
>>> +
>>
>> It seems that the PSCI conduit needs to be dynamically set.
> 
> Why? And how is this different from PSCI for resetting the system?

The PSCI implementation used for ArmVirtQemu is parsing the conduit
to use, so it can handle both cases:
https://github.com/tianocore/edk2/blob/fe405f08a09e9f2306c72aa23d8edfbcfaa23bff/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c#L49

> 
> Note that ArmVIrtQemu was never intended to run at EL2, even if it
> seems to work to some extent.

Ok understood.

> 
> 
>> The psci conduit that should be used is configured by qemu depending on the
>> virtualization=[on|off] parameter. When off, HVC must be used (SMC otherwise).
>> Cf:
>> https://github.com/qemu/qemu/blob/master/hw/arm/virt.c#L2052

(replacing with a permanent link)
https://github.com/qemu/qemu/blob/aa96ab7c9df59c615ca82b49c9062819e0a1c287/hw/arm/virt.c#L2080

>>
>> If using the wrong conduit, qemu traps the instruction and stops.
>> For KvmTool, the conduit is always HVC.
>>
>> Command used:
>> [PATH_TO]/qemu/build/qemu-system-aarch64 \
>>          -serial stdio -M virt,highmem=on,virtualization=off \
>>          -cpu cortex-a57 -smp 4 -m 4096 \
>>          -drive file=pflash0.img,format=raw,if=pflash,readonly=on \
>>          -drive file=pflash1.img,format=raw,if=pflash
>>
>> Regards,
>> Pierre
>>
>>
>> 
>>
>>

Re: [edk2-devel] [PATCH 2/3] ArmVirtPkg/ArmVirtQemu: Expose TRNG hypercall via RngDxe if implemented

[Ard Biesheuvel]

On Wed, 11 Jan 2023 at 18:38, PierreGondois <pierre.gondois@arm.com> wrote:
>
>
>
> On 1/11/23 17:49, Ard Biesheuvel wrote:
> > On Fri, 18 Nov 2022 at 17:48, PierreGondois <pierre.gondois@arm.com> wrote:
> >>
> >> Hello Ard,
> >>
> >> On 11/10/22 14:47, Ard Biesheuvel wrote:
> >>> Currently, we only expose the EFI_RNG_PROTOCOL in ArmVirtQemu if QEMU
> >>> provides a virtio-rng device, and it doesn't do so by default.
> >>>
> >>> Given that KVM exposes the ARM architected TRNG service (and has done so
> >>> for a while now), let's incorporate the RngDxe driver which has recently
> >>> grown support for the ARM firmware/hypervisor service.
> >>>
> >>> If both the service and the virtio device are available, two
> >>> implementations of the RNG protocol will be exposed, but this is fine:
> >>> callers that don't care about the distinction will grab the first one
> >>> available.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    ArmVirtPkg/ArmVirtQemu.dsc           | 11 +++++++++++
> >>>    ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
> >>>    ArmVirtPkg/ArmVirtQemuKernel.dsc     | 11 +++++++++++
> >>>    3 files changed, 27 insertions(+)
> >>>
> >>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> >>> index f77443229e8e..1771ad562225 100644
> >>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> >>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> >>> @@ -140,6 +140,8 @@ [PcdsFeatureFlag.common]
> >>>
> >>>      gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> >>>
> >>> +  gArmTokenSpaceGuid.PcdMonitorConduitHvc|TRUE
> >>> +
> >>
> >> It seems that the PSCI conduit needs to be dynamically set.
> >
> > Why? And how is this different from PSCI for resetting the system?
>
> The PSCI implementation used for ArmVirtQemu is parsing the conduit
> to use, so it can handle both cases:
> https://github.com/tianocore/edk2/blob/fe405f08a09e9f2306c72aa23d8edfbcfaa23bff/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c#L49
>

Hmm, that is a fair point.

I wonder if we should simply detect whether we are executing at EL1 or
EL2, and use HVC or SVC respectively.


> >
> > Note that ArmVIrtQemu was never intended to run at EL2, even if it
> > seems to work to some extent.
>
> Ok understood.
>
> >
> >
> >> The psci conduit that should be used is configured by qemu depending on the
> >> virtualization=[on|off] parameter. When off, HVC must be used (SMC otherwise).
> >> Cf:
> >> https://github.com/qemu/qemu/blob/master/hw/arm/virt.c#L2052
>
> (replacing with a permanent link)
> https://github.com/qemu/qemu/blob/aa96ab7c9df59c615ca82b49c9062819e0a1c287/hw/arm/virt.c#L2080
>
> >>
> >> If using the wrong conduit, qemu traps the instruction and stops.
> >> For KvmTool, the conduit is always HVC.
> >>
> >> Command used:
> >> [PATH_TO]/qemu/build/qemu-system-aarch64 \
> >>          -serial stdio -M virt,highmem=on,virtualization=off \
> >>          -cpu cortex-a57 -smp 4 -m 4096 \
> >>          -drive file=pflash0.img,format=raw,if=pflash,readonly=on \
> >>          -drive file=pflash1.img,format=raw,if=pflash
> >>
> >> Regards,
> >> Pierre
> >>
> >>
> >>
> >>
> >>
>
>
> 
>
>

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

[Laszlo Ersek]

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