On Thu, Aug 21, 2025 at 10:34:18AM +0800, Gary Lin wrote:
> On Tue, Aug 19, 2025 at 06:43:15PM +0530, Sudhakar Kuppusamy wrote:
> > If secure boot is enabled with static key management mode, the trusted
> > certificates will be extracted from the GRUB ELF Note and added to db list.
> > This is introduced by a subsequent patch.
> >
> > If secure boot is enabled with dynamic key management mode, the trusted
> > certificates and certificate/binary hash will be extracted from the PKS
> > and added to db list. The distrusted certificates, certificate/binary hash
> > are read from the PKS and added to dbx list. Both dbx and db lists are
> > introduced by
> > a subsequent patch.
> >
> > Note:-
> >
> > If the certificate or the certificate hash exists in the dbx list, then
> > do not add that certificate/certificate hash to the db list.
> >
> > Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
> > Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
> > Reviewed-by: Avnish Chouhan <avn...@linux.ibm.com>
> > ---
> > grub-core/commands/appendedsig/appendedsig.c | 452 ++++++++++++++++++-
> > include/grub/crypto.h | 1 +
> > include/grub/efi/pks.h | 112 +++++
> > include/grub/types.h | 4 +
> > 4 files changed, 563 insertions(+), 6 deletions(-)
> > create mode 100644 include/grub/efi/pks.h
> >
-->8--
> > +/* Add the certificate/binary hash into the db/dbx list. */
> > +static grub_err_t
> > +add_hash (const grub_uint8_t **data, const grub_size_t data_size,
> Do you intend to make 'data' a const variable in 'add_hash()'? If so, it
> should be 'grub_uint8_t ** const data'. With 'const grub_uint8_t **',
> 'data' becomes a pointer to a constant, and it doesn't match the
> parameter passed in create_dbx_list().
>
> > + grub_uint8_t ***signature_list, grub_size_t
> > **signature_size_list,
> > + grub_uint32_t *signature_list_entries)
> > +{
> > + grub_uint8_t **signatures;
> > + grub_size_t *signature_size;
> > + grub_uint32_t signature_entries = *signature_list_entries;
> > +
> > + if (*data == NULL || data_size == 0)
> > + return grub_error (GRUB_ERR_OUT_OF_RANGE, "certificate/binary-hash
> > data or size is not available");
> > +
> > + signatures = grub_realloc (*signature_list, sizeof (grub_uint8_t *) *
> > (signature_entries + 1));
> > + if (signatures == NULL)
> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > +
> > + signature_size = grub_realloc (*signature_size_list,
> > + sizeof (grub_size_t) * (signature_entries
> > + 1));
> > + if (signature_size == NULL)
> > + {
> > + /*
> > + * Allocated memory will be freed by
> > + * free_db_list/free_dbx_list.
> > + */
> > + signatures[signature_entries + 1] = NULL;
> > + *signature_list = signatures;
> > + *signature_list_entries = signature_entries + 1;
> > +
> > + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> > + }
> > +
> > + signatures[signature_entries] = (grub_uint8_t *) *data;
> > + signature_size[signature_entries] = data_size;
> > + signature_entries++;
> > + *data = NULL;
It seems that you want to use the memory from '*data' directly instead of
allocating a new buffer for the signature entry. It may be fine for some
local variables in other functions, but 'grub_pks_keystore.db[].data' and
'grub_pks_keystore.dbx[].data' are also the candidates of 'data'. By
setting '*data' to 'NULL', the data pointers of PKS variables were clean
up after invoking 'add_hash()'. It doesn't look an expected behavior to
me.
I'd suggest to declare 'data' as 'grub_uint8_t *' or 'grub_uint8_t *
const' and allocate a new buffer for signatures[signature_entries].
This keeps the 'data' pointer valid after invoking 'add_hash()' and
also prevents the 'address-of-packed-member' error mentioned in the
previous mail.
Cheers,
Gary Lin
> > +
> > + *signature_list = signatures;
> > + *signature_size_list = signature_size;
> > + *signature_list_entries = signature_entries;
> > +
> > + return GRUB_ERR_NONE;
> > +}
> > +
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
