[PATCH v3 0/2] Secure Boot Advanced Targeting (SBAT) support on powerpc

[PATCH v3 0/2] Secure Boot Advanced Targeting (SBAT) support on powerpc

[Sudhakar Kuppusamy]

In powerpc,  PE format Binary are not supported and can't use shim (https://github.com/rhboot/shim/blob/main/SBAT.md).
However, ELF binary are supported. So, we created new ELF note for SBAT in ELF binary which store the SBAT data and
SBAT verifier will be there in firmware to read SBAT data from ELF note and validate it.

this patch series consists of 2 parts:

 1) Patch 1: create new ELF Note for SBAT
    
    In order to store the SBAT data, we create a new ELF note. The string "Secure-Boot-Advanced-Targeting",
    zero-padded to 4 byte alignment, shall be entered in the name field.
    The string "sbat"'s ASCII values, 0x41536967, should be entered in the type field.

 2) Patch 2: adding sbat metadata into sbat ELF Note
    
    The SBAT metadata, which is read from .csv file and transformed into an ELF 
    note,is made into an image using the -s option.


(The rest of this cover letter concerns testing the entire end-to-end setup - SBAT.)

You can experiement with this using entirely free software.

You need the following trees:

https://github.com/SudhakarKuppusamy1/qemu branch sbat
https://github.com/SudhakarKuppusamy1/SLOF branch sbat
https://github.com/SudhakarKuppusamy1/grub branch sbat

You also need:
 - the SBAT metadata (.csv file)
 - the SBAT Variable (.csv file)
Both should followed the SBAT specification 
(https://github.com/rhboot/shim/blob/main/SBAT.md)

Example: https://github.com/SudhakarKuppusamy1/testing/sbat
   
   sbat_metadata.csv
   sbat_var.csv 

Lastly you will need a working a ppc64(le) vm.

sample vm: https://github.com/SudhakarKuppusamy1/testing/vm

   pseries-ubuntu-20.04.6.qcow2

Then:

 - build qemu (./configure --target-list=ppc64-softmmu && make).
   You need qemu-system-ppc64.

 - use xxd (ex: xxd -i sbat_var.csv sbat_var.h) to convert the SBAT Variable
   for verifying grub into a header file, and copy it in to SLOF/lib/libcrypto/sbat_var.h.
   It must create variables sbat_var_csv and sbat_var_csv_len.

 - build SLOF for qemu (make qemu)

 - verify that you can boot your VM with new SLOF and stock grub.

   To boot with new SLOF, pass -bios ./SLOF/boot_rom.bin . It should
   boot with new slof in secure boot mode.

   sudo ./build/qemu-system-ppc64 -m 8192 -M pseries-2.12,accel=kvm,cap-ail-mode-3=off,secure-boot=on -nographic -vga none 
   -smp 4 -hdd pseries-ubuntu-20.04.6.qcow2 -bios ./boot_rom.bin

 - Build grub in your VM.

 - Build the SBAT metadata into grub.The following incantation should give you 
   a working but non-portable grub, assuming you have grub installed on /dev/sda2:

   GRUB_MODULES="all_video boot btrfs cat configfile echo ext2 fat font gfxmenu 
   gfxterm gzio halt hfsplus http iso9660 jpeg loadenv loopback linux lvm mdraid09 
   mdraid1x minicmd net normal part_apple part_msdos part_gpt password_pbkdf2 png 
   reboot regexp search search_fs_uuid search_fs_file search_label serial sleep 
   syslinuxcfg test tftp video xfs"

   sudo ./grub-install --modules "$GRUB_MODULES" -d ./grub-core/ -v "/dev/sda2" 
   --sbat=./sbat_metadata.csv

   dd if=/boot/grub/powerpc-ieee1275/core.elf of=/dev/sda2

Sudhakar Kuppusamy (2):
  mkimage: create new ELF Note for SBAT
  mkimage: adding sbat metadata into sbat ELF Note on powerpc

 include/grub/util/mkimage.h |  4 +--
 util/grub-mkimagexx.c       | 51 +++++++++++++++++++++++++++++++++++--
 util/mkimage.c              | 17 ++++++++++---
 3 files changed, 64 insertions(+), 8 deletions(-)

-- 
2.43.5


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

[PATCH v3 1/2] mkimage: create new ELF Note for SBAT

[Sudhakar Kuppusamy]

In order to store the SBAT data, we create a new ELF note. The string "Secure-Boot-Advanced-Targeting",
zero-padded to 4 byte alignment, shall be entered in the name field. The string "sbat"'s ASCII values,
0x41536967, should be entered in the type field.

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Co-authored-by: Daniel Axtens <dja@axtens.net>
---
 include/grub/util/mkimage.h |  4 +--
 util/grub-mkimagexx.c       | 51 +++++++++++++++++++++++++++++++++++--
 util/mkimage.c              |  5 ++--
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/include/grub/util/mkimage.h b/include/grub/util/mkimage.h
index 3819a6744..9d74f82c5 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, char *sbat, 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, char *sbat, char **core_img, size_t *core_size,
 			     Elf64_Addr target_addr,
 			     struct grub_mkimage_layout *layout);
 
diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
index e50b29533..99f6ab05e 100644
--- a/util/grub-mkimagexx.c
+++ b/util/grub-mkimagexx.c
@@ -107,6 +107,14 @@ struct section_metadata
   const char *strtab;
 };
 
+#define GRUB_SBAT_NOTE_NAME "Secure-Boot-Advanced-Targeting"
+#define GRUB_SBAT_NOTE_TYPE 0x73626174 /* "sbat" */
+
+struct grub_sbat_note {
+  Elf32_Nhdr header;
+  char name[ALIGN_UP(sizeof(GRUB_SBAT_NOTE_NAME), 4)];
+};
+
 static int
 is_relocatable (const struct grub_install_image_target_desc *image_target)
 {
@@ -208,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, char *sbat, char **core_img, size_t *core_size,
 				    Elf_Addr target_addr,
 				    struct grub_mkimage_layout *layout)
 {
@@ -217,10 +225,17 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
   Elf_Ehdr *ehdr;
   Elf_Phdr *phdr;
   Elf_Shdr *shdr;
-  int header_size, footer_size = 0;
+  int header_size, footer_size = 0, footer_offset = 0;
   int phnum = 1;
   int shnum = 4;
   int string_size = sizeof (".text") + sizeof ("mods") + 1;
+  char *footer;
+
+  if (sbat)
+    {
+      phnum++;
+      footer_size += ALIGN_UP (sizeof (struct grub_sbat_note) + layout->sbat_size, 4);
+    }
 
   if (image_target->id != IMAGE_LOONGSON_ELF)
     phnum += 2;
@@ -248,6 +263,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
   ehdr = (void *) elf_img;
   phdr = (void *) (elf_img + sizeof (*ehdr));
   shdr = (void *) (elf_img + sizeof (*ehdr) + phnum * sizeof (*phdr));
+  footer = elf_img + program_size + header_size;
   memcpy (ehdr->e_ident, ELFMAG, SELFMAG);
   ehdr->e_ident[EI_CLASS] = ELFCLASSXX;
   if (!image_target->bigendian)
@@ -420,6 +436,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
       phdr->p_filesz = grub_host_to_target32 (XEN_NOTE_SIZE);
       phdr->p_memsz = 0;
       phdr->p_offset = grub_host_to_target32 (header_size + program_size);
+      footer = ptr;
+      footer_offset = XEN_NOTE_SIZE;
     }
 
   if (image_target->id == IMAGE_XEN_PVH)
@@ -453,6 +471,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
       phdr->p_filesz = grub_host_to_target32 (XEN_PVH_NOTE_SIZE);
       phdr->p_memsz = 0;
       phdr->p_offset = grub_host_to_target32 (header_size + program_size);
+      footer = ptr;
+      footer_offset = XEN_PVH_NOTE_SIZE;
     }
 
   if (note)
@@ -483,6 +503,33 @@ 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 = (elf_img + program_size + header_size + note_size);
+      footer_offset += note_size;
+    }
+
+  if (sbat)
+    {
+      int note_size = ALIGN_UP(sizeof (struct grub_sbat_note) + layout->sbat_size, 4);
+      struct grub_sbat_note *note_ptr = (struct grub_sbat_note *)footer;
+
+      note_ptr->header.n_namesz = grub_host_to_target32 (sizeof (GRUB_SBAT_NOTE_NAME));
+      note_ptr->header.n_descsz = grub_host_to_target32 (ALIGN_UP(layout->sbat_size, 4));
+      note_ptr->header.n_type = grub_host_to_target32 (GRUB_SBAT_NOTE_TYPE);
+      memcpy (note_ptr->name, GRUB_SBAT_NOTE_NAME, sizeof (GRUB_SBAT_NOTE_NAME));
+      memcpy ((char *)(note_ptr + 1), sbat, layout->sbat_size);
+
+      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);
+
+      footer += note_size;
+      footer_offset += note_size;
     }
 
   {
diff --git a/util/mkimage.c b/util/mkimage.c
index 8c5660825..e67f7ce73 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -1813,6 +1813,7 @@ grub_install_generate_image (const char *dir, const char *prefix,
     case IMAGE_I386_IEEE1275:
       {
 	grub_uint64_t target_addr;
+	char *sbat = NULL;
 	if (image_target->id == IMAGE_LOONGSON_ELF)
 	  {
 	    if (comp == GRUB_COMPRESSION_NONE)
@@ -1824,10 +1825,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, &core_img, &core_size,
+	  grub_mkimage_generate_elf32 (image_target, note, sbat, &core_img, &core_size,
 				       target_addr, &layout);
 	else
-	  grub_mkimage_generate_elf64 (image_target, note, &core_img, &core_size,
+	  grub_mkimage_generate_elf64 (image_target, note, sbat, &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

[PATCH v3 2/2] mkimage: adding sbat metadata into sbat ELF Note on powerpc

[Sudhakar Kuppusamy]

The SBAT metadata, which is read from .csv file and transformed into an ELF note,
is made into an image using the -s option.

Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Co-authored-by: Daniel Axtens <dja@axtens.net>
---
 util/mkimage.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/util/mkimage.c b/util/mkimage.c
index e67f7ce73..bafe190b0 100644
--- a/util/mkimage.c
+++ b/util/mkimage.c
@@ -943,8 +943,9 @@ grub_install_generate_image (const char *dir, const char *prefix,
       total_module_size += dtb_size + sizeof (struct grub_module_header);
     }
 
-  if (sbat_path != NULL && image_target->id != IMAGE_EFI)
-    grub_util_error (_(".sbat section can be embedded into EFI images only"));
+  if (sbat_path != NULL && (image_target->id != IMAGE_EFI && image_target->id != IMAGE_PPC))
+    grub_util_error (_(".sbat section can be embedded into EFI images/"
+                       "sbat ELF Note can be added into powerpc-ieee1275 images only"));
 
   if (disable_shim_lock)
     total_module_size += sizeof (struct grub_module_header);
@@ -1814,6 +1815,13 @@ grub_install_generate_image (const char *dir, const char *prefix,
       {
 	grub_uint64_t target_addr;
 	char *sbat = NULL;
+	if (sbat_path != NULL)
+	  {
+	    sbat_size = grub_util_get_image_size (sbat_path);
+	    sbat = xmalloc (sbat_size);
+	    grub_util_load_image (sbat_path, sbat);
+	    layout.sbat_size = sbat_size;
+	  }
 	if (image_target->id == IMAGE_LOONGSON_ELF)
 	  {
 	    if (comp == GRUB_COMPRESSION_NONE)
-- 
2.43.5


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

Re: [PATCH v3 1/2] mkimage: create new ELF Note for SBAT

[Daniel Kiper]

On Fri, Sep 13, 2024 at 04:57:58PM +0530, Sudhakar Kuppusamy wrote:
> In order to store the SBAT data, we create a new ELF note. The string "Secure-Boot-Advanced-Targeting",
> zero-padded to 4 byte alignment, shall be entered in the name field. The string "sbat"'s ASCII values,
> 0x41536967, should be entered in the type field.
>
> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
> Co-authored-by: Daniel Axtens <dja@axtens.net>

s/Co-authored-by/Signed-off-by/

... and AIUI Daniel's SOB should be first...

> ---
>  include/grub/util/mkimage.h |  4 +--
>  util/grub-mkimagexx.c       | 51 +++++++++++++++++++++++++++++++++++--
>  util/mkimage.c              |  5 ++--
>  3 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/include/grub/util/mkimage.h b/include/grub/util/mkimage.h
> index 3819a6744..9d74f82c5 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, char *sbat, 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, char *sbat, char **core_img, size_t *core_size,
>  			     Elf64_Addr target_addr,
>  			     struct grub_mkimage_layout *layout);
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index e50b29533..99f6ab05e 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -107,6 +107,14 @@ struct section_metadata
>    const char *strtab;
>  };
>
> +#define GRUB_SBAT_NOTE_NAME "Secure-Boot-Advanced-Targeting"

Why cannot be it in line with PE section name, i.e. ".sbat"?

> +#define GRUB_SBAT_NOTE_TYPE 0x73626174 /* "sbat" */

I think SBAT would be more appropriate here...

> +struct grub_sbat_note {
> +  Elf32_Nhdr header;
> +  char name[ALIGN_UP(sizeof(GRUB_SBAT_NOTE_NAME), 4)];
> +};
> +
>  static int
>  is_relocatable (const struct grub_install_image_target_desc *image_target)
>  {
> @@ -208,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, char *sbat, char **core_img, size_t *core_size,
>  				    Elf_Addr target_addr,
>  				    struct grub_mkimage_layout *layout)
>  {
> @@ -217,10 +225,17 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>    Elf_Ehdr *ehdr;
>    Elf_Phdr *phdr;
>    Elf_Shdr *shdr;
> -  int header_size, footer_size = 0;
> +  int header_size, footer_size = 0, footer_offset = 0;
>    int phnum = 1;
>    int shnum = 4;
>    int string_size = sizeof (".text") + sizeof ("mods") + 1;
> +  char *footer;
> +
> +  if (sbat)
> +    {
> +      phnum++;
> +      footer_size += ALIGN_UP (sizeof (struct grub_sbat_note) + layout->sbat_size, 4);
> +    }
>
>    if (image_target->id != IMAGE_LOONGSON_ELF)
>      phnum += 2;
> @@ -248,6 +263,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>    ehdr = (void *) elf_img;
>    phdr = (void *) (elf_img + sizeof (*ehdr));
>    shdr = (void *) (elf_img + sizeof (*ehdr) + phnum * sizeof (*phdr));
> +  footer = elf_img + program_size + header_size;
>    memcpy (ehdr->e_ident, ELFMAG, SELFMAG);
>    ehdr->e_ident[EI_CLASS] = ELFCLASSXX;
>    if (!image_target->bigendian)
> @@ -420,6 +436,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>        phdr->p_filesz = grub_host_to_target32 (XEN_NOTE_SIZE);
>        phdr->p_memsz = 0;
>        phdr->p_offset = grub_host_to_target32 (header_size + program_size);
> +      footer = ptr;
> +      footer_offset = XEN_NOTE_SIZE;
>      }
>
>    if (image_target->id == IMAGE_XEN_PVH)
> @@ -453,6 +471,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct grub_install_image_target_desc
>        phdr->p_filesz = grub_host_to_target32 (XEN_PVH_NOTE_SIZE);
>        phdr->p_memsz = 0;
>        phdr->p_offset = grub_host_to_target32 (header_size + program_size);
> +      footer = ptr;
> +      footer_offset = XEN_PVH_NOTE_SIZE;
>      }
>
>    if (note)
> @@ -483,6 +503,33 @@ 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 = (elf_img + program_size + header_size + note_size);
> +      footer_offset += note_size;
> +    }
> +
> +  if (sbat)
> +    {
> +      int note_size = ALIGN_UP(sizeof (struct grub_sbat_note) + layout->sbat_size, 4);

Missing space before "(". I can see similar mistakes in other places.
Please fix them too.

> +      struct grub_sbat_note *note_ptr = (struct grub_sbat_note *)footer;

Missing space after ")". And same as above, casts formatting are broken
in other places too.

> +
> +      note_ptr->header.n_namesz = grub_host_to_target32 (sizeof (GRUB_SBAT_NOTE_NAME));
> +      note_ptr->header.n_descsz = grub_host_to_target32 (ALIGN_UP(layout->sbat_size, 4));
> +      note_ptr->header.n_type = grub_host_to_target32 (GRUB_SBAT_NOTE_TYPE);
> +      memcpy (note_ptr->name, GRUB_SBAT_NOTE_NAME, sizeof (GRUB_SBAT_NOTE_NAME));
> +      memcpy ((char *)(note_ptr + 1), sbat, layout->sbat_size);
> +
> +      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);
> +
> +      footer += note_size;
> +      footer_offset += note_size;

You can drop these two lines.

Daniel

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

Re: [PATCH v3 2/2] mkimage: adding sbat metadata into sbat ELF Note on powerpc

[Daniel Kiper]

On Fri, Sep 13, 2024 at 04:57:59PM +0530, Sudhakar Kuppusamy wrote:
> The SBAT metadata, which is read from .csv file and transformed into an ELF note,
> is made into an image using the -s option.
>
> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
> Co-authored-by: Daniel Axtens <dja@axtens.net>

s/Co-authored-by/Signed-off-by/ and please reorder SOBs...

> ---
>  util/mkimage.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/util/mkimage.c b/util/mkimage.c
> index e67f7ce73..bafe190b0 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -943,8 +943,9 @@ grub_install_generate_image (const char *dir, const char *prefix,
>        total_module_size += dtb_size + sizeof (struct grub_module_header);
>      }
>
> -  if (sbat_path != NULL && image_target->id != IMAGE_EFI)
> -    grub_util_error (_(".sbat section can be embedded into EFI images only"));
> +  if (sbat_path != NULL && (image_target->id != IMAGE_EFI && image_target->id != IMAGE_PPC))
> +    grub_util_error (_(".sbat section can be embedded into EFI images/"
> +                       "sbat ELF Note can be added into powerpc-ieee1275 images only"));

I think "SBAT data can be added only to EFI or powerpc-ieee1275 images"
would be much better here.

Daniel

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

Re: [PATCH v3 2/2] mkimage: adding sbat metadata into sbat ELF Note on powerpc

[sudhakar]

On 2024-10-16 20:42, Daniel Kiper wrote:
> On Fri, Sep 13, 2024 at 04:57:59PM +0530, Sudhakar Kuppusamy wrote:
>> The SBAT metadata, which is read from .csv file and transformed into 
>> an ELF note,
>> is made into an image using the -s option.
>> 
>> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Co-authored-by: Daniel Axtens <dja@axtens.net>
> 
> s/Co-authored-by/Signed-off-by/ and please reorder SOBs...
> 
>> ---
>>  util/mkimage.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/util/mkimage.c b/util/mkimage.c
>> index e67f7ce73..bafe190b0 100644
>> --- a/util/mkimage.c
>> +++ b/util/mkimage.c
>> @@ -943,8 +943,9 @@ grub_install_generate_image (const char *dir, 
>> const char *prefix,
>>        total_module_size += dtb_size + sizeof (struct 
>> grub_module_header);
>>      }
>> 
>> -  if (sbat_path != NULL && image_target->id != IMAGE_EFI)
>> -    grub_util_error (_(".sbat section can be embedded into EFI images 
>> only"));
>> +  if (sbat_path != NULL && (image_target->id != IMAGE_EFI && 
>> image_target->id != IMAGE_PPC))
>> +    grub_util_error (_(".sbat section can be embedded into EFI 
>> images/"
>> +                       "sbat ELF Note can be added into 
>> powerpc-ieee1275 images only"));
> 
> I think "SBAT data can be added only to EFI or powerpc-ieee1275 images"
> would be much better here.
> 
> Daniel


Sure Daniel. We will make changes and repost it

Thanks,
Sudhakar Kuppusamy

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

Re: [PATCH v3 1/2] mkimage: create new ELF Note for SBAT

[sudhakar]

On 2024-10-16 20:36, Daniel Kiper wrote:
> On Fri, Sep 13, 2024 at 04:57:58PM +0530, Sudhakar Kuppusamy wrote:
>> In order to store the SBAT data, we create a new ELF note. The string 
>> "Secure-Boot-Advanced-Targeting",
>> zero-padded to 4 byte alignment, shall be entered in the name field. 
>> The string "sbat"'s ASCII values,
>> 0x41536967, should be entered in the type field.
>> 
>> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Co-authored-by: Daniel Axtens <dja@axtens.net>
> 
> s/Co-authored-by/Signed-off-by/
> 
> ... and AIUI Daniel's SOB should be first...
> 
>> ---
>>  include/grub/util/mkimage.h |  4 +--
>>  util/grub-mkimagexx.c       | 51 
>> +++++++++++++++++++++++++++++++++++--
>>  util/mkimage.c              |  5 ++--
>>  3 files changed, 54 insertions(+), 6 deletions(-)
>> 
>> diff --git a/include/grub/util/mkimage.h b/include/grub/util/mkimage.h
>> index 3819a6744..9d74f82c5 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, char *sbat, 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, char *sbat, char **core_img, size_t *core_size,
>>  			     Elf64_Addr target_addr,
>>  			     struct grub_mkimage_layout *layout);
>> 
>> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
>> index e50b29533..99f6ab05e 100644
>> --- a/util/grub-mkimagexx.c
>> +++ b/util/grub-mkimagexx.c
>> @@ -107,6 +107,14 @@ struct section_metadata
>>    const char *strtab;
>>  };
>> 
>> +#define GRUB_SBAT_NOTE_NAME "Secure-Boot-Advanced-Targeting"
> 
> Why cannot be it in line with PE section name, i.e. ".sbat"?

yes. I agree that we can use PE section name as Note Name here. However, 
I don't think it should be inline because we just defined the ELF Note 
Name here like a line number 78 ( #define GRUB_IEEE1275_NOTE_NAME 
"PowerPC").
Still do you think it should goes into inline?
> 
>> +#define GRUB_SBAT_NOTE_TYPE 0x73626174 /* "sbat" */
> 
> I think SBAT would be more appropriate here...
Yes Agree. we will make a changes
> 
>> +struct grub_sbat_note {
>> +  Elf32_Nhdr header;
>> +  char name[ALIGN_UP(sizeof(GRUB_SBAT_NOTE_NAME), 4)];
>> +};
>> +
>>  static int
>>  is_relocatable (const struct grub_install_image_target_desc 
>> *image_target)
>>  {
>> @@ -208,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, char *sbat, char **core_img, size_t *core_size,
>>  				    Elf_Addr target_addr,
>>  				    struct grub_mkimage_layout *layout)
>>  {
>> @@ -217,10 +225,17 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>    Elf_Ehdr *ehdr;
>>    Elf_Phdr *phdr;
>>    Elf_Shdr *shdr;
>> -  int header_size, footer_size = 0;
>> +  int header_size, footer_size = 0, footer_offset = 0;
>>    int phnum = 1;
>>    int shnum = 4;
>>    int string_size = sizeof (".text") + sizeof ("mods") + 1;
>> +  char *footer;
>> +
>> +  if (sbat)
>> +    {
>> +      phnum++;
>> +      footer_size += ALIGN_UP (sizeof (struct grub_sbat_note) + 
>> layout->sbat_size, 4);
>> +    }
>> 
>>    if (image_target->id != IMAGE_LOONGSON_ELF)
>>      phnum += 2;
>> @@ -248,6 +263,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>    ehdr = (void *) elf_img;
>>    phdr = (void *) (elf_img + sizeof (*ehdr));
>>    shdr = (void *) (elf_img + sizeof (*ehdr) + phnum * sizeof 
>> (*phdr));
>> +  footer = elf_img + program_size + header_size;
>>    memcpy (ehdr->e_ident, ELFMAG, SELFMAG);
>>    ehdr->e_ident[EI_CLASS] = ELFCLASSXX;
>>    if (!image_target->bigendian)
>> @@ -420,6 +436,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>        phdr->p_filesz = grub_host_to_target32 (XEN_NOTE_SIZE);
>>        phdr->p_memsz = 0;
>>        phdr->p_offset = grub_host_to_target32 (header_size + 
>> program_size);
>> +      footer = ptr;
>> +      footer_offset = XEN_NOTE_SIZE;
>>      }
>> 
>>    if (image_target->id == IMAGE_XEN_PVH)
>> @@ -453,6 +471,8 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
>> grub_install_image_target_desc
>>        phdr->p_filesz = grub_host_to_target32 (XEN_PVH_NOTE_SIZE);
>>        phdr->p_memsz = 0;
>>        phdr->p_offset = grub_host_to_target32 (header_size + 
>> program_size);
>> +      footer = ptr;
>> +      footer_offset = XEN_PVH_NOTE_SIZE;
>>      }
>> 
>>    if (note)
>> @@ -483,6 +503,33 @@ 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 = (elf_img + program_size + header_size + note_size);
>> +      footer_offset += note_size;
>> +    }
>> +
>> +  if (sbat)
>> +    {
>> +      int note_size = ALIGN_UP(sizeof (struct grub_sbat_note) + 
>> layout->sbat_size, 4);
> 
> Missing space before "(". I can see similar mistakes in other places.
> Please fix them too.
> 
>> +      struct grub_sbat_note *note_ptr = (struct grub_sbat_note 
>> *)footer;
> 
> Missing space after ")". And same as above, casts formatting are broken
> in other places too.
> 
>> +
>> +      note_ptr->header.n_namesz = grub_host_to_target32 (sizeof 
>> (GRUB_SBAT_NOTE_NAME));
>> +      note_ptr->header.n_descsz = grub_host_to_target32 
>> (ALIGN_UP(layout->sbat_size, 4));
>> +      note_ptr->header.n_type = grub_host_to_target32 
>> (GRUB_SBAT_NOTE_TYPE);
>> +      memcpy (note_ptr->name, GRUB_SBAT_NOTE_NAME, sizeof 
>> (GRUB_SBAT_NOTE_NAME));
>> +      memcpy ((char *)(note_ptr + 1), sbat, layout->sbat_size);
>> +
>> +      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);
>> +
>> +      footer += note_size;
>> +      footer_offset += note_size;
> 
> You can drop these two lines.
> 
> Daniel

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