From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Anthony Perard <anthony.perard@citrix.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Jordan Justen <jordan.l.justen@intel.com>,
Julien Grall <julien.grall@arm.com>
Subject: Re: [edk2-devel] [PATCH 09/16] OvmfPkg/EnrollDefaultKeys: extract typedefs to a header file
Date: Mon, 29 Apr 2019 14:30:11 +0200 [thread overview]
Message-ID: <e5484eec-1d7f-086e-7533-5d35618f2ad1@redhat.com> (raw)
In-Reply-To: <20190427005328.27005-10-lersek@redhat.com>
On 4/27/19 2:53 AM, Laszlo Ersek wrote:
> "EnrollDefaultKeys.c" defines three structure types: SINGLE_HEADER,
> REPEATING_HEADER, and SETTINGS. The definitions are scattered over the C
> file, and lack high-level summary comments.
>
> Extract the structures to "EnrollDefaultKeys.h", and add the missing
> comments.
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1747
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf | 1 +
> OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.h | 121 ++++++++++++++++++++
> OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c | 101 +---------------
> 3 files changed, 124 insertions(+), 99 deletions(-)
>
> diff --git a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> index 3a215df50863..9f315a8e6d90 100644
> --- a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> @@ -11,16 +11,17 @@ [Defines]
> BASE_NAME = EnrollDefaultKeys
> FILE_GUID = A0BAA8A3-041D-48A8-BC87-C36D121B5E3D
> MODULE_TYPE = UEFI_APPLICATION
> VERSION_STRING = 0.1
> ENTRY_POINT = ShellCEntryLib
>
> [Sources]
> EnrollDefaultKeys.c
> + EnrollDefaultKeys.h
>
> [Packages]
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> SecurityPkg/SecurityPkg.dec
> ShellPkg/ShellPkg.dec
>
> [Guids]
> diff --git a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.h b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.h
> new file mode 100644
> index 000000000000..9bcd87ff4f44
> --- /dev/null
> +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.h
> @@ -0,0 +1,121 @@
> +/** @file
> + Type definitions for the EnrollDefaultKeys application.
> +
> + Copyright (C) 2014-2019, Red Hat, Inc.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef ENROLL_DEFAULT_KEYS_H_
> +#define ENROLL_DEFAULT_KEYS_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +//
> +// Convenience structure types for constructing "signature lists" for
> +// authenticated UEFI variables.
> +//
> +// The most important thing about the variable payload is that it is a list of
> +// lists, where the element size of any given *inner* list is constant.
> +//
> +// Since X509 certificates vary in size, each of our *inner* lists will contain
> +// one element only (one X.509 certificate). This is explicitly mentioned in
> +// the UEFI specification, in "28.4.1 Signature Database", in a Note.
> +//
> +// The list structure looks as follows:
> +//
> +// struct EFI_VARIABLE_AUTHENTICATION_2 { |
> +// struct EFI_TIME { |
> +// UINT16 Year; |
> +// UINT8 Month; |
> +// UINT8 Day; |
> +// UINT8 Hour; |
> +// UINT8 Minute; |
> +// UINT8 Second; |
> +// UINT8 Pad1; |
> +// UINT32 Nanosecond; |
> +// INT16 TimeZone; |
> +// UINT8 Daylight; |
> +// UINT8 Pad2; |
> +// } TimeStamp; |
> +// |
> +// struct WIN_CERTIFICATE_UEFI_GUID { | |
> +// struct WIN_CERTIFICATE { | |
> +// UINT32 dwLength; ----------------------------------------+ |
> +// UINT16 wRevision; | |
> +// UINT16 wCertificateType; | |
> +// } Hdr; | +- DataSize
> +// | |
> +// EFI_GUID CertType; | |
> +// UINT8 CertData[1] = { <--- "struct hack" | |
> +// struct EFI_SIGNATURE_LIST { | | |
> +// EFI_GUID SignatureType; | | |
> +// UINT32 SignatureListSize; -------------------------+ | |
> +// UINT32 SignatureHeaderSize; | | |
> +// UINT32 SignatureSize; ---------------------------+ | | |
> +// UINT8 SignatureHeader[SignatureHeaderSize]; | | | |
> +// v | | |
> +// struct EFI_SIGNATURE_DATA { | | | |
> +// EFI_GUID SignatureOwner; | | | |
> +// UINT8 SignatureData[1] = { <--- "struct hack" | | | |
> +// X.509 payload | | | |
> +// } | | | |
> +// } Signatures[]; | | |
> +// } SigLists[]; | |
> +// }; | |
> +// } AuthInfo; | |
> +// }; |
> +//
> +// Given that the "struct hack" invokes undefined behavior (which is why C99
> +// introduced the flexible array member), and because subtracting those pesky
> +// sizes of 1 is annoying, and because the format is fully specified in the
> +// UEFI specification, we'll introduce two matching convenience structures that
> +// are customized for our X.509 purposes.
> +//
> +#pragma pack (1)
> +typedef struct {
> + EFI_TIME TimeStamp;
> +
> + //
> + // dwLength covers data below
> + //
> + UINT32 dwLength;
> + UINT16 wRevision;
> + UINT16 wCertificateType;
> + EFI_GUID CertType;
> +} SINGLE_HEADER;
> +
> +typedef struct {
> + //
> + // SignatureListSize covers data below
> + //
> + EFI_GUID SignatureType;
> + UINT32 SignatureListSize;
> + UINT32 SignatureHeaderSize; // constant 0
> + UINT32 SignatureSize;
> +
> + //
> + // SignatureSize covers data below
> + //
> + EFI_GUID SignatureOwner;
> +
> + //
> + // X.509 certificate follows
> + //
> +} REPEATING_HEADER;
> +#pragma pack ()
> +
> +
> +//
> +// A structure that collects the values of UEFI variables related to Secure
> +// Boot.
> +//
> +typedef struct {
> + UINT8 SetupMode;
> + UINT8 SecureBoot;
> + UINT8 SecureBootEnable;
> + UINT8 CustomMode;
> + UINT8 VendorKeys;
> +} SETTINGS;
> +
> +#endif /* ENROLL_DEFAULT_KEYS_H_ */
> diff --git a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> index 671efef8d6ad..fefea6638887 100644
> --- a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
> @@ -10,16 +10,18 @@
> #include <Guid/ImageAuthentication.h> // EFI_IMAGE_SECURITY_DATABASE
> #include <Library/BaseMemoryLib.h> // CopyGuid()
> #include <Library/DebugLib.h> // ASSERT()
> #include <Library/MemoryAllocationLib.h> // FreePool()
> #include <Library/ShellCEntryLib.h> // ShellAppMain()
> #include <Library/UefiLib.h> // AsciiPrint()
> #include <Library/UefiRuntimeServicesTableLib.h> // gRT
>
> +#include "EnrollDefaultKeys.h"
> +
> //
> // We'll use the certificate below as both Platform Key and as first Key
> // Exchange Key.
> //
> // "Red Hat Secure Boot (PK/KEK key 1)/emailAddress=secalert@redhat.com"
> // SHA1: fd:fc:7f:3c:7e:f3:e0:57:76:ad:d7:98:78:21:6c:9b:e0:e1:95:97
> //
> STATIC CONST UINT8 mRedHatPkKek1[] = {
> @@ -538,107 +540,16 @@ STATIC CONST UINT8 mSha256OfDevNull[] = {
> // EFI_SIGNATURE_DATA.SignatureData, and not the organization that issued
> // EFI_SIGNATURE_DATA.SignatureData.
> //
> STATIC CONST EFI_GUID mMicrosoftOwnerGuid = {
> 0x77fa9abd, 0x0359, 0x4d32,
> { 0xbd, 0x60, 0x28, 0xf4, 0xe7, 0x8f, 0x78, 0x4b },
> };
>
> -//
> -// The most important thing about the variable payload is that it is a list of
> -// lists, where the element size of any given *inner* list is constant.
> -//
> -// Since X509 certificates vary in size, each of our *inner* lists will contain
> -// one element only (one X.509 certificate). This is explicitly mentioned in
> -// the UEFI specification, in "28.4.1 Signature Database", in a Note.
> -//
> -// The list structure looks as follows:
> -//
> -// struct EFI_VARIABLE_AUTHENTICATION_2 { |
> -// struct EFI_TIME { |
> -// UINT16 Year; |
> -// UINT8 Month; |
> -// UINT8 Day; |
> -// UINT8 Hour; |
> -// UINT8 Minute; |
> -// UINT8 Second; |
> -// UINT8 Pad1; |
> -// UINT32 Nanosecond; |
> -// INT16 TimeZone; |
> -// UINT8 Daylight; |
> -// UINT8 Pad2; |
> -// } TimeStamp; |
> -// |
> -// struct WIN_CERTIFICATE_UEFI_GUID { | |
> -// struct WIN_CERTIFICATE { | |
> -// UINT32 dwLength; ----------------------------------------+ |
> -// UINT16 wRevision; | |
> -// UINT16 wCertificateType; | |
> -// } Hdr; | +- DataSize
> -// | |
> -// EFI_GUID CertType; | |
> -// UINT8 CertData[1] = { <--- "struct hack" | |
> -// struct EFI_SIGNATURE_LIST { | | |
> -// EFI_GUID SignatureType; | | |
> -// UINT32 SignatureListSize; -------------------------+ | |
> -// UINT32 SignatureHeaderSize; | | |
> -// UINT32 SignatureSize; ---------------------------+ | | |
> -// UINT8 SignatureHeader[SignatureHeaderSize]; | | | |
> -// v | | |
> -// struct EFI_SIGNATURE_DATA { | | | |
> -// EFI_GUID SignatureOwner; | | | |
> -// UINT8 SignatureData[1] = { <--- "struct hack" | | | |
> -// X.509 payload | | | |
> -// } | | | |
> -// } Signatures[]; | | |
> -// } SigLists[]; | |
> -// }; | |
> -// } AuthInfo; | |
> -// }; |
> -//
> -// Given that the "struct hack" invokes undefined behavior (which is why C99
> -// introduced the flexible array member), and because subtracting those pesky
> -// sizes of 1 is annoying, and because the format is fully specified in the
> -// UEFI specification, we'll introduce two matching convenience structures that
> -// are customized for our X.509 purposes.
> -//
> -#pragma pack (1)
> -typedef struct {
> - EFI_TIME TimeStamp;
> -
> - //
> - // dwLength covers data below
> - //
> - UINT32 dwLength;
> - UINT16 wRevision;
> - UINT16 wCertificateType;
> - EFI_GUID CertType;
> -} SINGLE_HEADER;
> -
> -typedef struct {
> - //
> - // SignatureListSize covers data below
> - //
> - EFI_GUID SignatureType;
> - UINT32 SignatureListSize;
> - UINT32 SignatureHeaderSize; // constant 0
> - UINT32 SignatureSize;
> -
> - //
> - // SignatureSize covers data below
> - //
> - EFI_GUID SignatureOwner;
> -
> - //
> - // X.509 certificate follows
> - //
> -} REPEATING_HEADER;
> -#pragma pack ()
> -
> /**
> Enroll a set of certificates in a global variable, overwriting it.
>
> The variable will be rewritten with NV+BS+RT+AT attributes.
>
> @param[in] VariableName The name of the variable to overwrite.
>
> @param[in] VendorGuid The namespace (ie. vendor GUID) of the variable to
> @@ -839,24 +750,16 @@ GetExact (
> AsciiPrint ("error: GetVariable(\"%s\", %g): expected size 0x%Lx, "
> "got 0x%Lx\n", VariableName, VendorGuid, (UINT64)DataSize, (UINT64)Size);
> return EFI_PROTOCOL_ERROR;
> }
>
> return EFI_SUCCESS;
> }
>
> -typedef struct {
> - UINT8 SetupMode;
> - UINT8 SecureBoot;
> - UINT8 SecureBootEnable;
> - UINT8 CustomMode;
> - UINT8 VendorKeys;
> -} SETTINGS;
> -
> STATIC
> EFI_STATUS
> GetSettings (
> OUT SETTINGS *Settings
> )
> {
> EFI_STATUS Status;
>
>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
next prev parent reply other threads:[~2019-04-29 12:30 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-27 0:53 [PATCH 00/16] OvmfPkg, ArmVirtPkg: upstream the EnrollDefaultKeys app Laszlo Ersek
2019-04-27 0:53 ` [PATCH 01/16] OvmfPkg: introduce EnrollDefaultKeys application Laszlo Ersek
2019-04-30 5:21 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 02/16] OvmfPkg/EnrollDefaultKeys: update @file comment blocks Laszlo Ersek
2019-04-30 5:13 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 03/16] OvmfPkg/EnrollDefaultKeys: refresh INF file Laszlo Ersek
2019-04-29 12:25 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 04/16] ArmVirtPkg: build EnrollDefaultKeys.efi Laszlo Ersek
2019-04-29 12:26 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 05/16] OvmfPkg/EnrollDefaultKeys: clean up minor whitespace wart Laszlo Ersek
2019-04-29 12:26 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 06/16] OvmfPkg/EnrollDefaultKeys: clean up global variable name prefixes Laszlo Ersek
2019-04-29 12:27 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 07/16] OvmfPkg/EnrollDefaultKeys: clean up acronym capitalization in identifiers Laszlo Ersek
2019-04-30 5:10 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 08/16] OvmfPkg/EnrollDefaultKeys: remove unneeded EFIAPI call. conv. specifiers Laszlo Ersek
2019-04-29 12:28 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 09/16] OvmfPkg/EnrollDefaultKeys: extract typedefs to a header file Laszlo Ersek
2019-04-29 12:30 ` Philippe Mathieu-Daudé [this message]
2019-04-27 0:53 ` [PATCH 10/16] OvmfPkg/EnrollDefaultKeys: split out certificate and signature constants Laszlo Ersek
2019-04-29 12:33 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 11/16] OvmfPkg/EnrollDefaultKeys: extract MICROSOFT_VENDOR_GUID Laszlo Ersek
2019-04-30 5:11 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 12/16] OvmfPkg/EnrollDefaultKeys: describe functions with leading comment blocks Laszlo Ersek
2019-04-30 5:12 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 13/16] OvmfPkg/EnrollDefaultKeys: document the steps of the entry point function Laszlo Ersek
2019-04-29 12:36 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 14/16] OvmfPkg: introduce OVMF_PK_KEK1_APP_PREFIX_GUID Laszlo Ersek
2019-04-30 5:24 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 15/16] OvmfPkg/EnrollDefaultKeys: enroll PK/KEK1 from the Type 11 SMBIOS table Laszlo Ersek
2019-04-30 5:34 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 0:53 ` [PATCH 16/16] OvmfPkg/EnrollDefaultKeys: remove Red Hat's hard-coded PK/KEK1 Laszlo Ersek
2019-04-30 5:35 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-27 1:07 ` [edk2-devel] [PATCH 00/16] OvmfPkg, ArmVirtPkg: upstream the EnrollDefaultKeys app Laszlo Ersek
2019-04-27 8:14 ` Ard Biesheuvel
2019-04-30 7:51 ` [edk2-devel] " Gary Lin
2019-04-30 12:32 ` Laszlo Ersek
2019-04-30 19:42 ` Ard Biesheuvel
2019-04-30 20:04 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e5484eec-1d7f-086e-7533-5d35618f2ad1@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox