[edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

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

All crypto host tests which consumed randlib broken due to:
https://github.com/tianocore/edk2/pull/5714
Not sure why this issue not reported  by CI when merge this PR.

The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
Then the Crypto API calls ```GetRandomNumber*``` will fail.
GetRandomNumber64 (
  OUT     UINT64  *Rand
  )
{
  ......
  if (!ArchIsRngSupported ()) {
    return FALSE;
  }

Is there a way to let unit test host to call the constructors correctly?

Regards,
Yi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119567): https://edk2.groups.io/g/devel/message/119567
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Yao, Jiewen]

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

Can we use a host test specific RngLib?



From: Li, Yi1 <yi1.li@intel.com>
Sent: Friday, June 14, 2024 3:08 PM
To: devel@edk2.groups.io
Cc: Hou, Wenxing <wenxing.hou@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: CryptoPkg host test broken due to smoketest for RDRAND

All crypto host tests which consumed randlib broken due to:
https://github.com/tianocore/edk2/pull/5714
Not sure why this issue not reported  by CI when merge this PR.

The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
Then the Crypto API calls ```GetRandomNumber*``` will fail.
GetRandomNumber64 (
  OUT     UINT64  *Rand
  )
{
  ......
  if (!ArchIsRngSupported ()) {
    return FALSE;
  }

Is there a way to let unit test host to call the constructors correctly?

Regards,
Yi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119568): https://edk2.groups.io/g/devel/message/119568
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

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

Yes, we can create a host test specific lib if no better ways.

Regards,
Yi

From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Friday, June 14, 2024 3:20 PM
To: Li, Yi1 <yi1.li@intel.com>; devel@edk2.groups.io
Cc: Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: RE: CryptoPkg host test broken due to smoketest for RDRAND

Can we use a host test specific RngLib?



From: Li, Yi1 <yi1.li@intel.com<mailto:yi1.li@intel.com>>
Sent: Friday, June 14, 2024 3:08 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Hou, Wenxing <wenxing.hou@intel.com<mailto:wenxing.hou@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
Subject: CryptoPkg host test broken due to smoketest for RDRAND

All crypto host tests which consumed randlib broken due to:
https://github.com/tianocore/edk2/pull/5714
Not sure why this issue not reported  by CI when merge this PR.

The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
Then the Crypto API calls ```GetRandomNumber*``` will fail.
GetRandomNumber64 (
  OUT     UINT64  *Rand
  )
{
  ......
  if (!ArchIsRngSupported ()) {
    return FALSE;
  }

Is there a way to let unit test host to call the constructors correctly?

Regards,
Yi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119569): https://edk2.groups.io/g/devel/message/119569
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Ard Biesheuvel]

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

For crypto unit tests, it is generally better to use a pseudo-random RNG,
with a known (but not constant) seed, so that potential failures can be
diagnosed more easily. E.g., the seed could be logged in the test output.


On Fri, 14 Jun 2024 at 09:24, Li, Yi1 <yi1.li@intel.com> wrote:

> Yes, we can create a host test specific lib if no better ways.
>
>
>
> Regards,
>
> Yi
>
>
>
> *From:* Yao, Jiewen <jiewen.yao@intel.com>
> *Sent:* Friday, June 14, 2024 3:20 PM
> *To:* Li, Yi1 <yi1.li@intel.com>; devel@edk2.groups.io
> *Cc:* Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <
> michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>
> *Subject:* RE: CryptoPkg host test broken due to smoketest for RDRAND
>
>
>
> Can we use a host test specific RngLib?
>
>
>
>
>
>
>
> *From:* Li, Yi1 <yi1.li@intel.com>
> *Sent:* Friday, June 14, 2024 3:08 PM
> *To:* devel@edk2.groups.io
> *Cc:* Hou, Wenxing <wenxing.hou@intel.com>; Yao, Jiewen <
> jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel <
> ardb+tianocore@kernel.org>
> *Subject:* CryptoPkg host test broken due to smoketest for RDRAND
>
>
>
> All crypto host tests which consumed randlib broken due to:
>
> https://github.com/tianocore/edk2/pull/5714
>
> Not sure why this issue not reported  by CI when merge this PR.
>
>
>
> The reason is that the ```BaseRngLibConstructor``` of rnglib is not called
> in host test, so ```mRdRandSupported``` is not enabled.
>
> Then the Crypto API calls ```GetRandomNumber*``` will fail.
>
> GetRandomNumber64 (
>
>   OUT     UINT64  *Rand
>
>   )
>
> {
>
>   ……
>
>   if (!ArchIsRngSupported ()) {
>
>     return FALSE;
>
>   }
>
>
>
> Is there a way to let unit test host to call the constructors correctly?
>
>
>
> Regards,
>
> Yi
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119570): https://edk2.groups.io/g/devel/message/119570
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Gerd Hoffmann]

