[PATCH v1 0/2] Fixing RngDxe error for ARM/AARCH64

[PATCH v1 0/2] Fixing RngDxe error for ARM/AARCH64

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491

On an ARM system that does not support firmware TRNG, the current logic
from RngDxe will cause the system to assert at the below line:
`ASSERT (Index != mAvailableAlgoArrayCount);`

The reason seems to be:
1. When initializing the number of `mAvailableAlgoArrayCount`, the logic
will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a
warning and still increment the counter because "RngGetBytes" might still
succeed:
https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3.
2. This will cause the main entry to publish the RNG protocol and accept
further usage.
3. However, during usage, the zero guid is always filtered out: 
https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91.
Thus, this will cause the system to always not able to find the algorithm
and fail the boot with an assert.

The suggestion is to at least make the logic of initializing
"mAvailableAlgoArrayCount" consistent and filtering algorithm consistent.

In addition, the usage of `mAvailableAlgoArray` will always trigger a
data abortion error, which is caused by buffer allocated is
`RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be
`RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM.

This patch fixed the 2 issues above. The change is verified on QEMU
virtual platform and proprietary physical platform.

Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>

Kun Qin (2):
  SecurityPkg: RngDxe: Unify handling of zero guid
  SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator

 SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 +++++----
 SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c         | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.41.0.windows.1

[PATCH v1 1/2] SecurityPkg: RngDxe: Unify handling of zero guid

[Kun Qin]

From: Kun Qin <kuqin@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491

The existing logic of initializing `mAvailableAlgoArrayCount` will treat
the zero GUID in `PcdCpuRngSupportedAlgorithm` as a legit case and
increment `mAvailableAlgoArrayCount`, causing the RNG protocol be
published.

However, when the protocol is invoked, any zero GUID will be filtered
out, leaving a possible edge case where the protocol only has a zero GUID
based algorithm and being filtered out will always result in an ASSERT.

This change marked the zero GUID as an issue and will not increment the
counter and thus avoid publishing the protocol completely.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>

Signed-off-by: Kun Qin <kuqin@microsoft.com>
---
 SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
index e8be217f8a8c..de279cdadeea 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
@@ -47,15 +47,16 @@ GetAvailableAlgorithms (
       );
     mAvailableAlgoArrayCount++;
 
-    DEBUG_CODE_BEGIN ();
     if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
+      DEBUG_CODE_BEGIN ();
       DEBUG ((
         DEBUG_WARN,
         "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n"
         ));
+
+      DEBUG_CODE_END ();
+      mAvailableAlgoArrayCount--;
     }
-
-    DEBUG_CODE_END ();
   }
 
   // Raw algorithm (Trng)
-- 
2.41.0.windows.1

[PATCH v1 2/2] SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator

[Kun Qin]

From: Kun Qin <kuqin@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491

mAvailableAlgoArray is currently allocated for "RNG_AVAILABLE_ALGO_MAX"
number of bytes, whereas it was dereferenced as "EFI_RNG_ALGORITHM".

This change fixed the buffer allocation logic by allocating a proper size
of buffer before referencing.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>

Signed-off-by: Kun Qin <kuqin@microsoft.com>
---
 SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 2 +-
 SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
