> On 21 Aug 2025, at 8:38 AM, Gary Lin <g...@suse.com> wrote:
>
> 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.
>
Thanks a lot Gary for the review and pointing this out.
Will do it.
Thanks,
Sudhakar
> 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