On Fri, Jun 14, 2024 at 07:07:41AM GMT, Li, Yi wrote:
> All crypto host tests which consumed randlib broken due to:
> https://github.com/tianocore/edk2/pull/5714
> Not sure why this issue not reported  by CI when merge this PR.
> 
> The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
> Then the Crypto API calls ```GetRandomNumber*``` will fail.
> GetRandomNumber64 (
>   OUT     UINT64  *Rand
>   )
> {
>   ......
>   if (!ArchIsRngSupported ()) {
>     return FALSE;
>   }
> 
> Is there a way to let unit test host to call the constructors correctly?

https://github.com/tianocore/edk2/pull/5775

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119571): https://edk2.groups.io/g/devel/message/119571
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

Approved, appreciate your quick response.

Thanks,
Yi 

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Friday, June 14, 2024 6:41 PM
To: devel@edk2.groups.io; Li, Yi1 <yi1.li@intel.com>
Cc: Hou, Wenxing <wenxing.hou@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

On Fri, Jun 14, 2024 at 07:07:41AM GMT, Li, Yi wrote:
> All crypto host tests which consumed randlib broken due to:
> https://github.com/tianocore/edk2/pull/5714
> Not sure why this issue not reported  by CI when merge this PR.
> 
> The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
> Then the Crypto API calls ```GetRandomNumber*``` will fail.
> GetRandomNumber64 (
>   OUT     UINT64  *Rand
>   )
> {
>   ......
>   if (!ArchIsRngSupported ()) {
>     return FALSE;
>   }
> 
> Is there a way to let unit test host to call the constructors correctly?

https://github.com/tianocore/edk2/pull/5775

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119572): https://edk2.groups.io/g/devel/message/119572
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

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

Yes we did use PRNG(DRBG), the entropy pool is polled from HW RngLib. Seed is generated by TSC.

Regards,
Yi

From: Ard Biesheuvel <ardb@kernel.org>
Sent: Friday, June 14, 2024 3:52 PM
To: Li, Yi1 <yi1.li@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Subject: Re: CryptoPkg host test broken due to smoketest for RDRAND

For crypto unit tests, it is generally better to use a pseudo-random RNG, with a known (but not constant) seed, so that potential failures can be diagnosed more easily. E.g., the seed could be logged in the test output.


On Fri, 14 Jun 2024 at 09:24, Li, Yi1 <yi1.li@intel.com<mailto:yi1.li@intel.com>> wrote:
Yes, we can create a host test specific lib if no better ways.

Regards,
Yi

From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Friday, June 14, 2024 3:20 PM
To: Li, Yi1 <yi1.li@intel.com<mailto:yi1.li@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Hou, Wenxing <wenxing.hou@intel.com<mailto:wenxing.hou@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb%2Btianocore@kernel.org>>
Subject: RE: CryptoPkg host test broken due to smoketest for RDRAND

Can we use a host test specific RngLib?



From: Li, Yi1 <yi1.li@intel.com<mailto:yi1.li@intel.com>>
Sent: Friday, June 14, 2024 3:08 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Hou, Wenxing <wenxing.hou@intel.com<mailto:wenxing.hou@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>
Subject: CryptoPkg host test broken due to smoketest for RDRAND

