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.

- 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).

- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to
be NUL-terminated, or not.

- "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.

- 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)

- 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".

- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?


- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type
UINTN, but (*SubRegionDataSize) has type UINT32. I think these types
should be the same.

- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)

- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a
pointer into the input buffer? Is it a copy, allocated dynamically?


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