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

Tue, 22 Oct 2024 22:07:55 -0700 

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 <sudha...@linux.ibm.com>
Co-authored-by: Daniel Axtens <d...@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

Reply via email to