All crypto host tests which consumed randlib broken due to:
https://github.com/tianocore/edk2/pull/5714
Not sure why this issue not reported  by CI when merge this PR.

The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host test, so ```mRdRandSupported``` is not enabled.
Then the Crypto API calls ```GetRandomNumber*``` will fail.
GetRandomNumber64 (
  OUT     UINT64  *Rand
  )
{
  ……
  if (!ArchIsRngSupported ()) {
    return FALSE;
  }

Is there a way to let unit test host to call the constructors correctly?

Regards,
Yi



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119573): https://edk2.groups.io/g/devel/message/119573
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Yao, Jiewen]

Hey
This PR seems just a workaround.

I don't feel it is right solution to hardcode BIT30.
What if the host platform does not have such capability? You will get failure later.

To fix this function, can we call real CPUID instruction to return real value?


UINT32
EFIAPI
UnitTestHostBaseLibAsmCpuid (
  IN      UINT32  Index,
  OUT     UINT32  *Eax   OPTIONAL,
  OUT     UINT32  *Ebx   OPTIONAL,
  OUT     UINT32  *Ecx   OPTIONAL,
  OUT     UINT32  *Edx   OPTIONAL
  )
{
  UINT32  RetEcx;

  RetEcx = 0;
  switch (Index) {
    case 1:
      RetEcx |= BIT30; /* RdRand */
      break;
  }

  if (Eax != NULL) {
    *Eax = 0;
  }
  if (Ebx != NULL) {
    *Ebx = 0;
  }

  if (Ecx != NULL) {
    *Ecx = RetEcx;
  }

  if (Edx != NULL) {
    *Edx = 0;
  }
  return Index;
}

> -----Original Message-----
> From: Li, Yi1 <yi1.li@intel.com>
> Sent: Friday, June 14, 2024 9:32 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io
> Cc: Hou, Wenxing <wenxing.hou@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>
> Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for
> RDRAND
> 
> Approved, appreciate your quick response.
> 
> Thanks,
> Yi
> 
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, June 14, 2024 6:41 PM
> To: devel@edk2.groups.io; Li, Yi1 <yi1.li@intel.com>
> Cc: Hou, Wenxing <wenxing.hou@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>
> Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for
> RDRAND
> 
> On Fri, Jun 14, 2024 at 07:07:41AM GMT, Li, Yi wrote:
> > All crypto host tests which consumed randlib broken due to:
> > https://github.com/tianocore/edk2/pull/5714
> > Not sure why this issue not reported  by CI when merge this PR.
> >
> > The reason is that the ```BaseRngLibConstructor``` of rnglib is not called in host
> test, so ```mRdRandSupported``` is not enabled.
> > Then the Crypto API calls ```GetRandomNumber*``` will fail.
> > GetRandomNumber64 (
> >   OUT     UINT64  *Rand
> >   )
> > {
> >   ......
> >   if (!ArchIsRngSupported ()) {
> >     return FALSE;
> >   }
> >
> > Is there a way to let unit test host to call the constructors correctly?
> 
> https://github.com/tianocore/edk2/pull/5775
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119574): https://edk2.groups.io/g/devel/message/119574
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Ard Biesheuvel]

On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> Hey
> This PR seems just a workaround.
>
> I don't feel it is right solution to hardcode BIT30.
> What if the host platform does not have such capability? You will get failure later.
>

Agreed. But that was already the case: RngLib assumed that RDRAND was
implemented without checking CPUID at all, and so the code was already
broken on systems without RDRAND.

>
> To fix this function, can we call real CPUID instruction to return real value?
>

That would be better. But this change just restores the old behavior.
And on top of that, Yi Li already merged it.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119575): https://edk2.groups.io/g/devel/message/119575
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Yao, Jiewen]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Saturday, June 15, 2024 12:14 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
> Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for
> RDRAND
> 
> On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > Hey
> > This PR seems just a workaround.
> >
> > I don't feel it is right solution to hardcode BIT30.
> > What if the host platform does not have such capability? You will get failure
> later.
> >
> 
> Agreed. But that was already the case: RngLib assumed that RDRAND was
> implemented without checking CPUID at all, and so the code was already
> broken on systems without RDRAND.

