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

Heinrich Schuchardt
2019-11-16T17:42:11Z

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