From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.128.65, mailfrom: philmd@redhat.com) Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by groups.io with SMTP; Mon, 29 Apr 2019 05:30:14 -0700 Received: by mail-wm1-f65.google.com with SMTP id y197so15526262wmd.0 for ; Mon, 29 Apr 2019 05:30:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=JZfrLSomGgDhEg+V70FYvGqC42hcpmWfBgugGoDOU8Q=; b=W+R78BArcchoUtOHFke8vIlXOhEQfJxxsFz0/5NzieCiebTHSYeNaYcUyEq9QOWh4i kdEvVdPgPwnk4KyDsKVPoFt4WezjCvB/QmaM/E88Z0QABrxETIqy0x2Vik8AwNHNSm4s zpD7/WGGEIHchRM1VflaOW/0ST0TRc0Ih2lMcuwa6fPfGEFcLALIAKfGkL82z2uOBwSj G0MXhY+XVrvu2x4nh4OXPo3coZxA8lTe3OQXgrnzQWgN8tv7DgLQO4b4AKP0MJECPZf3 4ujR0rud4EObGEgojrZN0EXyIOOimn85iLq7e7YGE73dmyW4OvnzjTRKPj+fS+NTJtyw JGNw== X-Gm-Message-State: APjAAAXtHrtU8UCSNKfmODrMQFSsZEQVV/YCMaWoVbUvjNz+2GHZOLZ8 5oCr99wJHBTEpnmcD40bwyh9uX6eGns= X-Google-Smtp-Source: APXvYqxRE2AjzYvSAJ1r3q3tQMnhS4chaJJvy48kX5zwV8G3zGBhaTGFVNQ5p9kGUWHjF3CVOCMRsg== X-Received: by 2002:a1c:f910:: with SMTP id x16mr17132047wmh.114.1556541012853; Mon, 29 Apr 2019 05:30:12 -0700 (PDT) Return-Path: Received: from [192.168.1.33] (193.red-88-21-103.staticip.rima-tde.net. [88.21.103.193]) by smtp.gmail.com with ESMTPSA id o8sm10842832wra.4.2019.04.29.05.30.11 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 29 Apr 2019 05:30:12 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 09/16] OvmfPkg/EnrollDefaultKeys: extract typedefs to a header file To: devel@edk2.groups.io, lersek@redhat.com Cc: Anthony Perard , Ard Biesheuvel , Jordan Justen , Julien Grall References: <20190427005328.27005-1-lersek@redhat.com> <20190427005328.27005-10-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: Date: Mon, 29 Apr 2019 14:30:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190427005328.27005-10-lersek@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Julien Grall > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1747 > Signed-off-by: Laszlo Ersek > --- > 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 > + > +// > +// 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 // EFI_IMAGE_SECURITY_DATABASE > #include // CopyGuid() > #include // ASSERT() > #include // FreePool() > #include // ShellAppMain() > #include // AsciiPrint() > #include // 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