[Jiewen] Sorry, I don’t understand your comment. " implemented without checking CPUID at all "

See below code. It does use CPUID to check the capability.

EFI_STATUS
EFIAPI
BaseRngLibConstructor (
  VOID
  )
{
  UINT32  RegEcx;

  //
  // Determine RDRAND support by examining bit 30 of the ECX register returned by
  // CPUID. A value of 1 indicates that processor support RDRAND instruction.
  //
  AsmCpuid (1, 0, 0, &RegEcx, 0);

  mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);

  if (mRdRandSupported) {
    mRdRandSupported = TestRdRand ();
  }

  return EFI_SUCCESS;
}


> 
> >
> > To fix this function, can we call real CPUID instruction to return real value?
> >
> 
> That would be better. But this change just restores the old behavior.
> And on top of that, Yi Li already merged it.

[Jiewen] I don’t think it is right to merge it without thorough review.

I think we need follow 24 hour rule.
Any patch requires at least 24 hours before merge, to give people chance to review and feedback.







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119576): https://edk2.groups.io/g/devel/message/119576
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Ard Biesheuvel]

On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Saturday, June 15, 2024 12:14 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
> > Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for
> > RDRAND
> >
> > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > >
> > > Hey
> > > This PR seems just a workaround.
> > >
> > > I don't feel it is right solution to hardcode BIT30.
> > > What if the host platform does not have such capability? You will get failure
> > later.
> > >
> >
> > Agreed. But that was already the case: RngLib assumed that RDRAND was
> > implemented without checking CPUID at all, and so the code was already
> > broken on systems without RDRAND.
>
> [Jiewen] Sorry, I don’t understand your comment. " implemented without checking CPUID at all "
>
> See below code. It does use CPUID to check the capability.
>
> EFI_STATUS
> EFIAPI
> BaseRngLibConstructor (
>   VOID
>   )
> {
>   UINT32  RegEcx;
>
>   //
>   // Determine RDRAND support by examining bit 30 of the ECX register returned by
>   // CPUID. A value of 1 indicates that processor support RDRAND instruction.
>   //
>   AsmCpuid (1, 0, 0, &RegEcx, 0);
>
>   mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);
>
>   if (mRdRandSupported) {
>     mRdRandSupported = TestRdRand ();
>   }
>
>   return EFI_SUCCESS;
> }
>
>

See commit 9301e5644cef5a5234f71b178373dd508cabb9ee

The old code had

+BOOLEAN
+EFIAPI
+ArchIsRngSupported (
+  VOID
+  )
+{
+  /*
+     Existing software depends on this always returning TRUE, so for
+     now hard-code it.
+
+     return mRdRandSupported;
+  */
+  return TRUE;
+}



> >
> > >
> > > To fix this function, can we call real CPUID instruction to return real value?
> > >
> >
> > That would be better. But this change just restores the old behavior.
> > And on top of that, Yi Li already merged it.
>
> [Jiewen] I don’t think it is right to merge it without thorough review.
>
> I think we need follow 24 hour rule.
> Any patch requires at least 24 hours before merge, to give people chance to review and feedback.
>

Agreed.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119579): https://edk2.groups.io/g/devel/message/119579
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

> I think we need follow 24 hour rule.
> Any patch requires at least 24 hours before merge, to give people chance to review and feedback.

Sure I will follow this rule future.
There is a critical Mbedtls bug based on this patch (blocked build) so I merged this patch quickly to unblock CI.
https://github.com/tianocore/edk2/pull/5773


Thanks,
Yi

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com> 
Sent: Saturday, June 15, 2024 12:45 AM
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND


> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Saturday, June 15, 2024 12:14 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Pedro Falcato 
> <pedro.falcato@gmail.com>
> Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest 
> for RDRAND
> 
> On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > Hey
> > This PR seems just a workaround.
> >
> > I don't feel it is right solution to hardcode BIT30.
> > What if the host platform does not have such capability? You will 
> > get failure
> later.
> >
> 
> Agreed. But that was already the case: RngLib assumed that RDRAND was 
> implemented without checking CPUID at all, and so the code was already 
> broken on systems without RDRAND.

