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/

> +{
> +  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/

> +      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().

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Reply via email to