Re: [PATCH v2 04/22] Add suport for signing grub with an appended signature

[Stefan Berger <stefanb@linux.ibm.com>]

On 6/30/21 4:40 AM, Daniel Axtens 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: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
>
> ---
>
> You can experiment with this code with a patched version of SLOF
> that verifies these signatures. You can find one at:
>     https://github.com/daxtens/SLOF
>
> I will be proposing this for inclusion in a future Power Architecture
> Platform Reference (PAPR).
> ---
>   include/grub/util/install.h |  8 ++++++--
>   include/grub/util/mkimage.h |  4 ++--
>   util/grub-install-common.c  | 15 +++++++++++---
>   util/grub-mkimage.c         | 11 +++++++++++
>   util/grub-mkimagexx.c       | 39 ++++++++++++++++++++++++++++++++++++-
>   util/mkimage.c              | 13 +++++++------
>   6 files changed, 76 insertions(+), 14 deletions(-)
>
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 7df3191f47ef..cf4531e02b66 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -67,6 +67,9 @@
>         N_("SBAT metadata"), 0 },						\
>     { "disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0,	\
>         N_("disable shim_lock verifier"), 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 }
>   
> @@ -128,7 +131,8 @@ enum grub_install_options {
>     GRUB_INSTALL_OPTIONS_INSTALL_CORE_COMPRESS,
>     GRUB_INSTALL_OPTIONS_DTB,
>     GRUB_INSTALL_OPTIONS_SBAT,
> -  GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK
> +  GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK,
> +  GRUB_INSTALL_OPTIONS_APPENDED_SIGNATURE_SIZE
>   };
>   
>   extern char *grub_install_source_directory;
> @@ -188,7 +192,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);
>   
> diff --git a/include/grub/util/mkimage.h b/include/grub/util/mkimage.h
> index 3819a67441c8..6f1da89b9b65 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 **core_img, size_t *core_size,
> +			     int note, 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 **core_img, size_t *core_size,
> +			     int note, 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 4e212e690c52..1216a203c292 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -461,10 +461,12 @@ static size_t npubkeys;
>   static char *sbat;
>   static int disable_shim_lock;
>   static grub_compression_t compression;
> +static size_t appsig_size;
>   
>   int
>   grub_install_parse (int key, char *arg)
>   {
> +  const char *end;
>     switch (key)
>       {
>       case 'C':
> @@ -562,6 +564,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;
> +      appsig_size = grub_strtol(arg, &end, 10);
> +      if (grub_errno)
> +        return 0;
> +      return 1;
>       default:
>         return 0;
>       }
> @@ -661,10 +669,11 @@ grub_install_make_image_wrap_file (const char *dir, const char *prefix,
>   		  " --output '%s' "
>   		  " --dtb '%s' "
>   		  "--sbat '%s' "
> -		  "--format '%s' --compression '%s' %s %s %s\n",
> +		  "--format '%s' --compression '%s' "
> +		  "--appended-signature-size %zu %s %s %s\n",
>   		  dir, prefix,
>   		  outname, dtb ? : "", sbat ? : "", mkimage_target,
> -		  compnames[compression], note ? "--note" : "",
> +		  compnames[compression], appsig_size, note ? "--note" : "",
>   		  disable_shim_lock ? "--disable-shim-lock" : "", s);
>     free (s);
>   
> @@ -675,7 +684,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);
>     while (dc--)
>       grub_install_pop_module ();
> diff --git a/util/grub-mkimage.c b/util/grub-mkimage.c
> index c0d559937020..d01eaeb8443a 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},
>     {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
> +  {"appended-signature-size", 'S', N_("SIZE"), 0, N_("Add a note segment reserving SIZE bytes for an appended signature"), 0},
>     { 0, 0, 0, 0, 0, 0 }
>   };
>   
> @@ -128,6 +129,7 @@ struct arguments
>     char *sbat;
>     int note;
>     int disable_shim_lock;
> +  size_t appsig_size;
>     const struct grub_install_image_target_desc *image_target;
>     grub_compression_t comp;
>   };
> @@ -138,6 +140,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;


Nit: char* end -> char *end;

Otherwise looks good to me.


>   
>     switch (key)
>       {
> @@ -170,6 +173,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);
> +      if (grub_errno)
> +        return 0;
> +      break;
> +
>       case 'm':
>         if (arguments->memdisk)
>   	free (arguments->memdisk);
> @@ -324,6 +334,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);
>   
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index d78fa3e53308..393119486d3f 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -84,6 +84,15 @@ struct grub_ieee1275_note
>     struct grub_ieee1275_note_desc descriptor;
>   };
>   
> +#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)];
> +};
> +
>   #define GRUB_XEN_NOTE_NAME "Xen"
>   
>   struct fixup_block_list
> @@ -207,7 +216,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 **core_img, size_t *core_size,
> +				    int note, size_t appsig_size, char **core_img, size_t *core_size,
>   				    Elf_Addr target_addr,
>   				    struct grub_mkimage_layout *layout)
>   {
> @@ -221,6 +230,12 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>     int shnum = 4;
>     int string_size = sizeof (".text") + sizeof ("mods") + 1;
>   
> +  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;
>   
> @@ -484,6 +499,28 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>         phdr->p_offset = grub_host_to_target32 (header_size + program_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 *)
> +      (elf_img + program_size + header_size + (note ? sizeof (struct grub_ieee1275_note) : 0));
> +
> +    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 + (note ? sizeof (struct grub_ieee1275_note) : 0));
> +  }
> +
>     {
>       char *str_start = (elf_img + sizeof (*ehdr) + phnum * sizeof (*phdr)
>   		       + shnum * sizeof (*shdr));
> diff --git a/util/mkimage.c b/util/mkimage.c
> index a26cf76f72f2..d2cb33883557 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -869,8 +869,9 @@ 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,
> -			     const char *sbat_path, int disable_shim_lock)
> +			     int note, size_t appsig_size, grub_compression_t comp,
> +			     const char *dtb_path, const char *sbat_path,
> +			     int disable_shim_lock)
>   {
>     char *kernel_img, *core_img;
>     size_t total_module_size, core_size;
> @@ -1773,11 +1774,11 @@ 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, &core_img, &core_size,
> -				       target_addr, &layout);
> +	  grub_mkimage_generate_elf32 (image_target, note, appsig_size, &core_img,
> +				       &core_size, target_addr, &layout);
>   	else
> -	  grub_mkimage_generate_elf64 (image_target, note, &core_img, &core_size,
> -				       target_addr, &layout);
> +	  grub_mkimage_generate_elf64 (image_target, note, appsig_size, &core_img,
> +				       &core_size, target_addr, &layout);
>         }
>         break;
>       }