Re: SubRegionAuthLib RFC
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
- 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.
- 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).
From: Mackay, Curtis A