[U-Boot] [PATCH 01/16] include: pe.h: add signature-related definitions

[Heinrich Schuchardt]

On 11/18/19 6:44 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Sat, Nov 16, 2019 at 06:42:11PM +0100, Heinrich Schuchardt wrote:
>> On 11/13/19 1:52 AM, AKASHI Takahiro wrote:
>>> The index (IMAGE_DIRECTORY_ENTRY_CERTTABLE) in a table points to
>>> a region containing authentication information (image's signature)
>>> in PE format.
>>>
>>> WIN_CERTIFICATE structure defines an embedded signature format.
>>>
>>> Those definitions will be used in my UEFI secure boot patch.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>   include/pe.h | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/include/pe.h b/include/pe.h
>>> index bff3b0aa7a6c..650478ca78af 100644
>>> --- a/include/pe.h
>>> +++ b/include/pe.h
>>> @@ -155,6 +155,7 @@ typedef struct _IMAGE_SECTION_HEADER {
>>>   	uint32_t Characteristics;
>>>   } IMAGE_SECTION_HEADER, *PIMAGE_SECTION_HEADER;
>>>
>>
>> Please, add a comment here:
>>
>> /* Indices for Optional Header Data Directories */
>
> Okay.
>
>>> +#define IMAGE_DIRECTORY_ENTRY_CERTTABLE         4
>>
>> Yes, 4 is the index of the certificate table. But EDK II calls this
>> EFI_IMAGE_DIRECTORY_ENTRY_SECURITY. Should we use the same name to avoid
>> confusion?
>
> No. Because the naming is consistent with
>
>>>   #define IMAGE_DIRECTORY_ENTRY_BASERELOC         5
>
> this existing one.

IMAGE_DIRECTORY_ENTRY_BASERELOC is a constant name used by EDK2 in
MdePkg/Include/IndustryStandard/PeImage.h. Why do you want to deviate
for EFI_IMAGE_DIRECTORY_ENTRY_SECURITY?

>
>>>
>>>   typedef struct _IMAGE_BASE_RELOCATION
>>> @@ -252,4 +253,19 @@ typedef struct _IMAGE_RELOCATION
>>>   #define IMAGE_REL_AMD64_PAIR            0x000F
>>>   #define IMAGE_REL_AMD64_SSPAN32         0x0010
>>>
>>> +/* certificate appended to PE image */
>>> +typedef struct _WIN_CERTIFICATE {
>>
>> We do not capitalize structs. Please use 'win_certificate' (like Linux
>> does).
>>
>>> +	uint32_t dwLength;
>>> +	uint16_t wRevision;
>>> +	uint16_t wCertificateType;
>>> +	uint8_t bCertificate[];
>>
>> We do not use camelcase and type prefixes. Please, use the same
>> component names as Linux (plus 'certificate' for your extra field).
>
> nak.
> Have you looked at 'pe.h' at all?
> There are already bunch of use of camelcase's and capital names
> since this file was first introduced by Alex.

This shouldn't have happened in the first place.

Best regards

Heinrich

>
>>> +} WIN_CERTIFICATE, *LPWIN_CERTIFICATE;
>>
>> Please, always make use of scripts/checkpatch.pl before submitting
>> patches. It says 'WARNING: do not add new typedefs'.
>
> ditto.
> I have run scripts/cehckpatch.pl and dare to leave them unchanged.
>
>> Please, avoid the typedefs.
>
> No. this is also consistent with other typedef's.
>
>
>>> +
>>
>> The following defines are verbatim copies from Linux. Please, also copy
>> the comment:
>>
>> /* Definitions for the contents of the certs data block */
>
> Okay.
>
>>> +#define WIN_CERT_TYPE_PKCS_SIGNED_DATA	0x0002
>>> +#define WIN_CERT_TYPE_EFI_OKCS115	0x0EF0
>>> +#define WIN_CERT_TYPE_EFI_GUID		0x0EF1
>>> +
>>> +#define WIN_CERT_REVISION_1_0		0x0100
>>> +#define WIN_CERT_REVISION_2_0		0x0200
>>> +
>>>   #endif /* _PE_H */
>>>
>>
>> Except for the style issues this looks ok.
>
> Thank you for your comments.
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich
>>
>