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

[AKASHI Takahiro]

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.

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

> >+} 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
>