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

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

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