OK. The interface ParallelHash256HashAll() seems OK for me.
Some comment:
1) Please add EFIAPI for ParallelHash256HashAll().
2) Please use BOOLEAN as return value.
3) Please remove LeftEncode() and RightEncode() from
https://github.com/zhihaoli1064/edk2/blob/master/CryptoPkg/Include/Library/BaseCryptLib.h.
They shall be internal functions.
4) Do we really need expose cSHAKE* API? If the request is just to support
ParallelHash256, then cSHAKE API could be internal one.
5) I suggest to make this API work in non-MP case as well. In current version,
it will fail for non-MP system, right? I think we can let BSP do all the work
in that case.
if (StartedApNum == 0) {
ReturnValue = 1;
goto Exit;
}
Thank you
Yao Jiewen
From: Li, Zhihao <[email protected]>
Sent: Tuesday, September 14, 2021 12:03 PM
To: Yao, Jiewen <[email protected]>; Andrew Fish <[email protected]>; Ethin
Probst <[email protected]>; Kinney, Michael D
<[email protected]>; edk2-devel-groups-io <[email protected]>
Cc: Wang, Jian J <[email protected]>; Wu, Hao A <[email protected]>; Lu,
XiaoyuX <[email protected]>; Jiang, Guomin <[email protected]>;
[email protected]; Fu, Siyuan <[email protected]>; Wu, Yidong
<[email protected]>; Li, Aaron <[email protected]>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Hi, Jiewen
Thanks for your suggestions.
In view of your advice, this RFC only talk about the new feature----Parallel
hash.
Mainly modification in
https://github.com/zhihaoli1064/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/Hash/Smm/ParallelHashSmm.c
Others will be designed anew.
From: Yao, Jiewen <[email protected]<mailto:[email protected]>>
Sent: Thursday, September 9, 2021 6:04 PM
To: Li, Zhihao <[email protected]<mailto:[email protected]>>; Andrew Fish
<[email protected]<mailto:[email protected]>>; Ethin Probst
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>;
edk2-devel-groups-io <[email protected]<mailto:[email protected]>>
Cc: Wang, Jian J <[email protected]<mailto:[email protected]>>; Wu, Hao
A <[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Hi
AllowList and DenyList are *secure boot* concept.
FMP auth lib is for *signed capsule* and it does not consider secure boot –
allow list and deny list.
In my mind, neither secure boot and FSP auth shall know the existence of
parallel hash. The verification logic shall be isolated.
Sorry, I don’t understand the design.
I am worried that you put too many concept together.
Adding parallel hash is one thing.
Adding allow list and deny list to FMP is another thing.
Please don’t mix them together.
I would like to request a design review for this feature.
Thank you
Yao Jiewen
From: Li, Zhihao <[email protected]<mailto:[email protected]>>
Sent: Thursday, September 9, 2021 5:49 PM
To: Yao, Jiewen <[email protected]<mailto:[email protected]>>; Andrew
Fish <[email protected]<mailto:[email protected]>>; Ethin Probst
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>;
edk2-devel-groups-io <[email protected]<mailto:[email protected]>>
Cc: Wang, Jian J <[email protected]<mailto:[email protected]>>; Wu, Hao
A <[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
I send this mail for asking that if there are any comments about parallel hash
feature.
Mainly modification:
CryptoPkg:
https://github.com/zhihaoli1064/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/Hash/Smm/ParallelHashSmm.c
MdeMoudulePkg:
https://github.com/zhihaoli1064/edk2/blob/master/MdeModulePkg/Include/Library/FmpAuthenticationLib.h
line59-67
SecurityPkg:
https://github.com/zhihaoli1064/edk2/blob/master/SecurityPkg/Library/FmpAuthenticationLibPkcs7/FmpAuthenticationLibPkcs7.c
line119-188,287-350
From: Li, Zhihao
Sent: Friday, September 3, 2021 4:44 PM
To: Yao, Jiewen <[email protected]<mailto:[email protected]>>; Andrew
Fish <[email protected]<mailto:[email protected]>>; edk2-devel-groups-io
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Wang, Jian J <[email protected]<mailto:[email protected]>>; Wu, Hao
A <[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Hi, Jiewen
I try to explant what means “more than once authentication(e.g.
VerifyImageWithDenylist and VerifyImageWithAllowlist)”.
When we confirm the image is effective, we have to confirm not only that image
certificate is on the whitelist, but also that it is not on the blacklist.
So it have two steps in verification process(only talk about
FmpAuthentication)----- VerifyImageWithDenylist and VerifyImageWithAllowlist.
VerifyImageWithDenylist confirms it not in blacklist while
VerifyImageWithAllowlist confirms it in whitelist.
==>VerifyImageWithDenylist should do FmpAuthentication and failed.
VerifyImageWithAllowlist Should do FmpAuthentication and success.
In our design:
Result=parallelhash256(image);------①
Status1= VerifyImageWithDenylist(image,result);---------②
Status2= VerifyImageWithAllowlist(image,result);---------③
Status1 is failed, status2 is success==>image is effective.
If do it inside of AuthenticateFmpImage
In step ②，it need do parallelhash256(image) .
And in step ③，it also need do parallelhash256(image) .
Because AuthenticateFmpImage Function is inside of VerifyImageWithDenylist And
VerifyImageWithAllowlist.
Poc code link of edk2:
https://github.com/zhihaoli1064/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/Hash/Smm/ParallelHashSmm.c
From: Yao, Jiewen <[email protected]<mailto:[email protected]>>
Sent: Friday, September 3, 2021 3:07 PM
To: Li, Zhihao <[email protected]<mailto:[email protected]>>; Andrew Fish
<[email protected]<mailto:[email protected]>>; edk2-devel-groups-io
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Wang, Jian J <[email protected]<mailto:[email protected]>>; Wu, Hao
A <[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Sorry, I hardly understand the explanation.
Do you have a URL for the POC code?
From: Li, Zhihao <[email protected]<mailto:[email protected]>>
Sent: Friday, September 3, 2021 2:58 PM
To: Yao, Jiewen <[email protected]<mailto:[email protected]>>; Andrew
Fish <[email protected]<mailto:[email protected]>>; edk2-devel-groups-io
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Wang, Jian J <[email protected]<mailto:[email protected]>>; Wu, Hao
A <[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Hi
Some explanation for confusion.
1. Is the result of the parallel hash identical to the current hash?
The result of parallelhash256 do not identical to the current hash. And we are
not intention to let parallelhash256 replace the current hash(SHA-256). But
doing the parallel hash before the current hash to reduce the size of current
hash input. Otherwise, the parallel hash effect is compressing the size of
FmpAuthentication input and the use of MP Services is the inseparable part of
this algorithm. It’s a new hash algorithm. So it should not move to
FmpAuthenticationLib.
1. Why we cannot do it inside of AuthenticateFmpImage?
Because of more than once authentication(e.g. VerifyImageWithDenylist and
VerifyImageWithAllowlist), if we do the parallel hash inside of
AuthenticateFmpImage(Denylist auth), we have to do another parallel hash for
Allowlist’s AuthenticateFmpImage. It’s repeat operation.
Poc code in branch named dev/sfu5/parallel_hash_ossl
The verify flow is:
ImageParaHash = ParallelHash-256 (Image)
PKCS7_Verify (PublicKey, ImageParaHash)
In FmpAuthenticationLibPkcs7 ,the parameter Output of
FmpAuthenticatedHandlerPkcs7WithParallelhash is image digest. It replace the
original image.
FmpAuthenticatedHandlerPkcs7WithParallelhash (
IN EFI_FIRMWARE_IMAGE_AUTHENTICATION *Image,
IN UINTN ImageSize,
IN CONST UINT8 *PublicKeyData,
IN UINTN PublicKeyDataLength,
IN UINT8 *Output
)
{
RETURN_STATUS Status;
BOOLEAN CryptoStatus;
VOID *P7Data;
UINTN P7Length;
VOID *TempBuffer;
UINTN PayloadHeaderSize = 69;
UINTN ParallelhashSize = 64;
P7Length = Image->AuthInfo.Hdr.dwLength -
(OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData));
P7Data = Image->AuthInfo.CertData;
// It is a signature across the variable data and the Monotonic Count value.
TempBuffer = AllocatePool(sizeof(Image->MonotonicCount) + ParallelhashSize +
PayloadHeaderSize);
CopyMem(
(UINT8 *)TempBuffer,
(UINT8 *)Image + sizeof(Image->MonotonicCount) +
Image->AuthInfo.Hdr.dwLength,
PayloadHeaderSize
);
CopyMem(
(UINT8 *)TempBuffer + PayloadHeaderSize,
Output,
ParallelhashSize
);
CopyMem(
(UINT8 *)TempBuffer + PayloadHeaderSize + ParallelhashSize,
&Image->MonotonicCount,
sizeof(Image->MonotonicCount)
);
CryptoStatus = Pkcs7Verify(
P7Data,
P7Length,
PublicKeyData,
PublicKeyDataLength,
(UINT8 *)TempBuffer,
PayloadHeaderSize + ParallelhashSize +
sizeof(Image->MonotonicCount)
);
FreePool(TempBuffer);
From: Yao, Jiewen <[email protected]<mailto:[email protected]>>
Sent: Friday, September 3, 2021 9:02 AM
To: Andrew Fish <[email protected]<mailto:[email protected]>>; edk2-devel-groups-io
<[email protected]<mailto:[email protected]>>; Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Li, Zhihao <[email protected]<mailto:[email protected]>>; Wang, Jian J
<[email protected]<mailto:[email protected]>>; Wu, Hao A
<[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: RE: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
Hi
Comment on 2/3.
I am not sure if the a new function AuthenticateFmpImageWithParallelhash() is
absolutely necessary.
Why you do the parallel hash before authentication and transfer the result to
AuthenticateFmpImage?
Why we cannot do it inside of AuthenticateFmpImage?
Ideally, we hope to hide *algorithm* from *business logic*.
Do you have any POC link?
Thank you
Yao Jiewen
From: Andrew Fish <[email protected]<mailto:[email protected]>>
Sent: Friday, September 3, 2021 7:16 AM
To: edk2-devel-groups-io <[email protected]<mailto:[email protected]>>;
Kinney, Michael D
<[email protected]<mailto:[email protected]>>
Cc: Li, Zhihao <[email protected]<mailto:[email protected]>>; Yao, Jiewen
<[email protected]<mailto:[email protected]>>; Wang, Jian J
<[email protected]<mailto:[email protected]>>; Wu, Hao A
<[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: Re: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib.
On Sep 2, 2021, at 8:50 AM, Michael D Kinney
<[email protected]<mailto:[email protected]>> wrote:
Hi Zhihao,
Is the result of the parallel hash identical to the current hash? If so, then
can we simply have a new instance of the FmpAuthenticationLib and hide the
ParallelHash256 digest inside this implementation of this new instance?
I do not think BaseCryptLib should depend on CPU MP Services Protocol. Can the
use of MP Services be moved up into the implementation of the new
FmpAuthenticationLib? If new BASE compatible primitives need to be added to
BaseCryptLib to support parallel hash, then those likely make sense.
Mike,
Stupid question but the BaseCryptLib seems to really be DxeCryptLib[1]? So are
you worried about adding the dependency to this DXE Lib? It depends on
UefiRuntimeServicesTableLib. Looks like SysCall/TimerWrapper.c[2] uses
gRT->GetTime(). It looks like if the time services are not available it returns
0 from time(), so there is only a quality of service implication to when it it
is used but no Depex?
How do you decide how many CPU threads to use?
If we end up splitting this up for “others” to handle the MP in DXE, PEI, or MM
then I think we probably need a more robust API set that abstracts breaking up
the work, and combining it back tougher. Well you would need the worker
functions to processes the broken up data on the APs. So I would imagine and
API that splits the work and you pass in the number of APs (or APs + BSP) and
you get N buffers out to process? Those buffers should describe the chunk to
the worker function, and when the worker function is done the get the answer
function can calculate the results from that.
[1] We don’t have a Base implementation of BaseCryptLib.
CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
LIBRARY_CLASS = BaseCryptLib|DXE_DRIVER DXE_CORE
UEFI_APPLICATION UEFI_DRIVER
CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
LIBRARY_CLASS = BaseCryptLib|PEIM PEI_CORE
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
LIBRARY_CLASS = BaseCryptLib|DXE_RUNTIME_DRIVER
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
LIBRARY_CLASS = BaseCryptLib|DXE_SMM_DRIVER SMM_CORE
MM_STANDALONE
[2]
https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c#L77
Thanks,
Andrew Fish
Thanks,
Mike
From: [email protected]<mailto:[email protected]>
<[email protected]<mailto:[email protected]>> On Behalf Of Li, Zhihao
Sent: Wednesday, September 1, 2021 6:38 PM
To: [email protected]<mailto:[email protected]>
Cc: Yao, Jiewen <[email protected]<mailto:[email protected]>>; Wang, Jian
J <[email protected]<mailto:[email protected]>>; Wu, Hao A
<[email protected]<mailto:[email protected]>>; Lu, XiaoyuX
<[email protected]<mailto:[email protected]>>; Jiang, Guomin
<[email protected]<mailto:[email protected]>>;
[email protected]<mailto:[email protected]>; Fu, Siyuan
<[email protected]<mailto:[email protected]>>; Wu, Yidong
<[email protected]<mailto:[email protected]>>; Li, Aaron
<[email protected]<mailto:[email protected]>>
Subject: [edk2-devel] [RFC] Add parallel hash feature into
CryptoPkg.BaseCryptLib
Hi, everyone.
We want to add new hash algorithm—cSHAKE256/ParallelHash256 defined by NIST SP
800-185—into BaseCryptLib of CryptoPkg. This feature can be applied for digital
authentication functions like Capsule Update. It utilizes multi-processor to
calculate the image digest in parallel for update capsule authentication so
that lessen the time of capsule authentication.
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3596
[Background]
The intention of this change is to improve the capsule authentication
performance.
Currently, the image is calculated to a hash value (usually by SHA-256), then
the hash value be signed by a certificate. The header, certificate, and image
binary be sealed to the capsule. In authentication phase, the program should
calculate the hash using image binary in capsule and then perform
authentication procedures.
[Proposal]
Now, we propose a new authentication flow, which firstly pre-calculates the
ParallelHash256 digest of the image binary in parallel with multi-processors,
then use the ParallelHash256 digest (instead of original image binary) in
subsequent SHA-256 hash for sign/authentication.
Since the big size image be compressed to the ParallelHash256 digest that only
have 256 bytes, the time of SHA-256 running would be less.
[Required Changes]
Mainly in CryptoPkg, MdeModulePkg, SecurityPkg:
1. CryptoPkg: need to add the new hash algorithm named
cSHAKE256/ParallelHash256 in BaseCrypLib. The ParallelHash function will
consume CPU MP Service Protocol, not sure if this is allowed in BaseCryptLib?
2. MdeMoudulePkg: Add new authenticate function
AuthenticateFmpImageWithParallelhash() to FmpAuthenticationLib. This is because
original AuthenticateFmpImage() interface only have 4 parameters while the new
have 5 parameters. The 5th parameter is ParallelHash256 digest raised above. We
try to do the parallel hash before authentication and transfer the result to
AuthenticateFmpImage function as parameter. So that we can do only once
parallel hash externally in the case of multiple authentication which saves
more time.
3. SecurityPkg: Add new function named
FmpAuthenticatedHandlerPkcs7WithParallelhash() and
AuthenticateFmpImageWithParallelhash() to FmpAuthenticationLibPkcs7. This is
because original interfaces not have the formal parameter (ParallelHash256
digest) we need. We try to do the parallel hash before authentication and
transfer the result to AuthenticateFmpImage and FmpAuthenticatedHandlerPkcs7
function as parameter. So that we can do only once parallel hash externally in
the case of multiple authentication which saves more time.
Please let me know if you have any comment or concern on this proposed change.
Thanks for your time and feedback!
Best regards,
Zhihao
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80675): https://edk2.groups.io/g/devel/message/80675
Mute This Topic: https://groups.io/mt/85340891/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
