public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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>

  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