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

[AKASHI Takahiro]

On Mon, Nov 18, 2019 at 07:26:55AM +0100, Heinrich Schuchardt wrote:
> 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?

Okay, but not due to EDK2, but because Windows uses it:
https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-imagedirectoryentrytodata


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

For consistency, we should keep the style in the *existing* files.

-Takahiro Akashi


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