[Jiewen] Sorry, I don’t understand your comment. " implemented without checking CPUID at all "

See below code. It does use CPUID to check the capability.

EFI_STATUS
EFIAPI
BaseRngLibConstructor (
  VOID
  )
{
  UINT32  RegEcx;

  //
  // Determine RDRAND support by examining bit 30 of the ECX register returned by
  // CPUID. A value of 1 indicates that processor support RDRAND instruction.
  //
  AsmCpuid (1, 0, 0, &RegEcx, 0);

  mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);

  if (mRdRandSupported) {
    mRdRandSupported = TestRdRand ();
  }

  return EFI_SUCCESS;
}


> 
> >
> > To fix this function, can we call real CPUID instruction to return real value?
> >
> 
> That would be better. But this change just restores the old behavior.
> And on top of that, Yi Li already merged it.

[Jiewen] I don’t think it is right to merge it without thorough review.

I think we need follow 24 hour rule.
Any patch requires at least 24 hours before merge, to give people chance to review and feedback.







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119582): https://edk2.groups.io/g/devel/message/119582
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

Hi Jiewen,

Currently Host lib using a dummy AsmCpuid implementation:
BaseLib\X86UnitTestHost.c
AsmCpuid -> UnitTestHostBaseLibAsmCpuid -> Return all zero (BIT30 of ECX hardcode to 1 after change of Gerd)

Did you mean prefer to use real AsmCpuid func in Host?
Or only use cpuid to check RdRand bit and set it.

Regards,
Yi

-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org> 
Sent: Saturday, June 15, 2024 1:16 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Saturday, June 15, 2024 12:14 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> > devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, 
> > Michael D <michael.d.kinney@intel.com>; Pedro Falcato 
> > <pedro.falcato@gmail.com>
> > Subject: Re: [edk2-devel] CryptoPkg host test broken due to 
> > smoketest for RDRAND
> >
> > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > >
> > > Hey
> > > This PR seems just a workaround.
> > >
> > > I don't feel it is right solution to hardcode BIT30.
> > > What if the host platform does not have such capability? You will 
> > > get failure
> > later.
> > >
> >
> > Agreed. But that was already the case: RngLib assumed that RDRAND 
> > was implemented without checking CPUID at all, and so the code was 
> > already broken on systems without RDRAND.
>
> [Jiewen] Sorry, I don’t understand your comment. " implemented without checking CPUID at all "
>
> See below code. It does use CPUID to check the capability.
>
> EFI_STATUS
> EFIAPI
> BaseRngLibConstructor (
>   VOID
>   )
> {
>   UINT32  RegEcx;
>
>   //
>   // Determine RDRAND support by examining bit 30 of the ECX register returned by
>   // CPUID. A value of 1 indicates that processor support RDRAND instruction.
>   //
>   AsmCpuid (1, 0, 0, &RegEcx, 0);
>
>   mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);
>
>   if (mRdRandSupported) {
>     mRdRandSupported = TestRdRand ();
>   }
>
>   return EFI_SUCCESS;
> }
>
>

See commit 9301e5644cef5a5234f71b178373dd508cabb9ee

The old code had

+BOOLEAN
+EFIAPI
+ArchIsRngSupported (
+  VOID
+  )
+{
+  /*
+     Existing software depends on this always returning TRUE, so for
+     now hard-code it.
+
+     return mRdRandSupported;
+  */
+  return TRUE;
+}



> >
> > >
> > > To fix this function, can we call real CPUID instruction to return real value?
> > >
> >
> > That would be better. But this change just restores the old behavior.
> > And on top of that, Yi Li already merged it.
>
> [Jiewen] I don’t think it is right to merge it without thorough review.
>
> I think we need follow 24 hour rule.
> Any patch requires at least 24 hours before merge, to give people chance to review and feedback.
>

Agreed.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119585): https://edk2.groups.io/g/devel/message/119585
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Michael D Kinney]

