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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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