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

[Heinrich Schuchardt]

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 */

> +#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?

>   #define IMAGE_DIRECTORY_ENTRY_BASERELOC         5
>
>   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).

> +} WIN_CERTIFICATE, *LPWIN_CERTIFICATE;

Please, always make use of scripts/checkpatch.pl before submitting
patches. It says 'WARNING: do not add new typedefs'.

Please, avoid the typedefs.

> +

The following defines are verbatim copies from Linux. Please, also copy
the comment:

/* Definitions for the contents of the certs data block */

> +#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.

Best regards

Heinrich