If the host test was updated to use GoogleTest/GoogleMock, then
the call to AsmCpuid() could be mocked instead of calling the
version of BaseLib that is safe to use from host envs.  Then
all the code paths can be tested properly.

Mike

> -----Original Message-----
> From: Li, Yi1 <yi1.li@intel.com>
> Sent: Friday, June 14, 2024 9:55 PM
> To: Ard Biesheuvel <ardb@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, Wenxing
> <wenxing.hou@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Pedro Falcato <pedro.falcato@gmail.com>
> Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for
> RDRAND
> 
> Hi Jiewen,
> 
> Currently Host lib using a dummy AsmCpuid implementation:
> BaseLib\X86UnitTestHost.c
> AsmCpuid -> UnitTestHostBaseLibAsmCpuid -> Return all zero (BIT30 of ECX
> hardcode to 1 after change of Gerd)
> 
> Did you mean prefer to use real AsmCpuid func in Host?
> Or only use cpuid to check RdRand bit and set it.
> 
> Regards,
> Yi
> 
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Saturday, June 15, 2024 1:16 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
> Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest for
> RDRAND
> 
> On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Saturday, June 15, 2024 12:14 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > > devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Pedro Falcato
> > > <pedro.falcato@gmail.com>
> > > Subject: Re: [edk2-devel] CryptoPkg host test broken due to
> > > smoketest for RDRAND
> > >
> > > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > > >
> > > > Hey
> > > > This PR seems just a workaround.
> > > >
> > > > I don't feel it is right solution to hardcode BIT30.
> > > > What if the host platform does not have such capability? You will
> > > > get failure
> > > later.
> > > >
> > >
> > > Agreed. But that was already the case: RngLib assumed that RDRAND
> > > was implemented without checking CPUID at all, and so the code was
> > > already broken on systems without RDRAND.
> >
> > [Jiewen] Sorry, I don’t understand your comment. " implemented without
> checking CPUID at all "
> >
> > See below code. It does use CPUID to check the capability.
> >
> > EFI_STATUS
> > EFIAPI
> > BaseRngLibConstructor (
> >   VOID
> >   )
> > {
> >   UINT32  RegEcx;
> >
> >   //
> >   // Determine RDRAND support by examining bit 30 of the ECX register
> returned by
> >   // CPUID. A value of 1 indicates that processor support RDRAND
> instruction.
> >   //
> >   AsmCpuid (1, 0, 0, &RegEcx, 0);
> >
> >   mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);
> >
> >   if (mRdRandSupported) {
> >     mRdRandSupported = TestRdRand ();
> >   }
> >
> >   return EFI_SUCCESS;
> > }
> >
> >
> 
> See commit 9301e5644cef5a5234f71b178373dd508cabb9ee
> 
> The old code had
> 
> +BOOLEAN
> +EFIAPI
> +ArchIsRngSupported (
> +  VOID
> +  )
> +{
> +  /*
> +     Existing software depends on this always returning TRUE, so for
> +     now hard-code it.
> +
> +     return mRdRandSupported;
> +  */
> +  return TRUE;
> +}
> 
> 
> 
> > >
> > > >
> > > > To fix this function, can we call real CPUID instruction to return real
> value?
> > > >
> > >
> > > That would be better. But this change just restores the old behavior.
> > > And on top of that, Yi Li already merged it.
> >
> > [Jiewen] I don’t think it is right to merge it without thorough review.
> >
> > I think we need follow 24 hour rule.
> > Any patch requires at least 24 hours before merge, to give people chance to
> review and feedback.
> >
> 
> Agreed.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119586): https://edk2.groups.io/g/devel/message/119586
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

[Li, Yi]

Sounds good, I will try it.

Thanks,
Yi

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Saturday, June 15, 2024 12:58 PM
To: Li, Yi1 <yi1.li@intel.com>; Ard Biesheuvel <ardb@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest for RDRAND

If the host test was updated to use GoogleTest/GoogleMock, then the call to AsmCpuid() could be mocked instead of calling the version of BaseLib that is safe to use from host envs.  Then all the code paths can be tested properly.

Mike

