Re: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

Re: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Pedro Falcato]

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

Hi,

Ping. Is there a blocker here?

Pedro

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

Re: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Pedro Falcato]

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

> Pedro:
>   To keep the same behavior, I suggest to only update
> BaseRngLibConstructor() API with TestRdRand, don't touch other APIs.

Liming,
the point is to fix the library's broken behavior, as previously discussed
in the ML, and documented in the commit message and the bugzilla.
Only updating BaseRngLibConstructor would do absolutely nothing.
ArchIsRngSupported needs updating because it's bogus.
The individual ArchGetRandomNumberN() do not *need* updating, but it's a
good idea to make sure that "Existing software depends on this always
returning TRUE" isn't actually reality.

Pedro

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

Re: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Benjamin Doron]

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

Hi,
Is there merit to removing the assert statement? When this instance of the RngLib class is used, the platform builder says there's RNG support. Asserts are a little easier to see than debug prints, especially when they stall the platform. I think that leaving it in is better. If asserts don't call CpuDeadLoop() or are removed from the build entirely, then ` ArchIsRngSupported()` will ensure safe behaviour (but incorrect functionality).

Also, I'm slightly concerned that the check for minimum RDRAND changes will succeed if it returns 0s, then 1s, then 0s... Though this seems like an RDRAND broken too conveniently, not a true errata.

Regards,
Benjamin

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

Re: [edk2-devel] [PATCH v4 1/1] MdePkg/BaseRngLib: Add a smoketest for RDRAND and check CPUID

[Pedro Falcato]

On Wed, Apr 26, 2023 at 10:54 PM Benjamin Doron
<benjamin.doron00@gmail.com> wrote:
>
> Hi,
> Is there merit to removing the assert statement? When this instance of the RngLib class is used, the platform builder says there's RNG support. Asserts are a little easier to see than debug prints, especially when they stall the platform. I think that leaving it in is better. If asserts don't call CpuDeadLoop() or are removed from the build entirely, then `ArchIsRngSupported()` will ensure safe behaviour (but incorrect functionality).

Certain platforms may not know if the CPU has rdrand or if it's broken
at all. For instance, OVMF (1st case) or firmware for the AMD AM4
mobos (where they had zen1, 2 and 3 with some RDRAND breakage in
between).
Also, as you mentioned, ASSERTs can be entirely removed from the
build. So I don't think asserts are a good idea here. Unless you want
to suggest hiding the ASSERT with a PCD, in which case, yuck?

> Also, I'm slightly concerned that the check for minimum RDRAND changes will succeed if it returns 0s, then 1s, then 0s... Though this seems like an RDRAND broken too conveniently, not a true errata.

Ack, I don't think the algorithm is perfect, but it's what Jason wrote
for Linux and suggested; I guess it may be adjusted if we so need, but
at the moment it definitely detects the egregious AMD breakage.

-- 
Pedro