On Mon, Jul 14, 2025 at 11:05:07PM +0530, Sudhakar Kuppusamy wrote: > Enhancing the infrastructure to enable the Platform Keystore (PKS) feature, > which provides access to the SB VERSION, DB, and DBX secure boot variables > from PKS. > > If secure boot is enabled with PKS, it will read secure boot variables > such as db and dbx from PKS and extract ESL's from it. > The ESL's would be saved in the platform keystore buffer, and > the appendedsig (module) would read it later to extract > the certificate's details from ESL. > > In the following scenarios, static key mode will be activated: > 1. When Secure Boot is enabled with static keys > 2. When SB Version is unavailable but Secure Boot is enabled > 3. When PKS support is unavailable but Secure Boot is enabled > > Note:- > > SB Version - Secure Boot mode > 1 - PKS > 0 - static key (embeded key)
I think "static key" mode should be explained in the commit message too. Additionally, I think it is worth explaining key differences between two modes. I do not mention these should be put into GRUB documentation... > Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com> > --- > grub-core/Makefile.am | 2 + > grub-core/Makefile.core.def | 2 + > grub-core/kern/ieee1275/ieee1275.c | 1 - > grub-core/kern/ieee1275/init.c | 18 +- > grub-core/kern/powerpc/ieee1275/ieee1275.c | 139 +++++++ > .../kern/powerpc/ieee1275/platform_keystore.c | 343 ++++++++++++++++++ > include/grub/ieee1275/ieee1275.h | 3 + > include/grub/powerpc/ieee1275/ieee1275.h | 21 ++ > .../grub/powerpc/ieee1275/platform_keystore.h | 126 +++++++ > 9 files changed, 651 insertions(+), 4 deletions(-) > create mode 100644 grub-core/kern/powerpc/ieee1275/ieee1275.c > create mode 100644 grub-core/kern/powerpc/ieee1275/platform_keystore.c > create mode 100644 include/grub/powerpc/ieee1275/platform_keystore.h [...] > diff --git a/grub-core/kern/powerpc/ieee1275/platform_keystore.c > b/grub-core/kern/powerpc/ieee1275/platform_keystore.c > new file mode 100644 > index 000000000..3af1ea28a > --- /dev/null > +++ b/grub-core/kern/powerpc/ieee1275/platform_keystore.c > @@ -0,0 +1,343 @@ > +/* > + * GRUB -- GRand Unified Bootloader > + * Copyright (C) 2024 Free Software Foundation, Inc. > + * Copyright (C) 2022, 2023, 2024, 2025 IBM Corporation > + * > + * GRUB is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * GRUB is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <grub/mm.h> > +#include <grub/types.h> > +#include <grub/misc.h> > +#include <grub/lockdown.h> > +#include <grub/ieee1275/ieee1275.h> > +#include <grub/powerpc/ieee1275/ieee1275.h> > +#include <grub/powerpc/ieee1275/platform_keystore.h> > + > +/* PKS consumer type for firmware. */ > +#define PKS_CONSUMER_FW ((grub_uint8_t) 1) > + > +/* PKS read object lable for secure boot version. */ > +#define SB_VERSION_KEY_NAME ((grub_uint8_t *) "SB_VERSION") > +#define SB_VERSION_KEY_LEN ((grub_size_t) 10) > + > +/* PKS read secure boot variable request type for db and dbx. */ > +#define DB ((grub_uint8_t) 1) > +#define DBX ((grub_uint8_t) 2) > + > +static grub_size_t pks_max_object_size; > +grub_uint8_t grub_pks_use_keystore = 0; > + > +/* Platform Keystore. */ > +grub_pks_t grub_pks_keystore = { .db = NULL, .dbx = NULL, .db_entries = 0, > .dbx_entries = 0 }; > + > +/* Convert the esl data into the ESL. */ > +static grub_esl_t * > +convert_to_esl (const grub_uint8_t *esl_data, const grub_size_t > esl_data_size) This does not convert anything. Just validate sizes and change data types. And this function is called once. So, please put its code where it belongs.... > +{ > + grub_esl_t *esl = NULL; > + > + if (esl_data_size < sizeof (grub_esl_t) || esl_data == NULL) > + return esl; > + > + esl = (grub_esl_t *) esl_data; > + > + return esl; > +} > + > +/* > + * Import the GUID, esd, and its size into the pks sd buffer and > + * pks sd entries from the EFI signature list. > + */ > +static grub_err_t > +esd_from_esl (const grub_uint8_t *esl_data, grub_size_t esl_size, > + const grub_size_t signature_size, const grub_packed_guid_t > *guid, > + grub_pks_sd_t **pks_sd, grub_size_t *pks_sd_entries) > +{ > + grub_esd_t *esd; > + grub_pks_sd_t *signature = *pks_sd; > + grub_size_t entries = *pks_sd_entries; > + grub_size_t data_size, offset = 0; > + > + /* Reads the esd from esl. */ > + while (esl_size > 0) > + { > + esd = (grub_esd_t *) (esl_data + offset); > + data_size = signature_size - sizeof (grub_esd_t); > + > + signature = grub_realloc (signature, (entries + 1) * sizeof > (grub_pks_sd_t)); > + if (signature == NULL) > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory."); > + > + signature[entries].data = grub_malloc (data_size * sizeof > (grub_uint8_t)); > + if (signature[entries].data == NULL) > + { > + /* > + * Allocated memory will be freed by > + * grub_free_platform_keystore. > + */ > + *pks_sd = signature; > + *pks_sd_entries = entries + 1; > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory."); > + } > + > + grub_memcpy (signature[entries].data, esd->signature_data, data_size); > + signature[entries].data_size = data_size; > + signature[entries].guid = *guid; > + entries++; > + esl_size -= signature_size; > + offset += signature_size; > + } > + > + *pks_sd = signature; > + *pks_sd_entries = entries; > + > + return GRUB_ERR_NONE; > +} > + > +/* Extract the esd after removing the esl header from esl. */ > +static grub_err_t > +esl_to_esd (const grub_uint8_t *esl_data, grub_size_t *next_esl, > + grub_pks_sd_t **pks_sd, grub_size_t *pks_sd_entries) > +{ > + grub_packed_guid_t guid; > + grub_esl_t *esl; > + grub_size_t offset, esl_size, > + signature_size, signature_header_size; > + > + esl = convert_to_esl (esl_data, *next_esl); > + if (esl == NULL) > + return grub_error (GRUB_ERR_BUG, "invalid ESL."); > + > + esl_size = grub_le_to_cpu32 (esl->signature_list_size); > + signature_header_size = grub_le_to_cpu32 (esl->signature_header_size); > + signature_size = grub_le_to_cpu32 (esl->signature_size); > + grub_memcpy (&guid, &esl->signature_type, sizeof (grub_packed_guid_t)); > + > + if (esl_size < sizeof (grub_esl_t) || esl_size > *next_esl) > + return grub_error (GRUB_ERR_BUG, "invalid ESL size (%u)\n", esl_size); > + > + *next_esl = esl_size; > + offset = sizeof (grub_esl_t) + signature_header_size; > + esl_size = esl_size - offset; > + > + return esd_from_esl (esl_data + offset, esl_size, signature_size, &guid, > + pks_sd, pks_sd_entries); > +} > + > +/* > + * Import the EFI signature data and the number of esd from the esl > + * into the pks sd buffer and pks sd entries. Could you be more consistent with using abbreviations, i.e. their case? Most of them should be all upper case. And what are esd/ESD" and "sd"? > + */ > +static grub_err_t > +pks_sd_from_esl (const grub_uint8_t *esl_data, grub_size_t esl_size, > + grub_pks_sd_t **pks_sd, grub_size_t *pks_sd_entries) > +{ > + grub_err_t rc; > + grub_size_t next_esl = esl_size; > + > + do > + { > + rc = esl_to_esd (esl_data, &next_esl, pks_sd, pks_sd_entries); > + if (rc != GRUB_ERR_NONE) > + break; > + > + esl_data += next_esl; > + esl_size -= next_esl; > + next_esl = esl_size; > + } > + while (esl_size > 0); > + > + return rc; > +} > + > +/* > + * Read the secure boot version from PKS as an object. > + * caller must free result. > + */ > +static grub_err_t > +read_sbversion_from_pks (grub_uint8_t **out, grub_size_t *outlen, > grub_size_t *policy) > +{ > + int rc; > + > + *out = grub_malloc (pks_max_object_size); > + if (*out == NULL) > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory."); > + > + rc = grub_ieee1275_pks_read_object (PKS_CONSUMER_FW, SB_VERSION_KEY_NAME, > + SB_VERSION_KEY_LEN, *out, > pks_max_object_size, > + outlen, policy); > + if (rc != 0) > + return grub_error (GRUB_ERR_READ_ERROR, "SB version read failed (%d)\n", > rc); > + > + return GRUB_ERR_NONE; > +} > + > +/* > + * Reads the secure boot variable from PKS. > + * caller must free result. s/caller/Caller/ > + */ > +static grub_err_t > +read_sbvar_from_pks (const grub_uint8_t sbvarflags, const grub_uint8_t > sbvartype, > + grub_uint8_t **out, grub_size_t *outlen) > +{ > + int rc; > + > + *out = grub_malloc (pks_max_object_size); > + if (*out == NULL) > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory."); > + > + rc = grub_ieee1275_pks_read_sbvar (sbvarflags, sbvartype, *out, > pks_max_object_size, outlen); > + if (rc == (int) IEEE1275_CELL_NOT_FOUND) This cast suggests the rc should be defined as different type... > + return grub_error (GRUB_ERR_UNKNOWN_COMMAND, "secure boot variable %s > not found (%d)", > + (sbvartype == DB ? "db" : "dbx"), rc); > + else if (rc != 0) If you look at the code of the grub_ieee1275_pks_read_sbvar() then you will see you return "-1" in case of an error. So, I think you should do "rc < 0" check here... > + return grub_error (GRUB_ERR_READ_ERROR, "secure boot variable %s reading > (%d)", > + (sbvartype == DB ? "db" : "dbx"), rc); > + > + return GRUB_ERR_NONE; > +} > + > +/* Test the availability of PKS support. */ > +static grub_err_t > +is_support_pks (void) > +{ > + int rc; > + grub_ieee1275_cell_t missing = 0; > + > + rc = grub_ieee1275_test ("pks-max-object-size", &missing); > + if (rc != 0 || missing == IEEE1275_CELL_INVALID) Ditto... > + grub_error (GRUB_ERR_BAD_FIRMWARE, "firmware doesn't have PKS > support.\n"); > + else > + { > + rc = grub_ieee1275_pks_max_object_size (&pks_max_object_size); > + if (rc != 0) Ditto... Please fix this problem everywhere... > + grub_error (GRUB_ERR_BAD_NUMBER, "PKS support is there but it has > zero objects.\n"); > + } > + > + return GRUB_ERR_NONE; > +} > + > +/* > + * Retrieve the secure boot variable from PKS, unpacks it, read the esd > + * from ESL, and store the information in the pks sd buffer. Please be consistent with case... > + */ > +static grub_err_t > +read_secure_boot_variables (const grub_uint8_t sbvarflags, const > grub_uint8_t sbvartype, > + grub_pks_sd_t **pks_sd, grub_size_t > *pks_sd_entries) > +{ > + grub_err_t rc; > + grub_uint8_t *esl_data = NULL; > + grub_size_t esl_data_size = 0; > + > + rc = read_sbvar_from_pks (sbvarflags, sbvartype, &esl_data, > &esl_data_size); > + if (rc == GRUB_ERR_NONE && esl_data_size != 0) > + rc = pks_sd_from_esl ((const grub_uint8_t *) esl_data, esl_data_size, > + pks_sd, pks_sd_entries); > + grub_free (esl_data); > + > + return rc; > +} > + > +/* > + * Reads secure boot version (SB_VERSION) and it supports following. > + * SB_VERSION > + * 1 - PKS > + * 0 - static key (embeded key) Same comment as for commit message. We need more info here... > + */ > +static grub_err_t > +get_secure_boot_version (void) > +{ > + grub_err_t rc; > + grub_uint8_t *data = NULL; > + grub_size_t len = 0, policy = 0; > + > + rc = read_sbversion_from_pks (&data, &len, &policy); > + if (rc == GRUB_ERR_NONE && (len != 1 || (*data >= 2))) > + rc = grub_error (GRUB_ERR_BAD_NUMBER, "found unexpected SB version > (%d)\n", *data); > + > + if (rc != GRUB_ERR_NONE) > + { > + grub_printf ("switch to static key.\n"); > + if (grub_is_lockdown () == GRUB_LOCKDOWN_ENABLED) > + grub_fatal ("secure boot locked down."); s/secure boot locked down./Secure boot locked down/ However, first of all I think it should be explained here why the lockdown cannot be enabled here... > + } > + else > + grub_pks_use_keystore = *data; Why do you assign version number to grub_pks_use_keystore which looks like a flag? Probably the name of variable is wrong or its type. > + grub_free (data); > + > + return rc; > +} > + > +/* Free allocated memory. */ > +void > +grub_pks_free_keystore (void) > +{ > + grub_size_t i; > + > + for (i = 0; i < grub_pks_keystore.db_entries; i++) > + grub_free (grub_pks_keystore.db[i].data); > + > + for (i = 0; i < grub_pks_keystore.dbx_entries; i++) > + grub_free (grub_pks_keystore.dbx[i].data); > + > + grub_free (grub_pks_keystore.db); > + grub_free (grub_pks_keystore.dbx); > + grub_memset (&grub_pks_keystore, 0, sizeof (grub_pks_t)); > +} > + > +/* Initialization of the Platform Keystore. */ > +grub_err_t > +grub_pks_keystore_init (void) > +{ > + grub_err_t rc; > + > + grub_dprintf ("ieee1275", "trying to load Platform Keystore.\n"); > + > + rc = is_support_pks (); > + if (rc != GRUB_ERR_NONE) > + { > + grub_printf ("switch to static key.\n"); s/grub_printf/grub_dprintf/? > + return rc; > + } > + > + /* Read SB_VERSION from PKS. */ > + rc = get_secure_boot_version (); > + if (rc != GRUB_ERR_NONE) > + return rc; > + > + if (grub_pks_use_keystore) This looks like a flag. Should not it be defined as bool? > + { > + grub_memset (&grub_pks_keystore, 0, sizeof (grub_pks_t)); > + /* Read DB from PKS. */ > + rc = read_secure_boot_variables (0, DB, &grub_pks_keystore.db, > &grub_pks_keystore.db_entries); > + if (rc == GRUB_ERR_NONE) > + { > + /* Read DBX from PKS. */ > + rc = read_secure_boot_variables (0, DBX, &grub_pks_keystore.dbx, > &grub_pks_keystore.dbx_entries); > + if (rc == GRUB_ERR_UNKNOWN_COMMAND) > + { > + grub_dprintf ("ieee1275", "dbx is not found in PKS.\n"); > + rc = GRUB_ERR_NONE; > + } > + } > + > + } > + > + if (rc != GRUB_ERR_NONE) > + grub_pks_free_keystore (); > + > + return rc; > +} > diff --git a/include/grub/ieee1275/ieee1275.h > b/include/grub/ieee1275/ieee1275.h > index c445d0499..3965c5fc6 100644 > --- a/include/grub/ieee1275/ieee1275.h > +++ b/include/grub/ieee1275/ieee1275.h > @@ -24,6 +24,9 @@ > #include <grub/types.h> > #include <grub/machine/ieee1275.h> > > +#define IEEE1275_CELL_INVALID ((grub_ieee1275_cell_t) -1) > +#define IEEE1275_CELL_NOT_FOUND ((grub_ieee1275_cell_t) -7) > + > #define GRUB_IEEE1275_CELL_FALSE ((grub_ieee1275_cell_t) 0) > #define GRUB_IEEE1275_CELL_TRUE ((grub_ieee1275_cell_t) -1) > > diff --git a/include/grub/powerpc/ieee1275/ieee1275.h > b/include/grub/powerpc/ieee1275/ieee1275.h > index 4eb207018..1772d1f6d 100644 > --- a/include/grub/powerpc/ieee1275/ieee1275.h > +++ b/include/grub/powerpc/ieee1275/ieee1275.h > @@ -28,4 +28,25 @@ typedef grub_uint32_t grub_ieee1275_cell_t; > #define PRIxGRUB_IEEE1275_CELL_T PRIxGRUB_UINT32_T > #define PRIuGRUB_IEEE1275_CELL_T PRIuGRUB_UINT32_T > > +#ifdef __powerpc__ > + > +extern int > +EXPORT_FUNC (grub_ieee1275_test) (const char *name, > + grub_ieee1275_cell_t *missing); > + > +extern int > +grub_ieee1275_pks_max_object_size (grub_size_t *result); > + > +extern int > +grub_ieee1275_pks_read_object (grub_uint8_t consumer, grub_uint8_t *label, > + grub_size_t label_len, grub_uint8_t *buffer, > + grub_size_t buffer_len, grub_size_t *data_len, > + grub_uint32_t *policies); > + > +extern int > +grub_ieee1275_pks_read_sbvar (grub_uint8_t sbvarflags, grub_uint8_t > sbvartype, > + grub_uint8_t *buffer, grub_size_t buffer_len, > + grub_size_t *data_len); It seems to me at least some of these functions should not be exported. If they are used in following patches then they should be exported when a given patch adds calls to these functions. > +#endif > + > #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */ > diff --git a/include/grub/powerpc/ieee1275/platform_keystore.h > b/include/grub/powerpc/ieee1275/platform_keystore.h > new file mode 100644 > index 000000000..d48c60200 > --- /dev/null > +++ b/include/grub/powerpc/ieee1275/platform_keystore.h > @@ -0,0 +1,126 @@ > +/* > + * Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved. This > + * program and the accompanying materials are licensed and made available > + * under the terms and conditions of the 2-Clause BSD License which > + * accompanies this distribution. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > met: > + * > + * 1. Redistributions of source code must retain the above copyright notice, > + * this list of conditions and the following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS > IS" > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > + * POSSIBILITY OF SUCH DAMAGE. > + * > + * https://github.com/tianocore/edk2-staging (edk2-staging repo of > tianocore), > + * the ImageAuthentication.h file under it, and here's the copyright and > license. > + * > + * MdePkg/Include/Guid/ImageAuthentication.h > + * > + * Copyright 2022, 2023, 2024, 2025 IBM Corp. > + */ > + > +#ifndef __PLATFORM_KEYSTORE_H__ > +#define __PLATFORM_KEYSTORE_H__ > + > +#include <grub/symbol.h> > +#include <grub/mm.h> > +#include <grub/types.h> > + > +#if __GNUC__ >= 9 > +#pragma GCC diagnostic ignored "-Waddress-of-packed-member" > +#endif This probably should not be in the header file... > +#define GRUB_MAX_HASH_SIZE 64 > + > +/* Secure Boot Mode. */ > +#define SB_DISABLED 0 > +#define SB_ENFORCED 2 > + > +/* > + * It is derived from EFI_SIGNATURE_DATA > + * > https://github.com/tianocore/edk2-staging/blob/master/MdePkg/Include/Guid/ImageAuthentication.h > + * > + * The structure of an EFI signature database (ESD).*/ Aha... Here it is... So, you should use correct ESD abbreviation everywhere... And missing space after "." here and below... > +struct grub_esd > +{ > + /* > + * An identifier which identifies the agent which added > + * the signature to the list. > + */ > + grub_packed_guid_t signature_owner; > + /* The format of the signature is defined by the SignatureType.*/ > + grub_uint8_t signature_data[]; > +} GRUB_PACKED; > + Please drop this empty line... > +typedef struct grub_esd grub_esd_t; > + > +/* > + * It is derived from EFI_SIGNATURE_LIST > + * > https://github.com/tianocore/edk2-staging/blob/master/MdePkg/Include/Guid/ImageAuthentication.h > + * > + * The structure of an EFI signature list (ESL).*/ > +struct grub_esl > +{ > + /* Type of the signature. GUID signature types are defined in below.*/ > + grub_packed_guid_t signature_type; > + /* Total size of the signature list, including this header.*/ > + grub_uint32_t signature_list_size; > + /* > + * Size of the signature header which precedes > + * the array of signatures. > + */ > + grub_uint32_t signature_header_size; > + /* Size of each signature.*/ > + grub_uint32_t signature_size; > +} GRUB_PACKED; > + Ditto... > +typedef struct grub_esl grub_esl_t; > + > +/* The structure of a PKS signature data.*/ Ha! sd == signature data. So, if you really use sd abbreviation in the comments please define it here, e.g. Signature Data (SD), and use it consistently. > +struct grub_pks_sd > +{ > + grub_packed_guid_t guid; /* Signature type. */ > + grub_uint8_t *data; /* Signature data. */ > + grub_size_t data_size; /* Size of signature data. */ > +} GRUB_PACKED; > + Redundant empty line... > +typedef struct grub_pks_sd grub_pks_sd_t; > + > +/* The structure of a PKS.*/ > +struct grub_pks > +{ > + grub_pks_sd_t *db; /* Signature database. */ > + grub_pks_sd_t *dbx; /* Forbidden signature database. */ > + grub_size_t db_entries; /* Size of signature database. */ > + grub_size_t dbx_entries; /* Size of forbidden signature database. */ > +} GRUB_PACKED; > + Ditto... > +typedef struct grub_pks grub_pks_t; > + > +/* Initialization of the Platform Keystore. */ > +extern grub_err_t > +grub_pks_keystore_init (void); > + > +/* Free allocated memory. */ > +extern void > +EXPORT_FUNC (grub_pks_free_keystore) (void); > + > +extern grub_uint8_t EXPORT_VAR (grub_pks_use_keystore); > +extern grub_pks_t EXPORT_VAR (grub_pks_keystore); Really, do these stuff need to be exported? Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
