[PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151

The EFI_RNG_PROTOCOL can advertise multiple algorithms through
Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
can be configured. It represents the algorithm used in RngLib.
PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.

When running KvmTool on a platform platform only having the RngLib,
the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.

To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
a. Zero Guids are skipped
b. If no algorithm is found, an ASSERT is triggered

To allow using the RngLib to be used for the case above, Zero Guids
should not be skipped (a.).
If no algorithm is found, don't prevent from booting on DEBUG builds
(b.).

Allow Zero Guids to be selected and don't ASSERT if no algorithm is
found. Also simplify the selection of the Rng algorithm when the
default one is selected by just picking up the first element of
mAvailableAlgoArray.

Reported-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---

Notes:
    v2:
    - Reformulate commit message.
    - Do not warn if no algorithm is found as the message
      would be printed on non-Arm platforms.

 .../RandomNumberGenerator/RngDxe/ArmRngDxe.c      | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
index 5ba319899ce9..722d53386373 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
@@ -76,7 +76,6 @@ RngGetRNG (
   )
 {
   EFI_STATUS  Status;
-  UINTN       Index;
 
   if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -86,21 +85,13 @@ RngGetRNG (
     //
     // Use the default RNG algorithm if RNGAlgorithm is NULL.
     //
-    for (Index = 0; Index < mAvailableAlgoArrayCount; Index++) {
-      if (!IsZeroGuid (&mAvailableAlgoArray[Index])) {
-        RNGAlgorithm = &mAvailableAlgoArray[Index];
-        goto FoundAlgo;
-      }
-    }
-
-    if (Index == mAvailableAlgoArrayCount) {
-      // No algorithm available.
-      ASSERT (Index != mAvailableAlgoArrayCount);
+    if (mAvailableAlgoArrayCount != 0) {
+      RNGAlgorithm = &mAvailableAlgoArray[0];
+    } else {
       return EFI_DEVICE_ERROR;
     }
   }
 
-FoundAlgo:
   if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
     Status = RngGetBytes (RNGValueLength, RNGValue);
     return Status;
-- 
2.25.1

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[Ard Biesheuvel]

On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
>
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>
> The EFI_RNG_PROTOCOL can advertise multiple algorithms through
> Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
> can be configured. It represents the algorithm used in RngLib.
> PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
>
> When running KvmTool on a platform platform only having the RngLib,
> the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
>
> To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
> a. Zero Guids are skipped
> b. If no algorithm is found, an ASSERT is triggered
>
> To allow using the RngLib to be used for the case above, Zero Guids
> should not be skipped (a.).
> If no algorithm is found, don't prevent from booting on DEBUG builds
> (b.).
>
> Allow Zero Guids to be selected and don't ASSERT if no algorithm is
> found. Also simplify the selection of the Rng algorithm when the
> default one is selected by just picking up the first element of
> mAvailableAlgoArray.
>
> Reported-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>

I am still confused by this.

Does this mean we might register the RNG protocol if we don't have
anything to back it up?

> ---
>
> Notes:
>     v2:
>     - Reformulate commit message.
>     - Do not warn if no algorithm is found as the message
>       would be printed on non-Arm platforms.
>
>  .../RandomNumberGenerator/RngDxe/ArmRngDxe.c      | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> index 5ba319899ce9..722d53386373 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
> @@ -76,7 +76,6 @@ RngGetRNG (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINTN       Index;
>
>    if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -86,21 +85,13 @@ RngGetRNG (
>      //
>      // Use the default RNG algorithm if RNGAlgorithm is NULL.
>      //
> -    for (Index = 0; Index < mAvailableAlgoArrayCount; Index++) {
> -      if (!IsZeroGuid (&mAvailableAlgoArray[Index])) {
> -        RNGAlgorithm = &mAvailableAlgoArray[Index];
> -        goto FoundAlgo;
> -      }
> -    }
> -
> -    if (Index == mAvailableAlgoArrayCount) {
> -      // No algorithm available.
> -      ASSERT (Index != mAvailableAlgoArrayCount);
> +    if (mAvailableAlgoArrayCount != 0) {
> +      RNGAlgorithm = &mAvailableAlgoArray[0];
> +    } else {
>        return EFI_DEVICE_ERROR;
>      }
>    }
>
> -FoundAlgo:
>    if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
>      Status = RngGetBytes (RNGValueLength, RNGValue);
>      return Status;
> --
> 2.25.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96434): https://edk2.groups.io/g/devel/message/96434
> Mute This Topic: https://groups.io/mt/95067856/5717338
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
> ------------
>
>

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[Sami Mujawar]

Hi Ard,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 18/11/2022, 09:56, "Ard Biesheuvel" <ardb@kernel.org> wrote:

    On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
    >
    > From: Pierre Gondois <pierre.gondois@arm.com>
    >
    > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
    >
    > The EFI_RNG_PROTOCOL can advertise multiple algorithms through
    > Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
    > can be configured. It represents the algorithm used in RngLib.
    > PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
    >
    > When running KvmTool on a platform platform only having the RngLib,
    > the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
    >
    > To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
    > a. Zero Guids are skipped
    > b. If no algorithm is found, an ASSERT is triggered
    >
    > To allow using the RngLib to be used for the case above, Zero Guids
    > should not be skipped (a.).
    > If no algorithm is found, don't prevent from booting on DEBUG builds
    > (b.).
    >
    > Allow Zero Guids to be selected and don't ASSERT if no algorithm is
    > found. Also simplify the selection of the Rng algorithm when the
    > default one is selected by just picking up the first element of
    > mAvailableAlgoArray.
    >
    > Reported-by: Sami Mujawar <sami.mujawar@arm.com>
    > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>

    I am still confused by this.

    Does this mean we might register the RNG protocol if we don't have
    anything to back it up?
[SAMI] From a Guest firmware implementation perspective, we do not know the available RNG source.
It may be CPU RNG, Arm FW TRNG or VIRTIO RNG. 
I would assume either one of CPU RNG or Arm FW TRNG would be implemented on the host platform. If none of these are present, we would want to fall back to VIRTIO RNG.

Considering this, I think we should not register the EFI_RNG_PRTOCOL if no supported algorithms are present.

The other argument would be that the protocol allows discovery of supported RNG source. But looking how this is consumed in Linux, I think it is better to not register EFI_RNG_PRTOCOL if no supported algorithms are present.

Please do let me know your thoughts.
[/SAMI]


    > ---
    >
    > Notes:
    >     v2:
    >     - Reformulate commit message.
    >     - Do not warn if no algorithm is found as the message
    >       would be printed on non-Arm platforms.
    >
    >  .../RandomNumberGenerator/RngDxe/ArmRngDxe.c      | 15 +++------------
    >  1 file changed, 3 insertions(+), 12 deletions(-)
    >
    > diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
    > index 5ba319899ce9..722d53386373 100644
    > --- a/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
    > +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c
    > @@ -76,7 +76,6 @@ RngGetRNG (
    >    )
    >  {
    >    EFI_STATUS  Status;
    > -  UINTN       Index;
    >
    >    if ((This == NULL) || (RNGValueLength == 0) || (RNGValue == NULL)) {
    >      return EFI_INVALID_PARAMETER;
    > @@ -86,21 +85,13 @@ RngGetRNG (
    >      //
    >      // Use the default RNG algorithm if RNGAlgorithm is NULL.
    >      //
    > -    for (Index = 0; Index < mAvailableAlgoArrayCount; Index++) {
    > -      if (!IsZeroGuid (&mAvailableAlgoArray[Index])) {
    > -        RNGAlgorithm = &mAvailableAlgoArray[Index];
    > -        goto FoundAlgo;
    > -      }
    > -    }
    > -
    > -    if (Index == mAvailableAlgoArrayCount) {
    > -      // No algorithm available.
    > -      ASSERT (Index != mAvailableAlgoArrayCount);
    > +    if (mAvailableAlgoArrayCount != 0) {
    > +      RNGAlgorithm = &mAvailableAlgoArray[0];
    > +    } else {
    >        return EFI_DEVICE_ERROR;
    >      }
    >    }
    >
    > -FoundAlgo:
    >    if (CompareGuid (RNGAlgorithm, PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
    >      Status = RngGetBytes (RNGValueLength, RNGValue);
    >      return Status;
    > --
    > 2.25.1
    >
    >
    >
    > ------------
    > Groups.io Links: You receive all messages sent to this group.
    > View/Reply Online (#96434): https://edk2.groups.io/g/devel/message/96434
    > Mute This Topic: https://groups.io/mt/95067856/5717338
    > Group Owner: devel+owner@edk2.groups.io
    > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
    > ------------
    >
    >

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[Ard Biesheuvel]

On Fri, 18 Nov 2022 at 11:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> Hi Ard,
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 18/11/2022, 09:56, "Ard Biesheuvel" <ardb@kernel.org> wrote:
>
>     On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
>     >
>     > From: Pierre Gondois <pierre.gondois@arm.com>
>     >
>     > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>     >
>     > The EFI_RNG_PROTOCOL can advertise multiple algorithms through
>     > Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
>     > can be configured. It represents the algorithm used in RngLib.
>     > PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
>     >
>     > When running KvmTool on a platform platform only having the RngLib,
>     > the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
>     >
>     > To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
>     > a. Zero Guids are skipped
>     > b. If no algorithm is found, an ASSERT is triggered
>     >
>     > To allow using the RngLib to be used for the case above, Zero Guids
>     > should not be skipped (a.).
>     > If no algorithm is found, don't prevent from booting on DEBUG builds
>     > (b.).
>     >
>     > Allow Zero Guids to be selected and don't ASSERT if no algorithm is
>     > found. Also simplify the selection of the Rng algorithm when the
>     > default one is selected by just picking up the first element of
>     > mAvailableAlgoArray.
>     >
>     > Reported-by: Sami Mujawar <sami.mujawar@arm.com>
>     > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>
>     I am still confused by this.
>
>     Does this mean we might register the RNG protocol if we don't have
>     anything to back it up?
> [SAMI] From a Guest firmware implementation perspective, we do not know the available RNG source.
> It may be CPU RNG, Arm FW TRNG or VIRTIO RNG.
> I would assume either one of CPU RNG or Arm FW TRNG would be implemented on the host platform. If none of these are present, we would want to fall back to VIRTIO RNG.
>
> Considering this, I think we should not register the EFI_RNG_PRTOCOL if no supported algorithms are present.
>
> The other argument would be that the protocol allows discovery of supported RNG source. But looking how this is consumed in Linux, I think it is better to not register EFI_RNG_PRTOCOL if no supported algorithms are present.
>
> Please do let me know your thoughts.

Agreed. I am adding support for the TRNG to the QEMU firmware, and if
this is supported, there is really no point in attaching a driver to
virtio-rng as well.

This means that checking for the existence of the EFI_RNG_PROTOCOL
should be sufficient to decide whether or not install another one.

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PierreGondois]

On 11/18/22 11:14, Ard Biesheuvel wrote:
> On Fri, 18 Nov 2022 at 11:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>>
>> Hi Ard,
>>
>> Please find my response inline marked [SAMI].
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 18/11/2022, 09:56, "Ard Biesheuvel" <ardb@kernel.org> wrote:
>>
>>      On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
>>      >
>>      > From: Pierre Gondois <pierre.gondois@arm.com>
>>      >
>>      > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>>      >
>>      > The EFI_RNG_PROTOCOL can advertise multiple algorithms through
>>      > Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
>>      > can be configured. It represents the algorithm used in RngLib.
>>      > PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
>>      >
>>      > When running KvmTool on a platform platform only having the RngLib,
>>      > the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
>>      >
>>      > To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
>>      > a. Zero Guids are skipped
>>      > b. If no algorithm is found, an ASSERT is triggered
>>      >
>>      > To allow using the RngLib to be used for the case above, Zero Guids
>>      > should not be skipped (a.).
>>      > If no algorithm is found, don't prevent from booting on DEBUG builds
>>      > (b.).
>>      >
>>      > Allow Zero Guids to be selected and don't ASSERT if no algorithm is
>>      > found. Also simplify the selection of the Rng algorithm when the
>>      > default one is selected by just picking up the first element of
>>      > mAvailableAlgoArray.
>>      >
>>      > Reported-by: Sami Mujawar <sami.mujawar@arm.com>
>>      > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>>
>>      I am still confused by this.
>>
>>      Does this mean we might register the RNG protocol if we don't have
>>      anything to back it up?
>> [SAMI] From a Guest firmware implementation perspective, we do not know the available RNG source.
>> It may be CPU RNG, Arm FW TRNG or VIRTIO RNG.
>> I would assume either one of CPU RNG or Arm FW TRNG would be implemented on the host platform. If none of these are present, we would want to fall back to VIRTIO RNG.
>>
>> Considering this, I think we should not register the EFI_RNG_PRTOCOL if no supported algorithms are present.
>>
>> The other argument would be that the protocol allows discovery of supported RNG source. But looking how this is consumed in Linux, I think it is better to not register EFI_RNG_PRTOCOL if no supported algorithms are present.
>>
>> Please do let me know your thoughts.
> 
> Agreed. I am adding support for the TRNG to the QEMU firmware, and if
> this is supported, there is really no point in attaching a driver to
> virtio-rng as well.
> 
> This means that checking for the existence of the EFI_RNG_PROTOCOL
> should be sufficient to decide whether or not install another one.

Hello,
There would maybe be a case where the consumer explicitly requests the
gEfiRngAlgorithmRaw GUID and the RngDxe doesn't detect it. So there would be
two EFI_RNG_PRTOCOL registered on different handles with different
algorithms available. I am not sure of what should happen then.

But about the inital point, EFI_RNG_PRTOCOL should indeed not be registered
if no algorithm is available.

Regards,
Pierre

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[Ard Biesheuvel]

On Fri, 18 Nov 2022 at 11:32, PierreGondois <pierre.gondois@arm.com> wrote:
>
>
>
> On 11/18/22 11:14, Ard Biesheuvel wrote:
> > On Fri, 18 Nov 2022 at 11:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
> >>
> >> Hi Ard,
> >>
> >> Please find my response inline marked [SAMI].
> >>
> >> Regards,
> >>
> >> Sami Mujawar
> >>
> >> On 18/11/2022, 09:56, "Ard Biesheuvel" <ardb@kernel.org> wrote:
> >>
> >>      On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
> >>      >
> >>      > From: Pierre Gondois <pierre.gondois@arm.com>
> >>      >
> >>      > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
> >>      >
> >>      > The EFI_RNG_PROTOCOL can advertise multiple algorithms through
> >>      > Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
> >>      > can be configured. It represents the algorithm used in RngLib.
> >>      > PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
> >>      >
> >>      > When running KvmTool on a platform platform only having the RngLib,
> >>      > the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
> >>      >
> >>      > To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
> >>      > a. Zero Guids are skipped
> >>      > b. If no algorithm is found, an ASSERT is triggered
> >>      >
> >>      > To allow using the RngLib to be used for the case above, Zero Guids
> >>      > should not be skipped (a.).
> >>      > If no algorithm is found, don't prevent from booting on DEBUG builds
> >>      > (b.).
> >>      >
> >>      > Allow Zero Guids to be selected and don't ASSERT if no algorithm is
> >>      > found. Also simplify the selection of the Rng algorithm when the
> >>      > default one is selected by just picking up the first element of
> >>      > mAvailableAlgoArray.
> >>      >
> >>      > Reported-by: Sami Mujawar <sami.mujawar@arm.com>
> >>      > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> >>
> >>      I am still confused by this.
> >>
> >>      Does this mean we might register the RNG protocol if we don't have
> >>      anything to back it up?
> >> [SAMI] From a Guest firmware implementation perspective, we do not know the available RNG source.
> >> It may be CPU RNG, Arm FW TRNG or VIRTIO RNG.
> >> I would assume either one of CPU RNG or Arm FW TRNG would be implemented on the host platform. If none of these are present, we would want to fall back to VIRTIO RNG.
> >>
> >> Considering this, I think we should not register the EFI_RNG_PRTOCOL if no supported algorithms are present.
> >>
> >> The other argument would be that the protocol allows discovery of supported RNG source. But looking how this is consumed in Linux, I think it is better to not register EFI_RNG_PRTOCOL if no supported algorithms are present.
> >>
> >> Please do let me know your thoughts.
> >
> > Agreed. I am adding support for the TRNG to the QEMU firmware, and if
> > this is supported, there is really no point in attaching a driver to
> > virtio-rng as well.
> >
> > This means that checking for the existence of the EFI_RNG_PROTOCOL
> > should be sufficient to decide whether or not install another one.
>
> Hello,
> There would maybe be a case where the consumer explicitly requests the
> gEfiRngAlgorithmRaw GUID and the RngDxe doesn't detect it. So there would be
> two EFI_RNG_PRTOCOL registered on different handles with different
> algorithms available. I am not sure of what should happen then.
>

Fair point. So in the QEMU case, I should test
- whether EFI_RNG_PROTOCOL exists
- whether it implements the raw flavor

and only in that case, avoid virtio-rng (which only supports raw as well)

> But about the inital point, EFI_RNG_PRTOCOL should indeed not be registered
> if no algorithm is available.
>

Indeed.

And btw, I noticed that the TrngLib has a whole bunch of ASSERT()s
that trigger when trying to use it on QEMU - can we rip those out as
well please?

Re: [edk2-devel] [PATCH v2 1/1] SecurityPkg/RngDxe: Fix Rng algo selection for Arm

[PierreGondois]

On 11/18/22 11:35, Ard Biesheuvel wrote:
> On Fri, 18 Nov 2022 at 11:32, PierreGondois <pierre.gondois@arm.com> wrote:
>>
>>
>>
>> On 11/18/22 11:14, Ard Biesheuvel wrote:
>>> On Fri, 18 Nov 2022 at 11:10, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>>>>
>>>> Hi Ard,
>>>>
>>>> Please find my response inline marked [SAMI].
>>>>
>>>> Regards,
>>>>
>>>> Sami Mujawar
>>>>
>>>> On 18/11/2022, 09:56, "Ard Biesheuvel" <ardb@kernel.org> wrote:
>>>>
>>>>       On Wed, 16 Nov 2022 at 16:02, PierreGondois <pierre.gondois@arm.com> wrote:
>>>>       >
>>>>       > From: Pierre Gondois <pierre.gondois@arm.com>
>>>>       >
>>>>       > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4151
>>>>       >
>>>>       > The EFI_RNG_PROTOCOL can advertise multiple algorithms through
>>>>       > Guids. The PcdCpuRngSupportedAlgorithm contains a Guid that
>>>>       > can be configured. It represents the algorithm used in RngLib.
>>>>       > PcdCpuRngSupportedAlgorithm is set to the Zero Guid for KvmTool.
>>>>       >
>>>>       > When running KvmTool on a platform platform only having the RngLib,
>>>>       > the only Guid available for EFI_RNG_PROTOCOL will be the zero Guid.
>>>>       >
>>>>       > To select the default algorithm in EFI_RNG_PROTOCOL.GetRng():
>>>>       > a. Zero Guids are skipped
>>>>       > b. If no algorithm is found, an ASSERT is triggered
>>>>       >
>>>>       > To allow using the RngLib to be used for the case above, Zero Guids
>>>>       > should not be skipped (a.).
>>>>       > If no algorithm is found, don't prevent from booting on DEBUG builds
>>>>       > (b.).
>>>>       >
>>>>       > Allow Zero Guids to be selected and don't ASSERT if no algorithm is
>>>>       > found. Also simplify the selection of the Rng algorithm when the
>>>>       > default one is selected by just picking up the first element of
>>>>       > mAvailableAlgoArray.
>>>>       >
>>>>       > Reported-by: Sami Mujawar <sami.mujawar@arm.com>
>>>>       > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>>>>
>>>>       I am still confused by this.
>>>>
>>>>       Does this mean we might register the RNG protocol if we don't have
>>>>       anything to back it up?
>>>> [SAMI] From a Guest firmware implementation perspective, we do not know the available RNG source.
>>>> It may be CPU RNG, Arm FW TRNG or VIRTIO RNG.
>>>> I would assume either one of CPU RNG or Arm FW TRNG would be implemented on the host platform. If none of these are present, we would want to fall back to VIRTIO RNG.
>>>>
>>>> Considering this, I think we should not register the EFI_RNG_PRTOCOL if no supported algorithms are present.
>>>>
>>>> The other argument would be that the protocol allows discovery of supported RNG source. But looking how this is consumed in Linux, I think it is better to not register EFI_RNG_PRTOCOL if no supported algorithms are present.
>>>>
>>>> Please do let me know your thoughts.
>>>
>>> Agreed. I am adding support for the TRNG to the QEMU firmware, and if
>>> this is supported, there is really no point in attaching a driver to
>>> virtio-rng as well.
>>>
>>> This means that checking for the existence of the EFI_RNG_PROTOCOL
>>> should be sufficient to decide whether or not install another one.
>>
>> Hello,
>> There would maybe be a case where the consumer explicitly requests the
>> gEfiRngAlgorithmRaw GUID and the RngDxe doesn't detect it. So there would be
>> two EFI_RNG_PRTOCOL registered on different handles with different
>> algorithms available. I am not sure of what should happen then.
>>
> 
> Fair point. So in the QEMU case, I should test
> - whether EFI_RNG_PROTOCOL exists
> - whether it implements the raw flavor
> 
> and only in that case, avoid virtio-rng (which only supports raw as well)
> 
>> But about the inital point, EFI_RNG_PRTOCOL should indeed not be registered
>> if no algorithm is available.
>>
> 
> Indeed.
> 
> And btw, I noticed that the TrngLib has a whole bunch of ASSERT()s
> that trigger when trying to use it on QEMU - can we rip those out as
> well please?

Yes ok, I will do that.