On 06/25/20 21:25, Sukerkar, Amol N wrote:
Please see my comments below.
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: firstname.lastname@example.org; 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
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.
Well as far as I'm concerned it's a simple rule. If a structure is
serialized from RAM into a disk file, or to the network, then the
structure should be packed. If it's only exchanged in RAM, on a single
computer, between modules that all conform to the UEFI spec (which
dictates natural alignment for structure members), then fields in the
structure should not be packed.
- 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?
The struct hack is not desirable because a snippet like
Verification = ...;
Value = Verification->CertData;
is undefined behavior. The compiler knows that CertData has 1 element,
IOW that subscript 1 is out of bounds for the array. Compilers get ever
more aggressive in exploiting undefined behavior, so we should not
introduce more of it.
Simply remove the CertData field from the structure, and replace it with
a comment that states (in natural language) more or less the same. For
an example, refer to the "Description", "FilePathList" and
"OptionalData" fields in EFI_LOAD_OPTION, in file
"MdePkg/Include/Uefi/UefiSpec.h". (Or, in the matching part of the UEFI
2.8 spec itself -- "3.1.3 Load Options".)
- 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.
That makes sense, but a comment should document it.
- "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)
[ANS] Could you suggest how we can make it compliant here?
My point was *not* whether the EFI_ prefix should be used, or should not
be used, for the structures introduced for this feature.
Instead, my point was that either *all* of the new structures should use
_EFI, or *none* should.
- SUB_REGION_HEADER: no EFI_ prefix
- SUB_REGION_VERIFICATION: no EFI_ prefix
- EFI_SUB_REGION_AUTHENTICATION: yes EFI_ prefix
- SIGNED_SUB_REGION: no EFI_ prefix
My suggestion is to either prepend EFI_ to SUB_REGION_HEADER,
SUB_REGION_VERIFICATION, and SIGNED_SUB_REGION, *or* to strip the prefix
from EFI_SUB_REGION_AUTHENTICATION. Whichever you choose is up to
whether this feature is going to be standardized. But, either way, the
prefix usage should be consistent.
- 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?
I have three concerns with this:
(1) The expression you provided does not take into account any possible
padding between the Hdr field and the CertData field. (See also my note
on packing.) So minimally, we should say
Verification.Hdr.Length = (OFFSET_OF (
(2) Are we sure a UINT16 length field can cover all CertData fields
encountered in practice? Will there ever be a CertData that's larger
(3) But, my question actually relates to the size of the Blob field.
SIGNED_SUB_REGION expresses the base address of the region, but not the
size thereof. Is that intentional?
- 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.
- 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 think the general idea behind the current prototype is fine -- take an
input (base, size) pair, and adjust both components of that pair on
output. That's OK, in my opinion.
It's just that the caller needs to know that the output pointer points
into the same original blob (in other words, that on success,
(*SubRegionDataBuffer) *aliases* SignedSubRegionImage).
This is relevant to avoid memory management errors (double freeing of
buffers, or memory leaks). Every function ever that outputs a pointer,
needs to explain in a comment to the caller, who is responsible for
freeing the pointed-to object.