Thank you Daniel for the review.
> On 23 Aug 2025, at 12:23 AM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Thu, Aug 21, 2025 at 01:25:04PM +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 PKS is enabled, it will read secure boot variables such as db and dbx
>> from PKS and extract EFI Signature List (ESL) from it. The ESLs 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 management mode will be activated:
>> 1. When Secure Boot is enabled with static key management mode
>> 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: Key Management Mode
>> 1 - Enable dynamic key management mode. Read the db and dbx variables from
>> PKS,
>> and use them for signature verification.
>> 0 - Enable static key management mode. Read keys from the GRUB ELF Note and
>> use it for signature verification.
>>
>> 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 | 14 +-
>> grub-core/kern/powerpc/ieee1275/ieee1275.c | 139 +++++++
>> .../kern/powerpc/ieee1275/platform_keystore.c | 341 ++++++++++++++++++
>> include/grub/ieee1275/ieee1275.h | 3 +
>> include/grub/powerpc/ieee1275/ieee1275.h | 20 +
>> .../grub/powerpc/ieee1275/platform_keystore.h | 142 ++++++++
>> 9 files changed, 662 insertions(+), 2 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/Makefile.am b/grub-core/Makefile.am
>> index e50db8106..cd6bb7c32 100644
>> --- a/grub-core/Makefile.am
>> +++ b/grub-core/Makefile.am
>> @@ -241,11 +241,13 @@ KERNEL_HEADER_FILES +=
>> $(top_builddir)/include/grub/machine/kernel.h
>> endif
>>
>> if COND_powerpc_ieee1275
>> +KERNEL_HEADER_FILES +=
>> $(top_srcdir)/include/grub/powerpc/ieee1275/ieee1275.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/ieee1275/ieee1275.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/ieee1275/alloc.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/terminfo.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/lib/arg.h
>> +KERNEL_HEADER_FILES +=
>> $(top_srcdir)/include/grub/powerpc/ieee1275/platform_keystore.h
>
> I am not entirely sure why you cannot put these two changes next to each
> other…
Sure, will put these two changes next to each other.
>
>> endif
>>
>> if COND_sparc64_ieee1275
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index d91694de0..05ccbf264 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -328,10 +328,12 @@ kernel = {
>> extra_dist = video/sis315_init.c;
>> mips_loongson = commands/keylayouts.c;
>>
>> + powerpc_ieee1275 = kern/powerpc/ieee1275/ieee1275.c;
>> powerpc_ieee1275 = kern/powerpc/cache.S;
>> powerpc_ieee1275 = kern/powerpc/dl.c;
>> powerpc_ieee1275 = kern/powerpc/compiler-rt.S;
>> powerpc_ieee1275 = kern/lockdown.c;
>> + powerpc_ieee1275 = kern/powerpc/ieee1275/platform_keystore.c;
>
> Ditto…
Will do.
>
>> sparc64_ieee1275 = kern/sparc64/cache.S;
>> sparc64_ieee1275 = kern/sparc64/dl.c;
>> diff --git a/grub-core/kern/ieee1275/ieee1275.c
>> b/grub-core/kern/ieee1275/ieee1275.c
>> index 36ca2dbfc..afa37a9f0 100644
>> --- a/grub-core/kern/ieee1275/ieee1275.c
>> +++ b/grub-core/kern/ieee1275/ieee1275.c
>> @@ -23,7 +23,6 @@
>>
>> #define IEEE1275_PHANDLE_INVALID ((grub_ieee1275_cell_t) -1)
>> #define IEEE1275_IHANDLE_INVALID ((grub_ieee1275_cell_t) 0)
>> -#define IEEE1275_CELL_INVALID ((grub_ieee1275_cell_t) -1)
>>
>>
>>
>> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
>> index 89303f33e..46bfaa9a8 100644
>> --- a/grub-core/kern/ieee1275/init.c
>> +++ b/grub-core/kern/ieee1275/init.c
>> @@ -51,6 +51,8 @@
>> #endif
>> #if defined(__powerpc__)
>> #include <grub/lockdown.h>
>> +#include <grub/powerpc/ieee1275/ieee1275.h>
>> +#include <grub/powerpc/ieee1275/platform_keystore.h>
>> #endif
>>
>> #ifdef __powerpc__
>> @@ -1007,6 +1009,7 @@ grub_parse_cmdline (void)
>> static void
>> grub_ieee1275_get_secure_boot (void)
>> {
>> + grub_err_t err;
>> grub_ieee1275_phandle_t root;
>> grub_uint32_t sb_mode = GRUB_SB_DISABLED;
>> int rc;
>> @@ -1038,7 +1041,16 @@ grub_ieee1275_get_secure_boot (void)
>> * Now, only support disabled and enforced.
>> */
>> if (sb_mode == GRUB_SB_ENFORCED)
>> - grub_lockdown ();
>> + {
>> + grub_dprintf ("ieee1275", "Secure Boot Enabled\n");
>> + grub_lockdown ();
>> + }
>> + else
>> + grub_dprintf ("ieee1275", "Secure Boot Disabled\n");
>
> Sorry, this change does not belong to this patch.
Sure, will do it.
>
>> + err = grub_pks_keystore_init ();
>> + if (err != GRUB_ERR_NONE)
>> + grub_error (err, "initialization of the Platform KeyStore failed\n");
>
> Only this one...
>
>> }
>> #endif /* __powerpc__ */
>
> [...]
>
>> 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..aaf90500f
>> --- /dev/null
>> +++ b/grub-core/kern/powerpc/ieee1275/platform_keystore.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * 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>
>> +
>> +#if __GNUC__ >= 9
>> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
>> +#endif
>> +
>> +/* PKS consumer type for firmware. */
>> +#define PKS_CONSUMER_FW ((grub_uint8_t) 1)
>> +
>> +/* The maximum object size interface name for a PKS object. */
>> +#define PKS_MAX_OBJ_SIZE ((grub_uint8_t *) "pks-max-object-size")
>> +
>> +/* 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;
>> +/*
>> + * Flag to indicate the key management.
>> + * True: Dynamic key management (use Platform KeySotre).
>> + * False: Static key management (use built-in Keys).
>> + */
>> +bool grub_pks_use_keystore = false;
>> +/*
>> + * Flag to indicate the availability of PSK support.
>> + * True: PKS support available.
>> + * False: No PKS support.
>> + */
>> +bool grub_pks_is_support_pks = false;
>
> Why could not we use grub_pks_use_keystore only instead of both
> grub_pks_use_keystore and grub_pks_is_support_pks? If you convince me
> that both are needed then s/grub_pks_is_support_pks/grub_pks_supported/…
Ok. I will be strict with one variable, i.e., grub_pks_keystore,
and will add the variables use_keystore and pks_supported to the member of
grub_pks_keystore.
>
>> +/* Platform KeyStore db and dbx. */
>> +grub_pks_t grub_pks_keystore = { .db = NULL, .dbx = NULL, .db_entries = 0,
>> .dbx_entries = 0 };
>> +
>> +/*
>> + * Import the Globally Unique Identifier (GUID), EFI Signature Database
>> (ESD),
>> + * and its size into the PKS Signature Database (SD) (i.e pks_sd buffer)
>> and pks_sd entries
>> + * from the EFI Signature List (ESL).
>> + */
>> +static grub_err_t
>> +esd_from_esl (const grub_uint8_t *esl_data, grub_size_t esl_size,
>
> s/esd_from_esl/__esl_to_esd/
Sure, will do it.
>
>> + const grub_size_t signature_size, const grub_packed_guid_t
>> *guid,
>> + grub_pks_sd_t **pks_sd, grub_uint32_t *pks_sd_entries)
>> +{
>> + grub_esd_t *esd;
>> + grub_pks_sd_t *signature = *pks_sd;
>> + grub_uint32_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_uint32_t *pks_sd_entries)
>> +{
>> + grub_packed_guid_t guid;
>> + grub_esl_t *esl;
>> + grub_size_t offset, esl_size,
>> + signature_size, signature_header_size;
>
> You do not need to wrap this line. I am OK with lines (a bit) longer
> than 80 characters…
Sure, will do it.
>
>> + /* Convert the ESL data into the ESL. */
>> + esl = (grub_esl_t *) esl_data;
>> + if (*next_esl < sizeof (grub_esl_t) || 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 Database (ESD) and the number of ESD from the
>> ESL
>> + * into the pks_sd buffer and pks_sd entries.
>> + */
>> +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_uint32_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_uint32_t *outlen,
>> grub_uint32_t *policy)
>> +{
>> + grub_int32_t 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,
>> pks_max_object_size, *out,
>> + 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.
>> + */
>> +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)
>> +{
>> + grub_int32_t 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,
>> pks_max_object_size, *out, outlen);
>> + if (rc == IEEE1275_CELL_NOT_FOUND)
>> + return grub_error (GRUB_ERR_UNKNOWN_COMMAND, "secure boot variable %s
>> not found (%d)",
>> + (sbvartype == DB ? "db" : "dbx"), rc);
>> + else if (rc < 0)
>> + 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)
>
> s/is_support_pks/is_pks_present/
Sure, will do it.
>
>> +{
>> + grub_int32_t rc;
>> + grub_ieee1275_cell_t missing = 0;
>> +
>> + rc = grub_ieee1275_test (PKS_MAX_OBJ_SIZE, &missing);
>> + if (rc < 0 || missing == IEEE1275_CELL_INVALID)
>> + return 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)
>> + return 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.
>> + */
>> +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_uint32_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 - Enable dynamic key management mode. Read the db and dbx variables
>> from PKS,
>> + and use them for signature verification.
>> + * 0 - Enable static key management mode. Read keys from the GRUB ELF Note
>> and
>> + * use it for signature verification.
>> + */
>> +static void
>> +get_secure_boot_version (void)
>
> s/get_secure_boot_version/is_pks_present/
>
> And it seems to me the get_secure_boot_version() should be merged with
> this function.
Sure, will do it.
Thanks,
Sudhakar
>
>> +{
>> + grub_err_t rc;
>> + grub_uint8_t *data = NULL;
>> + grub_uint32_t len = 0, policy = 0;
>> +
>> + rc = read_sbversion_from_pks (&data, &len, &policy);
>> + if (rc == GRUB_ERR_NONE && (len != 1 || (*data >= 2)))
>> + grub_error (GRUB_ERR_BAD_NUMBER, "found unexpected SB version (%d)\n",
>> *data);
>> +
>> + if (rc != GRUB_ERR_NONE)
>> + grub_dprintf ("ieee1275", "switch to static key\n");
>> + else if (*data)
>> + grub_pks_use_keystore = true;
>> +
>> + grub_free (data);
>> +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