index de279cdadeea..1d412ecff61d 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
@@ -33,7 +33,7 @@ GetAvailableAlgorithms (
   UINT16  MinorRevision;
 
   // Rng algorithms 2 times, one for the allocation, one to populate.
-  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
+  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX * sizeof (EFI_RNG_ALGORITHM));
   if (mAvailableAlgoArray == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
index 4b24f5c4a69b..5e621df601fb 100644
--- a/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
+++ b/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
@@ -32,7 +32,7 @@ GetAvailableAlgorithms (
   UINT16  MinorRevision;
 
   // Rng algorithms 2 times, one for the allocation, one to populate.
-  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
+  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX * sizeof (EFI_RNG_ALGORITHM));
   if (mAvailableAlgoArray == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
-- 
2.41.0.windows.1

Re: [PATCH v1 0/2] Fixing RngDxe error for ARM/AARCH64

[PierreGondois]

Hello Kun,

Thanks for the patch-set, there is another patch-set that also aims to fix the logic at:
https://edk2.groups.io/g/devel/message/104341

but I haven't got feedback so far. Would it be possible to try it out to see if
it also solves your issue ?

Regards,
Pierre

On 6/28/23 22:33, Kun Qin wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491
> 
> On an ARM system that does not support firmware TRNG, the current logic
> from RngDxe will cause the system to assert at the below line:
> `ASSERT (Index != mAvailableAlgoArrayCount);`
> 
> The reason seems to be:
> 1. When initializing the number of `mAvailableAlgoArrayCount`, the logic
> will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a
> warning and still increment the counter because "RngGetBytes" might still
> succeed:
> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3.
> 2. This will cause the main entry to publish the RNG protocol and accept
> further usage.
> 3. However, during usage, the zero guid is always filtered out:
> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91.
> Thus, this will cause the system to always not able to find the algorithm
> and fail the boot with an assert.
> 
> The suggestion is to at least make the logic of initializing
> "mAvailableAlgoArrayCount" consistent and filtering algorithm consistent.
> 
> In addition, the usage of `mAvailableAlgoArray` will always trigger a
> data abortion error, which is caused by buffer allocated is
> `RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be
> `RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM.
> 
> This patch fixed the 2 issues above. The change is verified on QEMU
> virtual platform and proprietary physical platform.
> 
> Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> 
> Kun Qin (2):
>    SecurityPkg: RngDxe: Unify handling of zero guid
>    SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator
> 
>   SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 +++++----
>   SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c         | 2 +-
>   2 files changed, 6 insertions(+), 5 deletions(-)
> 

Re: [PATCH v1 1/2] SecurityPkg: RngDxe: Unify handling of zero guid

[Sami Mujawar]

Hi Kun,

Can you look at Pierre's series and see if this issue is resolved, please?

I have also made further suggestions at 
https://edk2.groups.io/g/devel/message/106511.

Regards,

Sami Mujawar

On 28/06/2023 09:33 pm, Kun Qin wrote:
> From: Kun Qin <kuqin@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491
>
> The existing logic of initializing `mAvailableAlgoArrayCount` will treat
> the zero GUID in `PcdCpuRngSupportedAlgorithm` as a legit case and
> increment `mAvailableAlgoArrayCount`, causing the RNG protocol be
> published.
>
> However, when the protocol is invoked, any zero GUID will be filtered
> out, leaving a possible edge case where the protocol only has a zero GUID
> based algorithm and being filtered out will always result in an ASSERT.
>
> This change marked the zero GUID as an issue and will not increment the
> counter and thus avoid publishing the protocol completely.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
>
> Signed-off-by: Kun Qin <kuqin@microsoft.com>
> ---
>   SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> index e8be217f8a8c..de279cdadeea 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> @@ -47,15 +47,16 @@ GetAvailableAlgorithms (
>         );
>
>       mAvailableAlgoArrayCount++;
>
>   
>
> -    DEBUG_CODE_BEGIN ();
>
>       if (IsZeroGuid (PcdGetPtr (PcdCpuRngSupportedAlgorithm))) {
>
> +      DEBUG_CODE_BEGIN ();
>
>         DEBUG ((
>
>           DEBUG_WARN,
>
>           "PcdCpuRngSupportedAlgorithm should be a non-zero GUID\n"
>
>           ));
>
> +
>
> +      DEBUG_CODE_END ();
>
> +      mAvailableAlgoArrayCount--;
>
>       }
>
> -
>
> -    DEBUG_CODE_END ();
>
>     }
>
>   
>
>     // Raw algorithm (Trng)
>

Re: [PATCH v1 2/2] SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator

[Sami Mujawar]

Hi Kun,

Thank you for this fix.

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

Regards,

Sami Mujawar

On 28/06/2023 09:33 pm, Kun Qin wrote:
> From: Kun Qin <kuqin@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491
>
> mAvailableAlgoArray is currently allocated for "RNG_AVAILABLE_ALGO_MAX"
> number of bytes, whereas it was dereferenced as "EFI_RNG_ALGORITHM".
>
> This change fixed the buffer allocation logic by allocating a proper size
> of buffer before referencing.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
>
> Signed-off-by: Kun Qin <kuqin@microsoft.com>
> ---
>   SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 2 +-
>   SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> index de279cdadeea..1d412ecff61d 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c
> @@ -33,7 +33,7 @@ GetAvailableAlgorithms (
>     UINT16  MinorRevision;
>
>   
>
>     // Rng algorithms 2 times, one for the allocation, one to populate.
>
> -  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
>
> +  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX * sizeof (EFI_RNG_ALGORITHM));
>
>     if (mAvailableAlgoArray == NULL) {
>
>       return EFI_OUT_OF_RESOURCES;
>
>     }
>
> diff --git a/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c b/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
> index 4b24f5c4a69b..5e621df601fb 100644
> --- a/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
> +++ b/SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c
> @@ -32,7 +32,7 @@ GetAvailableAlgorithms (
>     UINT16  MinorRevision;
>
>   
>
>     // Rng algorithms 2 times, one for the allocation, one to populate.
>
> -  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX);
>
> +  mAvailableAlgoArray = AllocateZeroPool (RNG_AVAILABLE_ALGO_MAX * sizeof (EFI_RNG_ALGORITHM));
>
>     if (mAvailableAlgoArray == NULL) {
>
>       return EFI_OUT_OF_RESOURCES;
>
>     }
>

Re: [PATCH v1 0/2] Fixing RngDxe error for ARM/AARCH64

[Kun Qin]

Hi Pierre/Sami,

Thanks for the information. I tried the patch set you referred, it seems 
to have resolved
the zero GUID handling issue! So I am okay to drop the commit here:
https://edk2.groups.io/g/devel/message/106484

I also have a few questions on the patch set beyond functionality, we 
can discuss it there.

However, this specific patch is still needed to fix the access violation 
issue (thanks for the
reviewed-by tag, Sami):
https://edk2.groups.io/g/devel/message/106485

I can send a v2 to reflect the feedback above.

Thanks,
Kun

On 6/28/2023 11:43 PM, Pierre Gondois wrote:
> Hello Kun,
>
> Thanks for the patch-set, there is another patch-set that also aims to 
> fix the logic at:
> https://edk2.groups.io/g/devel/message/104341
>
> but I haven't got feedback so far. Would it be possible to try it out 
> to see if
> it also solves your issue ?
>
> Regards,
> Pierre
>
> On 6/28/23 22:33, Kun Qin wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4491
>>
>> On an ARM system that does not support firmware TRNG, the current logic
>> from RngDxe will cause the system to assert at the below line:
>> `ASSERT (Index != mAvailableAlgoArrayCount);`
>>
>> The reason seems to be:
>> 1. When initializing the number of `mAvailableAlgoArrayCount`, the logic
>> will only treat the zero guid of "PcdCpuRngSupportedAlgorithm" as a
>> warning and still increment the counter because "RngGetBytes" might 
>> still
>> succeed:
>> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c#L51C3-L51C3. 
>>
>> 2. This will cause the main entry to publish the RNG protocol and accept
>> further usage.
>> 3. However, during usage, the zero guid is always filtered out:
>> https://github.com/tianocore/edk2/blob/1a39bdf2c53858ebb39e6de1362203c65c163c63/SecurityPkg/RandomNumberGenerator/RngDxe/ArmRngDxe.c#L91. 
>>
>> Thus, this will cause the system to always not able to find the 
>> algorithm
>> and fail the boot with an assert.
>>
>> The suggestion is to at least make the logic of initializing
>> "mAvailableAlgoArrayCount" consistent and filtering algorithm 
>> consistent.
>>
>> In addition, the usage of `mAvailableAlgoArray` will always trigger a
>> data abortion error, which is caused by buffer allocated is
>> `RNG_AVAILABLE_ALGO_MAX` number of bytes, which should be
>> `RNG_AVAILABLE_ALGO_MAX` nubmer of EFI_RNG_ALGORITHM.
>>
>> This patch fixed the 2 issues above. The change is verified on QEMU
>> virtual platform and proprietary physical platform.
>>
>> Patch v1 branch: https://github.com/kuqin12/edk2/tree/fix_rng_edk2_v1
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>> Cc: Pierre Gondois <pierre.gondois@arm.com>
>>
>> Kun Qin (2):
>>    SecurityPkg: RngDxe: Unify handling of zero guid
>>    SecurityPkg: RngDxe: Fixing mAvailableAlgoArray allocator
>>
>>   SecurityPkg/RandomNumberGenerator/RngDxe/AArch64/AArch64Algo.c | 9 
>> +++++----
>>   SecurityPkg/RandomNumberGenerator/RngDxe/Arm/ArmAlgo.c | 2 +-
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>