Thank you Daniel for the review.
> On 27 Aug 2025, at 9:11 PM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Mon, Aug 25, 2025 at 04:38:33PM +0530, Sudhakar Kuppusamy wrote:
>> Introducing the following GRUB commands to manage the db list.
>>
>> 1. append_list_db:
>> Show the list of trusted certificates from the db list
>> 2. append_add_db_cert:
>> Add the trusted certificate to the db list
>> 3. append_rm_dbx_cert:
>> Remove the distrusted certificate from the db list
>> 4. append_verify:
>> Verify the signed file using db list
>>
>> Signed-off-by: Daniel Axtens <d...@axtens.net>
>> Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
>
> Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>
>
> Just three nits below...
>
>> ---
>> grub-core/commands/appendedsig/appendedsig.c | 310 +++++++++++++++++++
>> 1 file changed, 310 insertions(+)
>>
>> diff --git a/grub-core/commands/appendedsig/appendedsig.c
>> b/grub-core/commands/appendedsig/appendedsig.c
>> index 5eb7b768a..932bf2a7a 100644
>> --- a/grub-core/commands/appendedsig/appendedsig.c
>> +++ b/grub-core/commands/appendedsig/appendedsig.c
>> @@ -95,6 +95,14 @@ struct grub_database db = {.certs = NULL, .cert_entries =
>> 0};
>> */
>> static bool check_sigs = false;
>>
>> +/* Appended signature size. */
>> +static grub_size_t append_sig_len = 0;
>> +
>> +static void
>> +register_appended_signatures_cmd (void);
>> +static void
>> +unregister_appended_signatures_cmd (void);
>> +
>> static grub_ssize_t
>> pseudo_read (struct grub_file *file, char *buf, grub_size_t len)
>> {
>> @@ -108,6 +116,63 @@ static struct grub_fs pseudo_fs = {
>> .fs_read = pseudo_read
>> };
>>
>> +/*
>> + * We cannot use hexdump() to display hash data because it is typically
>> + * displayed in hexadecimal format, along with an ASCII representation of
>> + * the same data.
>> + * Example: sha256 hash data
>> + * 00000000 52 b5 90 49 64 de 22 d7 4e 5f 4f b4 1b 51 9c 34
>> |R..Id.".N_O..Q.4|
>> + * 00000010 b1 96 21 7c 91 78 a5 0d 20 8c e9 5c 22 54 53 f7 |..!|.x..
>> ..\"TS.|
>> + *
>> + * An appended signature only required to display the hexadecimal of the
>> hash data
>> + * by separating each byte with ":". So, we introduced a new method
>> dump_data_to_hex
>> + * to display it.
>> + * Example: Sha256 hash data
>> + * 52:b5:90:49:64:de:22:d7:4e:5f:4f:b4:1b:51:9c:34:
>> + * b1:96:21:7c:91:78:a5:0d:20:8c:e9:5c:22:54:53:f7
>> + */
>> +static void
>> +dump_data_to_hex (const grub_uint8_t *data, const grub_size_t length)
>
> s/dump_data_to_hex/hexdump_colon/
Sure, will change it.
>
>> +{
>> + grub_size_t i, count = 0;
>> +
>> + for (i = 0; i < length - 1; i++)
>> + {
>> + grub_printf ("%02x:", data[i]);
>> + count++;
>> + if (count == 16)
>> + {
>> + grub_printf ("\n ");
>> + count = 0;
>> + }
>> + }
>> +
>> + grub_printf ("%02x\n", data[i]);
>> +}
>
> [...]
>
>> +static grub_err_t
>> +remove_cert_from_db (const grub_uint8_t *data, const grub_size_t data_size)
>> +{
>> + grub_err_t rc;
>> + struct x509_certificate *cert;
>> +
>> + if (data == NULL || data_size == 0)
>> + return grub_error (GRUB_ERR_OUT_OF_RANGE, "certificate data or size is
>> not available");
>> +
>> + cert = grub_zalloc (sizeof (struct x509_certificate));
>> + if (cert == NULL)
>> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
>> +
>> + rc = parse_x509_certificate (data, data_size, cert);
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + grub_dprintf ("appendedsig", "cannot remove a certificate from the db
>> list\n");
>
> s/a certificate/an invalid certificate/
Sure, will do it.
>
>> + grub_free (cert);
>> + return rc;
>> + }
>> +
>> + add_cert_fingerprint (data, data_size, cert);
>> +
>> + /* Remove certificate from the db list. */
>> + _remove_cert_from_db (cert);
>> +
>> + return rc;
>> +}
>
> [...]
>
>> +static grub_command_t cmd_verify, cmd_list_db, cmd_dbx_cert, cmd_db_cert;
>> +
>> +/* It registers the appended signatures GRUB commands. */
>> +static void
>> +register_appended_signatures_cmd (void)
>> +{
>> + cmd_verify = grub_register_command ("append_verify",
>> grub_cmd_verify_signature, N_("SIGNED_FILE"),
>> + N_("Verify SIGNED_FILE against the
>> trusted X.509 certificates in the db list"));
>> + cmd_list_db = grub_register_command ("append_list_db", grub_cmd_list_db,
>> 0,
>> + N_("Show the list of trusted X.509
>> certificates from the db list"));
>> + cmd_db_cert = grub_register_command ("append_add_db_cert",
>> grub_cmd_db_cert, N_("X509_CERTIFICATE"),
>> + N_("Add trusted X509_CERTIFICATE to
>> the db list"));
>> + cmd_dbx_cert = grub_register_command ("append_rm_dbx_cert",
>> grub_cmd_dbx_cert, N_("X509_CERTIFICATE"),
>> + N_("Remove distrusted
>> X509_CERTIFICATE from the db list"));
>> +}
>> +
>> +/* It unregisters the appended signatures GRUB commands. */
>> +static void
>> +unregister_appended_signatures_cmd (void)
>> +{
>> + grub_unregister_command (cmd_verify);
>> + grub_unregister_command (cmd_list_db);
>> + grub_unregister_command (cmd_db_cert);
>> + grub_unregister_command (cmd_dbx_cert);
>> +}
>
> I cannot seen any reason to have register_appended_signatures_cmd() and
> unregister_appended_signatures_cmd() functions. Please put their code
> directly into GRUB_MOD_INIT() and GRUB_MOD_FINI().
Subsequent patches (patch 17) use these functions to register/unregister the
static and
dynamic key support GRUB commands based on key management. That's why I kept it
as a separate functions.
Thanks,
Sudhakar
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
