Re: SubRegionAuthLib RFC


Sukerkar, Amol N
 

Hi Laszlo,

Please see my comments below.

Thanks,
Amol

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC

Hi,

On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.
I don't have a stake in this feature, but because the presentation isn't long, I've briefly skimmed it:

- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.

- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?

- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.

- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.

- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?

- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.

- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?


- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.
[ANS] Agree. Needs to change.

- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.

- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.


I have no higher-level comments (i.e. on the intended use case).

Thanks
Laszlo


From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J
<jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar,
Amol N <amol.n.sukerkar@...>; Agrawal, Sachin
<sachin.agrawal@...>
Subject: SubRegionAuthLib RFC

Hi,

I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.

A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.

To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.

Please provide feedback and comments on the design.

Best regards,
Curtis Mackay



Join rfc@edk2.groups.io to automatically receive all group messages.