Re: [PATCH v2 01/21] powerpc-ieee1275: Add support for signing grub with an appended signature

[sudhakar <sudhakar@linux.ibm.com>]

Hi Daniel,

Thank you for your valuable review comments.


On 2025-05-22 23:19, Daniel Kiper wrote:
> On Thu, Mar 27, 2025 at 01:02:22AM +0530, Sudhakar Kuppusamy wrote:
>> From: Rashmica Gupta <rashmica.g@gmail.com>
>> 
>> Add infrastructure to allow firmware to verify the integrity of grub
>> by use of a Linux-kernel-module-style appended signature. We initially
>> target powerpc-ieee1275, but the code should be extensible to other
>> platforms.
>> 
>> Usually these signatures are appended to a file without modifying the
>> ELF file itself. (This is what the 'sign-file' tool does, for 
>> example.)
>> The verifier loads the signed file from the file system and looks at 
>> the
>> end of the file for the appended signature. However, on 
>> powerpc-ieee1275
>> platforms, the bootloader is often stored directly in the PReP 
>> partition
>> as raw bytes without a file-system. This makes determining the 
>> location
>> of an appended signature more difficult.
>> 
>> To address this, we add a new ELF note.
>> 
>> The name field of shall be the string "Appended-Signature", 
>> zero-padded
>> to 4 byte alignment. The type field shall be 0x41536967 (the ASCII 
>> values
>> for the string "ASig"). It must be the final section in the ELF 
>> binary.
>> 
>> The description shall contain the appended signature structure as 
>> defined
>> by the Linux kernel. The description will also be padded to be a 
>> multiple
>> of 4 bytes. The padding shall be added before the appended signature
>> structure (not at the end) so that the final bytes of a signed ELF 
>> file
>> are the appended signature magic.
>> 
>> A subsequent patch documents how to create a grub core.img validly 
>> signed
>> under this scheme.
>> 
>> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  include/grub/util/install.h |  7 +++++--
>>  include/grub/util/mkimage.h |  4 ++--
>>  util/grub-install-common.c  | 15 ++++++++++++---
>>  util/grub-mkimage.c         | 11 +++++++++++
>>  util/grub-mkimagexx.c       | 38 
>> ++++++++++++++++++++++++++++++++++++-
>>  util/mkimage.c              |  6 +++---
>>  6 files changed, 70 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>> index 5c0a52ca2..3aabc4285 100644
>> --- a/include/grub/util/install.h
>> +++ b/include/grub/util/install.h
>> @@ -69,6 +69,8 @@
>>        N_("disable shim_lock verifier"), 0 },				\
>>    { "disable-cli", GRUB_INSTALL_OPTIONS_DISABLE_CLI, 0, 0,		\
>>      N_("disabled command line interface access"), 0 },			\
>> +  { "appended-signature-size", 
>> GRUB_INSTALL_OPTIONS_APPENDED_SIGNATURE_SIZE,  \
>> +    "SIZE", 0, N_("Add a note segment reserving SIZE bytes for an 
>> appended signature"), 1}, \
>>    { "verbose", 'v', 0, 0,						\
>>      N_("print verbose messages."), 1 }
>> 
>> @@ -132,7 +134,8 @@ enum grub_install_options {
>>    GRUB_INSTALL_OPTIONS_DTB,
>>    GRUB_INSTALL_OPTIONS_SBAT,
>>    GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK,
>> -  GRUB_INSTALL_OPTIONS_DISABLE_CLI
>> +  GRUB_INSTALL_OPTIONS_DISABLE_CLI,
>> +  GRUB_INSTALL_OPTIONS_APPENDED_SIGNATURE_SIZE
>>  };
>> 
>>  extern char *grub_install_source_directory;
>> @@ -192,7 +195,7 @@ grub_install_generate_image (const char *dir, 
>> const char *prefix,
>>  			     size_t npubkeys,
>>  			     char *config_path,
>>  			     const struct grub_install_image_target_desc *image_target,
>> -			     int note,
>> +			     int note, size_t appsig_size,
>>  			     grub_compression_t comp, const char *dtb_file,
>>  			     const char *sbat_path, const int disable_shim_lock,
>>  			     const int disable_cli);
>> diff --git a/include/grub/util/mkimage.h b/include/grub/util/mkimage.h
>> index 9d74f82c5..0d40383eb 100644
>> --- a/include/grub/util/mkimage.h
>> +++ b/include/grub/util/mkimage.h
>> @@ -51,12 +51,12 @@ grub_mkimage_load_image64 (const char 
>> *kernel_path,
>>  			   const struct grub_install_image_target_desc *image_target);
>>  void
>>  grub_mkimage_generate_elf32 (const struct 
>> grub_install_image_target_desc *image_target,
>> -			     int note, char *sbat, char **core_img, size_t *core_size,
>> +			     int note, char *sbat, size_t appsig_size, char **core_img, 
>> size_t *core_size,
>>  			     Elf32_Addr target_addr,
>>  			     struct grub_mkimage_layout *layout);
>>  void
>>  grub_mkimage_generate_elf64 (const struct 
>> grub_install_image_target_desc *image_target,
>> -			     int note, char *sbat, char **core_img, size_t *core_size,
>> +			     int note, char *sbat, size_t appsig_size, char **core_img, 
>> size_t *core_size,
>>  			     Elf64_Addr target_addr,
>>  			     struct grub_mkimage_layout *layout);
>> 
>> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>> index 22bccb6a3..22f0e56cb 100644
>> --- a/util/grub-install-common.c
>> +++ b/util/grub-install-common.c
>> @@ -467,10 +467,12 @@ static char *sbat;
>>  static int disable_shim_lock;
>>  static grub_compression_t compression;
>>  static int disable_cli;
>> +static size_t appsig_size;
>> 
>>  int
>>  grub_install_parse (int key, char *arg)
>>  {
>> +  const char *end;
> 
> Please add empty line after this. In general separate definitions from
> the code.
> 
>>    switch (key)
>>      {
>>      case GRUB_INSTALL_OPTIONS_INSTALL_CORE_COMPRESS:
>> @@ -571,6 +573,12 @@ grub_install_parse (int key, char *arg)
>>        grub_util_error (_("Unrecognized compression `%s'"), arg);
>>      case GRUB_INSTALL_OPTIONS_GRUB_MKIMAGE:
>>        return 1;
>> +    case GRUB_INSTALL_OPTIONS_APPENDED_SIGNATURE_SIZE:
>> +      grub_errno = 0;
> 
> If you have enum types please use them properly...
> 
>   grub_errno = GRUB_ERR_NONE
> 
>> +      appsig_size = grub_strtol (arg, &end, 10);
>> +      if (grub_errno)
> 
> This check is not reliable. Good example how it should be done is in
> commit ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).
> 
> ... and if (grub_errno != GRUB_ERR_NONE)
> 
> Please fix above mentioned issues in all patches.
> 
>> +        return 0;
>> +      return 1;
>>      default:
>>        return 0;
>>      }
>> @@ -683,9 +691,10 @@ grub_install_make_image_wrap_file (const char 
>> *dir, const char *prefix,
>>    *p = '\0';
>> 
>>    grub_util_info ("grub-mkimage --directory '%s' --prefix '%s' 
>> --output '%s'"
>> -		  " --format '%s' --compression '%s'%s%s%s%s\n",
>> +		  " --format '%s' --compression '%s'"
>> +		  " --appended-signature-size %zu %s %s %s %s\n",
>>  		  dir, prefix, outname,
>> -		  mkimage_target, compnames[compression],
>> +		  mkimage_target, compnames[compression], appsig_size,
>>  		  note ? " --note" : "",
>>  		  disable_shim_lock ? " --disable-shim-lock" : "",
>>  		  disable_cli ? " --disable-cli" : "", s);
>> @@ -698,7 +707,7 @@ grub_install_make_image_wrap_file (const char 
>> *dir, const char *prefix,
>>    grub_install_generate_image (dir, prefix, fp, outname,
>>  			       modules.entries, memdisk_path,
>>  			       pubkeys, npubkeys, config_path, tgt,
>> -			       note, compression, dtb, sbat,
>> +			       note, appsig_size, compression, dtb, sbat,
>>  			       disable_shim_lock, disable_cli);
>>    while (dc--)
>>      grub_install_pop_module ();
>> diff --git a/util/grub-mkimage.c b/util/grub-mkimage.c
>> index 547f7310f..5b6c99d83 100644
>> --- a/util/grub-mkimage.c
>> +++ b/util/grub-mkimage.c
>> @@ -84,6 +84,7 @@ static struct argp_option options[] = {
>>    {"sbat", 's', N_("FILE"), 0, N_("SBAT metadata"), 0},
>>    {"disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0, 
>> N_("disable shim_lock verifier"), 0},
>>    {"disable-cli", GRUB_INSTALL_OPTIONS_DISABLE_CLI, 0, 0, N_("disable 
>> command line interface access"), 0},
>> +  {"appended-signature-size", 'S', N_("SIZE"), 0, N_("Add a note 
>> segment reserving SIZE bytes for an appended signature"), 0},
>>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>>    { 0, 0, 0, 0, 0, 0 }
>>  };
>> @@ -130,6 +131,7 @@ struct arguments
>>    int note;
>>    int disable_shim_lock;
>>    int disable_cli;
>> +  size_t appsig_size;
>>    const struct grub_install_image_target_desc *image_target;
>>    grub_compression_t comp;
>>  };
>> @@ -140,6 +142,7 @@ argp_parser (int key, char *arg, struct argp_state 
>> *state)
>>    /* Get the input argument from argp_parse, which we
>>       know is a pointer to our arguments structure. */
>>    struct arguments *arguments = state->input;
>> +  const char *end;
>> 
>>    switch (key)
>>      {
>> @@ -172,6 +175,13 @@ argp_parser (int key, char *arg, struct 
>> argp_state *state)
>>        arguments->note = 1;
>>        break;
>> 
>> +    case 'S':
>> +      grub_errno = 0;
>> +      arguments->appsig_size = grub_strtol (arg, &end, 10);
> 
> As above...
> 
>> +      if (grub_errno)
> 
> Ditto.
> 
>> +        return 0;
>> +      break;
>> +
>>      case 'm':
>>        if (arguments->memdisk)
>>  	free (arguments->memdisk);
>> @@ -330,6 +340,7 @@ main (int argc, char *argv[])
>>  			       arguments.memdisk, arguments.pubkeys,
>>  			       arguments.npubkeys, arguments.config,
>>  			       arguments.image_target, arguments.note,
>> +			       arguments.appsig_size,
>>  			       arguments.comp, arguments.dtb,
>>  			       arguments.sbat, arguments.disable_shim_lock,
>>  			       arguments.disable_cli);
>> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
>> index 448862b2e..6fe348e5b 100644
>> --- a/util/grub-mkimagexx.c
>> +++ b/util/grub-mkimagexx.c
>> @@ -115,6 +115,14 @@ struct grub_sbat_note {
>>    char name[ALIGN_UP(sizeof(GRUB_SBAT_NOTE_NAME), 4)];
>>  };
>> 
>> +#define GRUB_APPENDED_SIGNATURE_NOTE_NAME "Appended-Signature"
>> +#define GRUB_APPENDED_SIGNATURE_NOTE_TYPE 0x41536967 /* "ASig" */
>> +struct grub_appended_signature_note
>> +{
>> +  Elf32_Nhdr header;
>> +  char name[ALIGN_UP (sizeof (GRUB_APPENDED_SIGNATURE_NOTE_NAME), 
>> 4)];
>> +};
>> +
>>  static int
>>  is_relocatable (const struct grub_install_image_target_desc 
>> *image_target)
>>  {
>> @@ -216,7 +224,7 @@ grub_arm_reloc_jump24 (grub_uint32_t *target, 
>> Elf32_Addr sym_addr)
>> 
>>  void
>>  SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc *image_target,
>> -				    int note, char *sbat, char **core_img, size_t *core_size,
>> +				    int note, char *sbat, size_t appsig_size, char **core_img, 
>> size_t *core_size,
>>  				    Elf_Addr target_addr,
>>  				    struct grub_mkimage_layout *layout)
>>  {
>> @@ -237,6 +245,12 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>        footer_size += ALIGN_UP (sizeof (struct grub_sbat_note) + 
>> layout->sbat_size, 4);
>>      }
>> 
>> +  if (appsig_size)
>> +    {
>> +      phnum++;
>> +      footer_size += ALIGN_UP (sizeof (struct 
>> grub_appended_signature_note) + appsig_size, 4);
>> +    }
>> +
>>    if (image_target->id != IMAGE_LOONGSON_ELF)
>>      phnum += 2;
>> 
>> @@ -527,6 +541,28 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>        phdr->p_filesz = grub_host_to_target32 (note_size);
>>        phdr->p_memsz = 0;
>>        phdr->p_offset = grub_host_to_target32 (header_size + 
>> program_size + footer_offset);
>> +      footer += note_size;
>> +      footer_offset += note_size;
>> +    }
>> +
>> +  if (appsig_size)
>> +    {
>> +      int note_size = ALIGN_UP (sizeof (struct 
>> grub_appended_signature_note) + appsig_size, 4);
>> +      struct grub_appended_signature_note *note_ptr = (struct 
>> grub_appended_signature_note *) footer;
> 
> Again, missing empty line here...
> 
>> +      note_ptr->header.n_namesz = grub_host_to_target32 (sizeof 
>> (GRUB_APPENDED_SIGNATURE_NOTE_NAME));
>> +      /* needs to sit at the end, so we round this up and sign some 
>> zero padding */
>> +      note_ptr->header.n_descsz = grub_host_to_target32 (ALIGN_UP 
>> (appsig_size, 4));
>> +      note_ptr->header.n_type = grub_host_to_target32 
>> (GRUB_APPENDED_SIGNATURE_NOTE_TYPE);
>> +      strcpy (note_ptr->name, GRUB_APPENDED_SIGNATURE_NOTE_NAME);
>> +      phdr++;
>> +      phdr->p_type = grub_host_to_target32 (PT_NOTE);
>> +      phdr->p_flags = grub_host_to_target32 (PF_R);
>> +      phdr->p_align = grub_host_to_target32 
>> (image_target->voidp_sizeof);
>> +      phdr->p_vaddr = 0;
>> +      phdr->p_paddr = 0;
>> +      phdr->p_filesz = grub_host_to_target32 (note_size);
>> +      phdr->p_memsz = 0;
>> +      phdr->p_offset = grub_host_to_target32 (header_size + 
>> program_size + footer_offset);
>>      }
>> 
>>    {
>> diff --git a/util/mkimage.c b/util/mkimage.c
>> index b46df2909..f5c59f563 100644
>> --- a/util/mkimage.c
>> +++ b/util/mkimage.c
>> @@ -885,7 +885,7 @@ grub_install_generate_image (const char *dir, 
>> const char *prefix,
>>  			     char *memdisk_path, char **pubkey_paths,
>>  			     size_t npubkeys, char *config_path,
>>  			     const struct grub_install_image_target_desc *image_target,
>> -			     int note, grub_compression_t comp, const char *dtb_path,
>> +			     int note, size_t appsig_size, grub_compression_t comp, const 
>> char *dtb_path,
>>  			     const char *sbat_path, int disable_shim_lock,
>>  			     int disable_cli)
>>  {
>> @@ -1833,10 +1833,10 @@ grub_install_generate_image (const char *dir, 
>> const char *prefix,
>>  	else
>>  	  target_addr = image_target->link_addr;
>>  	if (image_target->voidp_sizeof == 4)
>> -	  grub_mkimage_generate_elf32 (image_target, note, sbat, &core_img, 
>> &core_size,
>> +	  grub_mkimage_generate_elf32 (image_target, note, sbat, 
>> appsig_size, &core_img, &core_size,
>>  				       target_addr, &layout);
>>  	else
>> -	  grub_mkimage_generate_elf64 (image_target, note, sbat, &core_img, 
>> &core_size,
>> +	  grub_mkimage_generate_elf64 (image_target, note, sbat, 
>> appsig_size, &core_img, &core_size,
>>  				       target_addr, &layout);
> 
> What will happen if somebody attempts to generate PE image with 
> appended
> signature? I think GRUB utils should complain then loudly.

v2 won't complain if somebody attempts to generate PE image with 
appended signature.

In v3, I have added the check condition to complain loudly if somebody 
attempts to generate PE image with appended signature.


Thanks,
Sudhakar Kuppusamy


> Daniel

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