> -----Original Message-----
> From: Li, Yi1 <yi1.li@intel.com>
> Sent: Friday, June 14, 2024 9:55 PM
> To: Ard Biesheuvel <ardb@kernel.org>; Yao, Jiewen 
> <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Hou, 
> Wenxing <wenxing.hou@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
> Subject: RE: [edk2-devel] CryptoPkg host test broken due to smoketest 
> for RDRAND
> 
> Hi Jiewen,
> 
> Currently Host lib using a dummy AsmCpuid implementation:
> BaseLib\X86UnitTestHost.c
> AsmCpuid -> UnitTestHostBaseLibAsmCpuid -> Return all zero (BIT30 of 
> ECX hardcode to 1 after change of Gerd)
> 
> Did you mean prefer to use real AsmCpuid func in Host?
> Or only use cpuid to check RdRand bit and set it.
> 
> Regards,
> Yi
> 
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Saturday, June 15, 2024 1:16 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Pedro Falcato 
> <pedro.falcato@gmail.com>
> Subject: Re: [edk2-devel] CryptoPkg host test broken due to smoketest 
> for RDRAND
> 
> On Fri, 14 Jun 2024 at 18:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Saturday, June 15, 2024 12:14 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: Li, Yi1 <yi1.li@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> > > devel@edk2.groups.io; Hou, Wenxing <wenxing.hou@intel.com>; 
> > > Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato 
> > > <pedro.falcato@gmail.com>
> > > Subject: Re: [edk2-devel] CryptoPkg host test broken due to 
> > > smoketest for RDRAND
> > >
> > > On Fri, 14 Jun 2024 at 18:09, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> > > >
> > > > Hey
> > > > This PR seems just a workaround.
> > > >
> > > > I don't feel it is right solution to hardcode BIT30.
> > > > What if the host platform does not have such capability? You 
> > > > will get failure
> > > later.
> > > >
> > >
> > > Agreed. But that was already the case: RngLib assumed that RDRAND 
> > > was implemented without checking CPUID at all, and so the code was 
> > > already broken on systems without RDRAND.
> >
> > [Jiewen] Sorry, I don’t understand your comment. " implemented 
> > without
> checking CPUID at all "
> >
> > See below code. It does use CPUID to check the capability.
> >
> > EFI_STATUS
> > EFIAPI
> > BaseRngLibConstructor (
> >   VOID
> >   )
> > {
> >   UINT32  RegEcx;
> >
> >   //
> >   // Determine RDRAND support by examining bit 30 of the ECX 
> > register
> returned by
> >   // CPUID. A value of 1 indicates that processor support RDRAND
> instruction.
> >   //
> >   AsmCpuid (1, 0, 0, &RegEcx, 0);
> >
> >   mRdRandSupported = ((RegEcx & RDRAND_MASK) == RDRAND_MASK);
> >
> >   if (mRdRandSupported) {
> >     mRdRandSupported = TestRdRand ();
> >   }
> >
> >   return EFI_SUCCESS;
> > }
> >
> >
> 
> See commit 9301e5644cef5a5234f71b178373dd508cabb9ee
> 
> The old code had
> 
> +BOOLEAN
> +EFIAPI
> +ArchIsRngSupported (
> +  VOID
> +  )
> +{
> +  /*
> +     Existing software depends on this always returning TRUE, so for
> +     now hard-code it.
> +
> +     return mRdRandSupported;
> +  */
> +  return TRUE;
> +}
> 
> 
> 
> > >
> > > >
> > > > To fix this function, can we call real CPUID instruction to 
> > > > return real
> value?
> > > >
> > >
> > > That would be better. But this change just restores the old behavior.
> > > And on top of that, Yi Li already merged it.
> >
> > [Jiewen] I don’t think it is right to merge it without thorough review.
> >
> > I think we need follow 24 hour rule.
> > Any patch requires at least 24 hours before merge, to give people 
> > chance to
> review and feedback.
> >
> 
> Agreed.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119587): https://edk2.groups.io/g/devel/message/119587
Mute This Topic: https://groups.io/mt/106666288/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-