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

[Vladimir 'phcoder' Serbinenko]

Passing signature size to the tools looks very clumsy. How does the
user know the signature size? Can we somehow determine it
programmatically ourselves?

On Wed, Dec 18, 2024 at 5:58 PM Sudhakar Kuppusamy
<sudhakar@linux.ibm.com> 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>
> ---
>  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;
>    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;
> +      appsig_size = grub_strtol (arg, &end, 10);
> +      if (grub_errno)
> +        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..6c5063ac2 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);
> +      if (grub_errno)
> +        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;
> +      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);
>        }
>        break;
> --
> 2.43.5
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko