Re: [PATCH 2/2] lvm: Add support for cachevol and integrity lv

[Daniel Kiper]

On Sun, Jun 09, 2024 at 03:35:06PM -0400, Patrick Plenefisch wrote:
> lv matching must be done after processing the ignored feature
> indirections, as integrity volumes & caches may have several levels
> of indirection that the segments must be shifted through.
>
> pv matching must be completely finished before validating a
> volume, otherwise referenced raid stripes may not have pv
> data applied yet
>
> Signed-off-by: Patrick Plenefisch 
> ---
>  grub-core/disk/diskfilter.c |  6 ++-
>  grub-core/disk/lvm.c| 85 +++--
>  2 files changed, 57 insertions(+), 34 deletions(-)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 21e239511..dc3bd943b 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -966,8 +966,6 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>
>for (lv = vg->lvs; lv; lv = lv->next)
>  {
> -  grub_err_t err;
> -
>/* RAID 1 and single-disk RAID 0 don't use a chunksize but code
>   assumes one so set one. */
>for (i = 0; i < lv->segment_count; i++)
> @@ -979,6 +977,10 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>&& lv->segments[i].stripe_size == 0)
>  lv->segments[i].stripe_size = 64;
>  }
> +}
> +  for (lv = vg->lvs; lv; lv = lv->next)
> +{
> +  grub_err_t err;
>
>err = validate_lv(lv);
>if (err)
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 10bc965a4..17e225596 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -805,13 +805,27 @@ grub_lvm_detect (grub_disk_t disk,
>seg->nodes[seg->node_count - 1].name = tmp;
>  }
>  }
> +  /* Cache and integrity LVs have extra parts that
> +   * we can ignore for our read-only access */

Please stick to the GRUB coding style [1]...

>else if (grub_memcmp (p, "cache\"",
> -   sizeof ("cache\"") - 1) == 0)
> +   sizeof ("cache\"") - 1) == 0
> +   || grub_memcmp (p, "cache+CACHE_USES_CACHEVOL\"",
> +   sizeof ("cache+CACHE_USES_CACHEVOL\"") - 1) == 0
> +   || grub_memcmp (p, "integrity\"",
> +   sizeof ("integrity\"") - 1) == 0)

I think grub_strncmp() instead of grub_memcmp() would be more natural here.

>  {
>struct ignored_feature_lv *ignored_feature = NULL;
>
>char *p2, *p3;
>grub_size_t sz;
> +#ifdef GRUB_UTIL
> +  p2 = grub_strchr (p, '"');
> +  if (p2)
> +*p2 = '\0';
> +  grub_util_info ("Ignoring extra metadata type '%s' for
> %s", p, lv->name);
> +  if (p2)
> +*p2 ='"';
> +#endif
>
>ignored_feature = grub_zalloc (sizeof (*ignored_feature));
>if (!ignored_feature)
> @@ -892,7 +906,7 @@ grub_lvm_detect (grub_disk_t disk,
>char *p2;
>p2 = grub_strchr (p, '"');
>if (p2)
> -*p2 = 0;
> +*p2 = '\0';

OK but this change should be mentioned in the commit message, i.e. you
do this on the occasion.

>grub_util_info ("unknown LVM type %s", p);
>if (p2)
>  *p2 ='"';
> @@ -936,36 +950,6 @@ grub_lvm_detect (grub_disk_t disk,
>  }
>  }
>
> -  /* Match lvs.  */
> -  {
> -struct grub_diskfilter_lv *lv1;
> -struct grub_diskfilter_lv *lv2;
> -for (lv1 = vg->lvs; lv1; lv1 = lv1->next)
> -  for (i = 0; i < lv1->segment_count; i++)
> -for (j = 0; j < lv1->segments[i].node_count; j++)
> -  {
> -if (vg->pvs)
> -  for (pv = vg->pvs; pv; pv = pv->next)
> -{
> -  if (! grub_strcmp (pv->name,
> - lv1->segments[i].nodes[j].name))
> -{
> -  lv1->segments[i].nodes[j].pv = pv;
> -  break;
> -}
> -}
> -if (lv1->segments[i].nodes[j].pv == NULL)
> -  for (lv2 = vg->lvs; lv2; lv2 = lv2->next)
> -{
> -  if (lv1 == lv2)
> -continue;
> -  if (grub_strcmp (lv2->name,
> -   lv1->segments[i].nodes[j].name) == 0)
> -lv1->segments[i].nodes[j].lv = lv2;
> -}
> -  }
> -
> -  }
>
>{
>  struct ignored_feature_lv *ignored_feature;
> @@ -1014,9 +998,46 @@ grub_lvm_detect (grub_disk_t disk,
>  ignored_feature->lv = NULL;
>}
>}
> +  else
> +  {
> +

Please drop this empty line.

May I ask you to send patches as a reply to a cover letter?
If you use "git send-email" it will do it for you.

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

___
Grub-devel mailing list

Re: [PATCH 1/2] disk/lvm: Make cache_lv more generic as ignored_feature_lv

[Daniel Kiper]

On Sun, Jun 09, 2024 at 03:34:58PM -0400, Patrick Plenefisch wrote:
> This patch isn't necessary by itself, but when combined with the next
> patch it enhances readability as ignored_features_lv is then used for
> multiple types of extra LV's, not just cache LV's
>
> Signed-off-by: Patrick Plenefisch 
> ---
>  grub-core/disk/lvm.c | 179 +++
>  1 file changed, 77 insertions(+), 102 deletions(-)
>
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 794248540..10bc965a4 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -34,12 +34,11 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> -struct cache_lv
> +struct ignored_feature_lv
>  {
>struct grub_diskfilter_lv *lv;
> -  char *cache_pool;

I think this change should go to separate patch as well.
And of course the new patch should have good commit message...

>char *origin;
> -  struct cache_lv *next;
> +  struct ignored_feature_lv *next;
>  };

Daniel

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

Re: [PATCH v4 00/10] UEFI NX support and NX Linux loader using shim loader protocol

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:03PM +0100, Mate Kukri wrote:
> Currently the patchset consists of:
> - Reworked Fedora NX patches to make GRUB itself work under NX.
> - Julian Andres Klode's loader framework patch (used in Debian and Ubuntu for
> the downstream loader).
> - Implemented shim loader protocol support using the above loader framework.
> - Added patch to disallow using the legacy Linux loader when NX is required.

When I apply the patches git complains in the following way:

  Applying: modules: make .module_license read-only
  Applying: modules: strip .llvm_addrsig sections and similar.
  Applying: modules: Don't allocate space for non-allocable sections.
  Applying: modules: load module sections at page-aligned addresses
  Applying: nx: add memory attribute get/set API
  Applying: nx: set page permissions for loaded modules.
  .git/rebase-apply/patch:137: space before tab in indent.
  )
  warning: 1 line adds whitespace errors.
  Applying: nx: set the nx compatible flag in EFI grub images
  Applying: efi: Provide wrappers for load_image, start_image, unload_image
  .git/rebase-apply/patch:174: trailing whitespace.
  grub_efi_status_t
  warning: 1 line adds whitespace errors.
  Applying: efi: Use shim's loader protocol for EFI image verification and 
loading
  Applying: efi: Disallow fallback to legacy Linux loader when shim says NX is 
required.

Daniel

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

Re: [PATCH v4 07/10] nx: set the nx compatible flag in EFI grub images

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:10PM +0100, Mate Kukri wrote:
> For NX, we need the grub binary to announce that it is compatible with

s/grub/GRUB/

> the NX feature.  This implies that when loading the executable grub

Ditto. May I ask you to use correct project name?

> image, several attributes are true:
>
> - the binary doesn't need an executable stack
> - the binary doesn't need sections to be both executable and writable
> - the binary knows how to use the EFI Memory Attributes protocol on code
>   it is loading.
>
> This patch
> - adds a definition for the PE DLL Characteristics flag GRUB_PE32_NX_COMPAT
> - changes grub-mkimage to set that flag.
>
> Original-Author: Peter Jones 
> Signed-off-by: Mate Kukri 

If you fix nits mentioned above then you can add
Reviewed-by: Daniel Kiper ...

Daniel

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

Re: [PATCH v4 06/10] nx: set page permissions for loaded modules.

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:09PM +0100, Mate Kukri wrote:
> For NX, we need to set write and executable permissions on the sections
> of grub modules when we load them.
>
> On sections with SHF_ALLOC set, which is typically everything except
> .modname and the symbol and string tables, this patch clears the Read
> Only flag on sections that have the ELF flag SHF_WRITE set, and clears
> the No eXecute flag on sections with SHF_EXECINSTR set.  In all other
> cases it sets both flags.
>
> Original-Author: Peter Jones 
> Original-Author: Robbie Harwood 
> Original-Author: Laszlo Ersek 
> Signed-off-by: Mate Kukri 
> ---
>  grub-core/kern/dl.c | 104 ++--
>  include/grub/dl.h   |  46 
>  2 files changed, 137 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 8338f7436..3341d78d6 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -616,25 +616,97 @@ grub_dl_relocate_symbols (grub_dl_t mod, void *ehdr)
>   grub_dl_segment_t seg;
>   grub_err_t err;
>
> - /* Find the target segment.  */
> - for (seg = mod->segment; seg; seg = seg->next)
> -   if (seg->section == s->sh_info)
> - break;
> + seg = grub_dl_find_segment(mod, s->sh_info);
> +if (!seg)
> +   continue;

This hunk is suspicious...

> - if (seg)
> -   {
> - if (!mod->symtab)
> -   return grub_error (GRUB_ERR_BAD_MODULE, "relocation without 
> symbol table");
> + if (!mod->symtab)
> +   return grub_error (GRUB_ERR_BAD_MODULE, "relocation without symbol 
> table");
>
> - err = grub_arch_dl_relocate_symbols (mod, ehdr, s, seg);
> - if (err)
> -   return err;
> -   }
> + err = grub_arch_dl_relocate_symbols (mod, ehdr, s, seg);
> + if (err)
> +   return err;
>}
>
>return GRUB_ERR_NONE;
>  }
>
> +/* Only define this on EFI to save space in core */
> +#ifdef GRUB_MACHINE_EFI
> +static grub_err_t
> +grub_dl_set_mem_attrs (grub_dl_t mod, void *ehdr)
> +{
> +  unsigned i;

s/unsigned/unsigned int/

> +  const Elf_Shdr *s;
> +  const Elf_Ehdr *e = ehdr;
> +  grub_err_t err;
> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> +  grub_size_t arch_addralign = grub_arch_dl_min_alignment ();
> +  grub_addr_t tgaddr;
> +  grub_size_t tgsz;
> +#endif
> +
> +  for (i = 0, s = (const Elf_Shdr *)((const char *) e + e->e_shoff);
> +   i < e->e_shnum;
> +   i++, s = (const Elf_Shdr *)((const char *) s + e->e_shentsize))
> +{
> +  grub_dl_segment_t seg;
> +  grub_uint64_t set_attrs = GRUB_MEM_ATTR_R;
> +  grub_uint64_t clear_attrs = GRUB_MEM_ATTR_W|GRUB_MEM_ATTR_X;
> +
> +  seg = grub_dl_find_segment(mod, i);
> +  if (!seg)
> + continue;
> +
> +  if (seg->size == 0 || !(s->sh_flags & SHF_ALLOC))
> + continue;
> +
> +  if (s->sh_flags & SHF_WRITE)
> + {
> +   set_attrs |= GRUB_MEM_ATTR_W;
> +   clear_attrs &= ~GRUB_MEM_ATTR_W;
> + }
> +
> +  if (s->sh_flags & SHF_EXECINSTR)
> + {
> +   set_attrs |= GRUB_MEM_ATTR_X;
> +   clear_attrs &= ~GRUB_MEM_ATTR_X;
> + }
> +
> +  err = grub_update_mem_attrs ((grub_addr_t)(seg->addr), seg->size,
> +set_attrs, clear_attrs);
> +  if (err != GRUB_ERR_NONE)
> + return err;
> +}
> +
> +#if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv)
> +  tgaddr = grub_min((grub_addr_t)mod->tramp, (grub_addr_t)mod->got);
> +  tgsz = grub_max((grub_addr_t)mod->trampptr, (grub_addr_t)mod->gotptr) - 
> tgaddr;

Wrong coding style for function calls and casts.

> +  if (tgsz)
> +{
> +  tgsz = ALIGN_UP(tgsz, arch_addralign);
> +
> +  if (tgaddr < (grub_addr_t)mod->base ||
> +  tgsz > (grub_addr_t)-1 - tgaddr ||
> +   tgaddr + tgsz > (grub_addr_t)mod->base + mod->sz)

Wrong casts coding style...

> + return grub_error (GRUB_ERR_BUG,
> +"BUG: trying to protect pages outside of module "
> +"allocation (\"%s\"): module base %p, size 0x%"
> +PRIxGRUB_SIZE "; tramp/GOT base 0x%" PRIxGRUB_ADDR
> +", size 0x%" PRIxGRUB_SIZE,
> +mod->name, mod->base, mod->sz, tgaddr, tgsz);
> +  err = grub_update_mem_attrs (tgaddr, tgsz, 
> GRUB_MEM_ATTR_R|GRUB_MEM_ATTR_X,
> +GRUB_MEM_ATTR_W);

Missing space before and after "|" (I saw similar mistakes in the other
places too). And you do not need to wrap this line.

> +  if (err != GRUB_ERR_NONE)
> + return err;
> +}
> +#endif
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
> +
>  /* Load a module from core memory.  */
>  grub_dl_t
>  grub_dl_load_core_noinit (void *addr, grub_size_t size)
> @@ -668,6 +740,7 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>mod->ref_count = 1;
>
>grub_dprintf ("modules", "relocating 

Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2

[Daniel Kiper]

On Tue, Jun 25, 2024 at 02:42:31PM +0800, Gary Lin wrote:
> On Mon, Jun 24, 2024 at 07:28:14PM +0200, Daniel Kiper wrote:
> > On Thu, Mar 07, 2024 at 04:59:05PM +0800, Gary Lin via Grub-devel wrote:
> > > On Thu, Feb 08, 2024 at 08:58:43PM +0100, Daniel Kiper wrote:
> > > > Hey,
> > > >
> > > --8<--
> > > >
> > > > And I have attached the Coverity report. All issues reported there have
> > > > to be fixed. If you cannot fix an issue you have to explain why you
> > > > cannot do that and what is potential impact on the code stability,
> > > > security, etc.
> > > >
> > > I have went through all the coverity issues. There are 6 issues in the
> > > TPM2 stack and the utility:
> > >
> > > CID 435775, CID 435771, CID 435770, CID 435769, CID 435767, CID 435761
> > >
> > > Those will be fixed in the next version.
> > >
> > > The 9 issues are from libtasn1 and gnulib.
> > >
> > > There are two memory corruption issue: CID 435762 and CID 435766, and
> > > I've filed the issues in libtasn1 upstream.
> > >
> > > - CID 435762
> > >   https://gitlab.com/gnutls/libtasn1/-/issues/49
> > >   This is a valid case. However, the only exploitable function,
> > >   _asn1_insert_tag_der(), is disabled in grub2 patch, so the severity is
> > >   low. I have a quick fix but upstream would like to fix it in another
> > >   way.
> > > - CID 435766
> > >   https://gitlab.com/gnutls/libtasn1/-/issues/50
> > >   IMO, this is false positive, but I need libtasn1 upstream to confirm
> > >   that.
> > >
> > > Then, the remaining 7 Integer handling issues are involved with the macros
> > > from gnulib, and I believe those are false positive.
> > >
> > > The following are my analyses:
> > >
> > > 
> > > *** CID 435774:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> > > /grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
> > > 475*/
> > > 476   if (leading != 0 && der[len_len + k] == 0x80)
> > > 477   return ASN1_DER_ERROR;
> > > 478   leading = 0;
> > > 479
> > > 480   /* check for wrap around */
> > > >>> CID 435774:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> > > >>> "val < 1 ? 0 : val) - 1 < 0) ? ~1 ? 0 : val) + 1 << 62UL 
> > > >>> /* sizeof (+val) * 8 - 2 */) - 1) * 2 + 1) : ((1 ? 0 : val) + 0)) >> 
> > > >>> 7)" is always false regardless of the values of its operands. This 
> > > >>> occurs as the second operand of "?:".
> > > 481   if (INT_LEFT_SHIFT_OVERFLOW (val, 7))
> > > 482   return ASN1_DER_ERROR;
> > > 483
> > > 484   val = val << 7;
> > > 485   val |= der[len_len + k] & 0x7F;
> > > 486
> > >
> > > Here are the related macros:
> > >
> > > #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
> > >
> > > #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
> > >
> > > #define _GL_SIGNED_INT_MAXIMUM(e)   \
> > >   (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
> > >
> > > #define _GL_INT_MINIMUM(e)  \
> > >   (EXPR_SIGNED (e)  \
> > >? ~ _GL_SIGNED_INT_MAXIMUM (e)   \
> > >: _GL_INT_CONVERT (e, 0))
> > >
> > > #define INT_LEFT_SHIFT_OVERFLOW(a, b) \
> > >   INT_LEFT_SHIFT_RANGE_OVERFLOW (a, b, \
> > >  _GL_INT_MINIMUM (a), _GL_INT_MAXIMUM (a))
> > >
> > > #define INT_LEFT_SHIFT_RANGE_OVERFLOW(a, b, min, max)   \
> > >   ((a) < 0  \
> > >? (a) < (min) >> (b) \
> > >: (max) >> (b) < (a))
> > >
> > > The statement in question is expanded "(a) < (min) >> (b)" of
> > > INT_LEFT_SHIFT_RANGE_OVERFLOW. Since 'val' is 'uint64_t', '(a) < 0' is 
> > > always
> > > false, so the result of that statement doen't matter.
> >
> > Something is mis

Re: [PATCH v9 00/22] Automatic Disk Unlock with TPM2

[Daniel Kiper]

On Thu, Mar 07, 2024 at 04:59:05PM +0800, Gary Lin via Grub-devel wrote:
> On Thu, Feb 08, 2024 at 08:58:43PM +0100, Daniel Kiper wrote:
> > Hey,
> >
> --8<--
> >
> > And I have attached the Coverity report. All issues reported there have
> > to be fixed. If you cannot fix an issue you have to explain why you
> > cannot do that and what is potential impact on the code stability,
> > security, etc.
> >
> I have went through all the coverity issues. There are 6 issues in the
> TPM2 stack and the utility:
>
> CID 435775, CID 435771, CID 435770, CID 435769, CID 435767, CID 435761
>
> Those will be fixed in the next version.
>
> The 9 issues are from libtasn1 and gnulib.
>
> There are two memory corruption issue: CID 435762 and CID 435766, and
> I've filed the issues in libtasn1 upstream.
>
> - CID 435762
>   https://gitlab.com/gnutls/libtasn1/-/issues/49
>   This is a valid case. However, the only exploitable function,
>   _asn1_insert_tag_der(), is disabled in grub2 patch, so the severity is
>   low. I have a quick fix but upstream would like to fix it in another
>   way.
> - CID 435766
>   https://gitlab.com/gnutls/libtasn1/-/issues/50
>   IMO, this is false positive, but I need libtasn1 upstream to confirm
>   that.
>
> Then, the remaining 7 Integer handling issues are involved with the macros
> from gnulib, and I believe those are false positive.
>
> The following are my analyses:
>
> 
> *** CID 435774:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> /grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
> 475*/
> 476   if (leading != 0 && der[len_len + k] == 0x80)
> 477   return ASN1_DER_ERROR;
> 478   leading = 0;
> 479
> 480   /* check for wrap around */
> >>> CID 435774:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
> >>> "val < 1 ? 0 : val) - 1 < 0) ? ~1 ? 0 : val) + 1 << 62UL /* 
> >>> sizeof (+val) * 8 - 2 */) - 1) * 2 + 1) : ((1 ? 0 : val) + 0)) >> 7)" is 
> >>> always false regardless of the values of its operands. This occurs as the 
> >>> second operand of "?:".
> 481   if (INT_LEFT_SHIFT_OVERFLOW (val, 7))
> 482   return ASN1_DER_ERROR;
> 483
> 484   val = val << 7;
> 485   val |= der[len_len + k] & 0x7F;
> 486
>
> Here are the related macros:
>
> #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
>
> #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
>
> #define _GL_SIGNED_INT_MAXIMUM(e)   \
>   (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
>
> #define _GL_INT_MINIMUM(e)  \
>   (EXPR_SIGNED (e)  \
>? ~ _GL_SIGNED_INT_MAXIMUM (e)   \
>: _GL_INT_CONVERT (e, 0))
>
> #define INT_LEFT_SHIFT_OVERFLOW(a, b) \
>   INT_LEFT_SHIFT_RANGE_OVERFLOW (a, b, \
>  _GL_INT_MINIMUM (a), _GL_INT_MAXIMUM (a))
>
> #define INT_LEFT_SHIFT_RANGE_OVERFLOW(a, b, min, max)   \
>   ((a) < 0  \
>? (a) < (min) >> (b) \
>: (max) >> (b) < (a))
>
> The statement in question is expanded "(a) < (min) >> (b)" of
> INT_LEFT_SHIFT_RANGE_OVERFLOW. Since 'val' is 'uint64_t', '(a) < 0' is always
> false, so the result of that statement doen't matter.

Something is missing and/or requires clarification/expansion here.
AFAICT at least your description completely ignores "(max) >> (b) < (a)"
expression result.

> 
> *** CID 435773:  Integer handling issues  (NO_EFFECT)
> /grub-core/lib/libtasn1/lib/decoding.c: 439 in asn1_get_object_id_der()
> 433 return ASN1_DER_ERROR;
> 434
> 435   val0 = 0;
> 436
> 437   for (k = 0; k < len; k++)
> 438 {
> >>> CID 435773:  Integer handling issues  (NO_EFFECT)
> >>> This less-than-zero comparison of an unsigned value is never true. 
> >>> "(1 ? 0UL : val0) - 1UL < 0UL".
> 439   if (INT_LEFT_SHIFT_OVERFLOW (val0, 7))
> 440   return ASN1_DER_ERROR;
> 441
> 442   val0 <<= 7;
> 443   val0 |= der[len_len + k] & 0x7F;
> 444   if (!(der

Re: [PATCH v4 05/10] nx: add memory attribute get/set API

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:08PM +0100, Mate Kukri wrote:
> For NX, we need to set the page access permission attributes for write
> and execute permissions.
>
> This patch adds two new primitives, grub_set_mem_attrs() and
> grub_clear_mem_attrs(), and associated constant definitions, to be used
> for that purpose.
>
> For most platforms, it adds a dummy implementation that returns
> GRUB_ERR_NONE.
>
> On EFI platforms, it implements the primitives using the EFI
> Memory Attribute Protocol (defined in UEFI 2.10 specification).
>
> Original-Author: Peter Jones 

Why not Signed-off-by instead?

> Signed-off-by: Mate Kukri 
> ---
>  grub-core/kern/efi/mm.c | 127 
>  include/grub/efi/api.h  |  25 
>  include/grub/mm.h   |  33 +++
>  3 files changed, 185 insertions(+)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 6a6fba891..9af851e8f 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -687,3 +687,130 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
>return GRUB_ERR_NONE;
>  }
>  #endif
> +
> +static inline grub_uint64_t

You can drop inline from here.

> +grub_mem_attrs_to_uefi_mem_attrs (grub_uint64_t attrs)
> +{
> +  grub_uint64_t ret = GRUB_EFI_MEMORY_RP |
> +   GRUB_EFI_MEMORY_RO |
> +   GRUB_EFI_MEMORY_XP;

I would put this into one line.

> +  if (attrs & GRUB_MEM_ATTR_R)
> +ret &= ~GRUB_EFI_MEMORY_RP;
> +
> +  if (attrs & GRUB_MEM_ATTR_W)
> +ret &= ~GRUB_EFI_MEMORY_RO;
> +
> +  if (attrs & GRUB_MEM_ATTR_X)
> +ret &= ~GRUB_EFI_MEMORY_XP;

All GRUB_EFI_MEMORY_* constants are not defined in this patch. I think
they should be...

> +
> +  return ret;
> +}
> +
> +static inline grub_uint64_t

You can drop inline from here.

> +uefi_mem_attrs_to_grub_mem_attrs (grub_uint64_t attrs)
> +{
> +  grub_uint64_t ret = GRUB_MEM_ATTR_R |
> +   GRUB_MEM_ATTR_W |
> +   GRUB_MEM_ATTR_X;

Again, you do need to wrap this line...

> +  if (attrs & GRUB_EFI_MEMORY_RP)
> +ret &= ~GRUB_MEM_ATTR_R;
> +
> +  if (attrs & GRUB_EFI_MEMORY_RO)
> +ret &= ~GRUB_MEM_ATTR_W;
> +
> +  if (attrs & GRUB_EFI_MEMORY_XP)
> +ret &= ~GRUB_MEM_ATTR_X;
> +
> +  return ret;
> +}
> +
> +grub_err_t
> +grub_get_mem_attrs (grub_addr_t addr, grub_size_t size, grub_uint64_t *attrs)
> +{
> +  grub_efi_memory_attribute_protocol_t *proto;
> +  grub_efi_physical_address_t physaddr = addr;
> +  grub_guid_t protocol_guid = GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;

Please add "static" before grub_guid_t initializations.

> +  grub_efi_status_t efi_status;
> +
> +  if (physaddr & 0xfff || size & 0xfff || size == 0 || attrs == NULL)

Should not 0xfff be defined as a mask (constant)?

> +{
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +  N_("grub_get_mem_attrs() called with invalid 
> arguments"));
> +}

Please drop redundant curly braces.

> +
> +  proto = grub_efi_locate_protocol (_guid, 0);
> +  if (!proto)
> +{
> +  /* No protocol -> do nothing, all memory is RWX in boot services */
> +  *attrs = GRUB_MEM_ATTR_R | GRUB_MEM_ATTR_W | GRUB_MEM_ATTR_X;
> +  return GRUB_ERR_NONE;
> +}
> +
> +  efi_status = proto->get_memory_attributes(proto, physaddr, size, attrs);
> +  if (efi_status != GRUB_EFI_SUCCESS)
> +{
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +  N_("grub_get_mem_attrs() called with invalid 
> arguments"));
> +}

Ditto and in the other places too...

> +
> +  *attrs = uefi_mem_attrs_to_grub_mem_attrs (*attrs);
> +
> +  grub_dprintf ("nx", "get 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR":%c%c%c\n",

Missing spaces before and after PRIxGRUB_ADDR.

> + addr, addr + size - 1,
> + (*attrs & GRUB_MEM_ATTR_R) ? 'r' : '-',
> + (*attrs & GRUB_MEM_ATTR_W) ? 'w' : '-',
> + (*attrs & GRUB_MEM_ATTR_X) ? 'x' : '-');
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_update_mem_attrs (grub_addr_t addr, grub_size_t size,
> +grub_uint64_t set_attrs, grub_uint64_t clear_attrs)
> +{
> +  grub_efi_memory_attribute_protocol_t *proto;
> +  grub_efi_physical_address_t physaddr = addr;
> +  grub_guid_t protocol_guid = GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;

Again, please add "static" to grub_guid_t initializations.

> +  grub_efi_status_t efi_status = GRUB_EFI_SUCCESS;
> +  grub_uint64_t uefi_set_attrs, uefi_clear_attrs;

s/grub_uint64_t/grub_efi_uint64_t/

> +
> +

Please drop redundant empty line.

> +  if (physaddr & 0xfff || size & 0xfff || size == 0)

Certainly 0xfff deserves being a constant...

> +{
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +  N_("grub_update_mem_attrs() called with invalid 
> arguments"));
> +}
> +
> +  proto = grub_efi_locate_protocol (_guid, 0);
> +  if (!proto)

Please use "if (proto == NULL)" in all cases like that one.

> +{
> +  /* No 

Re: [PATCH v4 04/10] modules: load module sections at page-aligned addresses

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:07PM +0100, Mate Kukri wrote:
> Currently we load module sections at whatever alignment gcc+ld happened
> to dump into the ELF section header, which is often less then the page
> size. Since NX protections are page based, this alignment must be
> rounded up to page size on platforms supporting NX protections.
>
> This patch switches most EFI platforms to load module sections at 4kB
> page-aligned addresses.  To do so, it adds an new per-arch function,
> grub_arch_dl_min_alignment(), which returns the alignment needed for
> dynamically loaded sections (in bytes).  Currently it sets it to 4096
> when GRUB_MACHINE_EFI is true on x86_64, i386, arm, arm64, and emu, and
> 1-byte alignment on everything else.
>
> It then changes the allocation size computation and the loader code in
> grub_dl_load_segments() to align the locations and sizes up to these
> boundaries, and fills any added padding with zeros.
>
> All of this happens before relocations are applied, so the relocations
> factor that in with no change.
>
> Original-Author: Peter Jones 
> Original-Author: Laszlo Ersek 
> Signed-off-by: Mate Kukri 
> ---
>  docs/grub-dev.texi  |  6 ++---
>  grub-core/kern/arm/dl.c | 13 +
>  grub-core/kern/arm64/dl.c   | 13 +
>  grub-core/kern/dl.c | 53 ++---
>  grub-core/kern/emu/full.c   | 13 +
>  grub-core/kern/i386/dl.c| 13 +
>  grub-core/kern/ia64/dl.c|  9 +++
>  grub-core/kern/mips/dl.c|  8 ++
>  grub-core/kern/powerpc/dl.c |  9 +++
>  grub-core/kern/riscv/dl.c   | 13 +
>  grub-core/kern/sparc64/dl.c |  9 +++
>  grub-core/kern/x86_64/dl.c  | 13 +
>  include/grub/dl.h   |  2 ++
>  13 files changed, 155 insertions(+), 19 deletions(-)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index 1276c5930..2f782cda5 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -996,9 +996,9 @@ declare startup asm file ($cpu_$platform_startup) as well 
> as any other files
>  (e.g. init.c and callwrap.S) (e.g. $cpu_$platform = 
> kern/$cpu/$platform/init.c).
>  At this stage you will also need to add dummy dl.c and cache.S with functions
>  grub_err_t grub_arch_dl_check_header (void *ehdr), grub_err_t
> -grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr) (dl.c) and
> -void grub_arch_sync_caches (void *address, grub_size_t len) (cache.S). They
> -won't be used for now.
> +grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr) (dl.c), 
> grub_uint32_t
> +grub_arch_dl_min_alignment (void), and void grub_arch_sync_caches (void
> +*address, grub_size_t len) (cache.S). They won't be used for now.
>
>  You will need to create directory include/$cpu/$platform and a file
>  include/$cpu/types.h. The latter following this template:
> diff --git a/grub-core/kern/arm/dl.c b/grub-core/kern/arm/dl.c
> index eab9d17ff..926073793 100644
> --- a/grub-core/kern/arm/dl.c
> +++ b/grub-core/kern/arm/dl.c
> @@ -278,3 +278,16 @@ grub_arch_dl_check_header (void *ehdr)
>
>return GRUB_ERR_NONE;
>  }
> +
> +/*
> + * Tell the loader what our minimum section alignment is.
> + */
> +grub_size_t
> +grub_arch_dl_min_alignment (void)
> +{
> +#ifdef GRUB_MACHINE_EFI
> +  return 4096;
> +#else
> +  return 1;
> +#endif
> +}

I would define this as a constant in the include/grub/efi/memory.h.
OK, we have to have definition for non-EFI case somewhere as well.

> diff --git a/grub-core/kern/arm64/dl.c b/grub-core/kern/arm64/dl.c
> index a2b5789a9..95c6d5bf4 100644
> --- a/grub-core/kern/arm64/dl.c
> +++ b/grub-core/kern/arm64/dl.c
> @@ -196,3 +196,16 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
>
>return GRUB_ERR_NONE;
>  }
> +
> +/*
> + * Tell the loader what our minimum section alignment is.
> + */
> +grub_size_t
> +grub_arch_dl_min_alignment (void)
> +{
> +#ifdef GRUB_MACHINE_EFI
> +  return 4096;
> +#else
> +  return 1;
> +#endif
> +}

Ditto and below as well please...

> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 37db9fab0..8338f7436 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -224,25 +224,35 @@ grub_dl_load_segments (grub_dl_t mod, const Elf_Ehdr *e)
>  {
>unsigned i;
>const Elf_Shdr *s;
> -  grub_size_t tsize = 0, talign = 1;
> +  grub_size_t tsize = 0, talign = 1, arch_addralign = 1;
>  #if !defined (__i386__) && !defined (__x86_64__) && !defined(__riscv) && \
>!defined (__loongarch__)
>grub_size_t tramp;
> +  grub_size_t tramp_align;
>grub_size_t got;
> +  grub_size_t got_align;
>grub_err_t err;
>  #endif
>char *ptr;
>
> +  arch_addralign = grub_arch_dl_min_alignment ();
> +
>for (i = 0, s = (const Elf_Shdr *)((const char *) e + e->e_shoff);
> i < e->e_shnum;
> i++, s = (const Elf_Shdr *)((const char *) s + e->e_shentsize))
>  {
> +  grub_size_t sh_addralign;
> +  grub_size_t sh_size;
> +
>if (s->sh_size == 0 || !(s->sh_flags & 

Re: [PATCH v4 03/10] modules: Don't allocate space for non-allocable sections.

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:06PM +0100, Mate Kukri wrote:
> From: Peter Jones 
>
> Currently when loading grub modules, we allocate space for all sections,
> including those without SHF_ALLOC set.  We then copy the sections that
> /do/ have SHF_ALLOC set into the allocated memory, leaving some of our
> allocation untouched forever.  Additionally, on platforms with GOT
> fixups and trampolines, we currently compute alignment round-ups for the
> sections and sections with sh_size = 0.
>
> This patch removes the extra space from the allocation computation, and
> makes the allocation computation loop skip empty sections as the loading
> loop does.
>
> Signed-off-by: Peter Jones 
> Signed-off-by: Jan Setje-Eilers 
> Signed-off-by: Mate Kukri 
> Reviewed-By: Vladimir Serbinenko

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v4 02/10] modules: strip .llvm_addrsig sections and similar.

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:05PM +0100, Mate Kukri wrote:
> From: Peter Jones 
>
> Currently grub modules built with clang or gcc have several sections
> which we don't actually need or support.
>
> We already have a list of section to skip in genmod.sh, and this patch
> adds the following sections to that list (as well as a few newlines):
>
> .note.gnu.property
> .llvm*
>
> Note that the glob there won't work without a new enough linker, but the
> failure is just reversion to the status quo, so that's not a big problem.
>
> Signed-off-by: Peter Jones 
> Signed-off-by: Jan Setje-Eilers 
> Signed-off-by: Mate Kukri 
> Reviewed-By: Vladimir Serbinenko

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v4 01/10] modules: make .module_license read-only

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:57:04PM +0100, Mate Kukri wrote:
> From: Peter Jones 
>
> Currently .module_license is set writable (that is, the section has the
> SHF_WRITE flag set) in the module's ELF headers.  This probably never
> actually matters, but it can't possibly be correct.
>
> This patch sets that data as "const", which causes that flag not to be
> set.
>
> Signed-off-by: Peter Jones 
> Signed-off-by: Jan Setje-Eilers 
> Signed-off-by: Mate Kukri 
> Reviewed-By: Vladimir Serbinenko

Reviewed-by: Daniel Kiper 

Daniel

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

[ANNOUNCEMENT] Linux Plumbers Conference - Systems Boot and Security Microconference

[Daniel Kiper via Grub-devel]

Hi all,

I have great pleasure of informing you that the call for proposals is open for
Systems Boot and Security Microconference [1] which is a part of Linux Plumbers
Conference [2]. The conference will be held in Vienna, Austria between 18th and
20th of September.

If you want to discuss with us your project, idea, etc. please post your
proposal at CfP page [3] and do not forget to set Track to "Systems Boot and
Security MC". The CfP closes on July 14th.

If you have any questions, please drop me a line.

Daniel

[1] https://lpc.events/event/18/contributions/1676/
[2] https://lpc.events/
[3] https://lpc.events/event/18/abstracts/

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

Re: [PATCH v17 11/20] key_protector: Add TPM2 Key Protector

[Daniel Kiper via Grub-devel]

On Thu, Jun 20, 2024 at 03:35:32PM +0800, Gary Lin wrote:
> On Wed, Jun 19, 2024 at 06:34:13PM +0200, Daniel Kiper wrote:
> > On Fri, Jun 14, 2024 at 02:45:44PM +0800, Gary Lin wrote:
> > > From: Hernan Gatta 
> > >
> > > The TPM2 key protector is a module that enables the automatic retrieval
> > > of a fully-encrypted disk's unlocking key from a TPM 2.0.
> > >
> > > The theory of operation is such that the module accepts various
> > > arguments, most of which are optional and therefore possess reasonable
> > > defaults. One of these arguments is the keyfile/tpm2key parameter, which
> > > is mandatory. There are two supported key formats:
> > >
> > > 1. Raw Sealed Key (--keyfile)
> > >When sealing a key with TPM2_Create, the public portion of the sealed
> > >key is stored in TPM2B_PUBLIC, and the private portion is in
> > >TPM2B_PRIVATE. The raw sealed key glues the fully marshalled
> > >TPM2B_PUBLIC and TPM2B_PRIVATE into one file.
> > >
> > > 2. TPM 2.0 Key (--tpm2key)
> > >The following is the ASN.1 definition of TPM 2.0 Key File:
> > >
> > >TPMPolicy ::= SEQUENCE {
> > >  CommandCode   [0] EXPLICIT INTEGER
> > >  CommandPolicy [1] EXPLICIT OCTET STRING
> > >}
> > >
> > >TPMAuthPolicy ::= SEQUENCE {
> > >  Name[0] EXPLICIT UTF8STRING OPTIONAL
> > >  Policy  [1] EXPLICIT SEQUENCE OF TPMPolicy
> > >}
> > >
> > >TPMKey ::= SEQUENCE {
> > >  typeOBJECT IDENTIFIER
> > >  emptyAuth   [0] EXPLICIT BOOLEAN OPTIONAL
> > >  policy  [1] EXPLICIT SEQUENCE OF TPMPolicy OPTIONAL
> > >  secret  [2] EXPLICIT OCTET STRING OPTIONAL
> > >  authPolicy  [3] EXPLICIT SEQUENCE OF TPMAuthPolicy OPTIONAL
> > >  description [4] EXPLICIT UTF8String OPTIONAL,
> > >  rsaParent   [5] EXPLICIT BOOLEAN OPTIONAL,
> > >  parent  INTEGER
> > >  pubkey  OCTET STRING
> > >  privkey OCTET STRING
> > >}
> > >
> > >   The TPM2 key protector only expects a "sealed" key in DER encoding,
> > >   so 'type' is always 2.23.133.10.1.5, 'emptyAuth' is 'TRUE', and
> > >   'secret' is empty. 'policy' and 'authPolicy' are the possible policy
> > >   command sequences to construst the policy digest to unseal the key.
> > >   Similar to the raw sealed key, the public portion (TPM2B_PUBLIC) of
> > >   the sealed key is stored in 'pubkey', and the private portion
> > >   (TPM2B_PRIVATE) is in 'privkey'.
> > >
> > >   For more details: 
> > > https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html
> > >
> > > This sealed key file is created via the grub-protect tool. The tool
> > > utilizes the TPM's sealing functionality to seal (i.e., encrypt) an
> > > unlocking key using a Storage Root Key (SRK) to the values of various
> > > Platform Configuration Registers (PCRs). These PCRs reflect the state
> > > of the system as it boots. If the values are as expected, the system
> > > may be considered trustworthy, at which point the TPM allows for a
> > > caller to utilize the private component of the SRK to unseal (i.e.,
> > > decrypt) the sealed key file. The caller, in this case, is this key
> > > protector.
> > >
> > > The TPM2 key protector registers two commands:
> > >
> > > - tpm2_key_protector_init: Initializes the state of the TPM2 key
> > >protector for later usage, clearing any
> > >previous state, too, if any.
> > >
> > > - tpm2_key_protector_clear: Clears any state set by 
> > > tpm2_key_protector_init.
> > >
> > > The way this is expected to be used requires the user to, either
> > > interactively or, normally, via a boot script, initialize/configure
> > > the key protector and then specify that it be used by the 'cryptomount'
> > > command (modifications to this command are in a different patch).
> > >
> > > For instance, to unseal the raw sealed key file:
> > >
> > > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-1.key
> > > cryptomount -u  -P tpm2
> > >
> > > tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-2.key 
> > > --pcrs=7,11
> > > cryptomount -u  -P tpm2
> > >
> > > Or, to unseal the TPM 2.0 Key file:
> > >
> >

Re: [PATCH v17 10/20] tpm2: Add TPM Software Stack (TSS)

[Daniel Kiper via Grub-devel]

On Thu, Jun 20, 2024 at 02:13:02PM +0800, Gary Lin wrote:
> On Wed, Jun 19, 2024 at 04:04:47PM +0200, Daniel Kiper wrote:
> > On Wed, Jun 19, 2024 at 02:41:13PM +0800, Gary Lin wrote:
> > > On Tue, Jun 18, 2024 at 03:30:03PM +0200, Daniel Kiper wrote:
> > > > On Fri, Jun 14, 2024 at 02:45:43PM +0800, Gary Lin wrote:
> > > > > From: Hernan Gatta 
> > > > >
> > > > > A Trusted Platform Module (TPM) Software Stack (TSS) provides logic to
> > > > > compose and submit TPM commands and parse reponses.
> > > > >
> > > > > A limited number of TPM commands may be accessed via the EFI TCG2
> > > > > protocol. This protocol exposes functionality that is primarily geared
> > > > > toward TPM usage within the context of Secure Boot. For all other TPM
> > > > > commands, however, such as sealing and unsealing, this protocol does 
> > > > > not
> > > > > provide any help, with the exception of passthrough command 
> > > > > submission.
> > > > >
> > > > > The SubmitCommand method allows a caller to send raw commands to the
> > > > > system's TPM and to receive the corresponding response. These
> > > > > command/response pairs are formatted using the TPM wire protocol. To
> > > > > construct commands in this way, and to parse the TPM's response, it is
> > > > > necessary to, first, possess knowledge of the various TPM structures, 
> > > > > and,
> > > > > second, of the TPM wire protocol itself.
> > > > >
> > > > > As such, this patch includes a set of header files that define the
> > > > > necessary TPM structures and TSS functions, implementations of various
> > > > > TPM2_* functions (inventoried below), and logic to write and read 
> > > > > command
> > > > > and response buffers, respectively, using the TPM wire protocol.
> > > > >
> > > > > Functions: TPM2_Create, TPM2_CreatePrimary, TPM2_EvictControl,
> > > > > TPM2_FlushContext, TPM2_Load, TPM2_PCR_Read, TPM2_PolicyGetDigest,
> > > > > TPM2_PolicyPCR, TPM2_ReadPublic, TPM2_StartAuthSession, TPM2_Unseal,
> > > > > TPM2_LoadExternal, TPM2_Hash, TPM2_VerifySignature,
> > > > > TPM2_PolicyAuthorize, TPM2_TestParms
> > > > >
> > > > > Signed-off-by: Hernan Gatta 
> > > > > Signed-off-by: Gary Lin 
> > > > > Reviewed-by: Stefan Berger 
> > > > > ---
> > > > >  grub-core/tpm2/buffer.c|  145 +++
> > > > >  grub-core/tpm2/mu.c| 1168 
> > > > > 
> > > > >  grub-core/tpm2/tcg2.c  |  143 +++
> > > > >  grub-core/tpm2/tpm2.c  | 1048 +
> > > > >  include/grub/tpm2/buffer.h |   65 ++
> > > > >  include/grub/tpm2/internal/functions.h |  156 
> > > > >  include/grub/tpm2/internal/structs.h   |  768 
> > > > >  include/grub/tpm2/internal/types.h |  403 
> > > > >  include/grub/tpm2/mu.h |  396 
> > > > >  include/grub/tpm2/tcg2.h   |   34 +
> > > > >  include/grub/tpm2/tpm2.h   |   34 +
> > > > >  11 files changed, 4360 insertions(+)
> > > > >  create mode 100644 grub-core/tpm2/buffer.c
> > > > >  create mode 100644 grub-core/tpm2/mu.c
> > > > >  create mode 100644 grub-core/tpm2/tcg2.c
> > > > >  create mode 100644 grub-core/tpm2/tpm2.c
> > > > >  create mode 100644 include/grub/tpm2/buffer.h
> > > > >  create mode 100644 include/grub/tpm2/internal/functions.h
> > > > >  create mode 100644 include/grub/tpm2/internal/structs.h
> > > > >  create mode 100644 include/grub/tpm2/internal/types.h
> > > > >  create mode 100644 include/grub/tpm2/mu.h
> > > > >  create mode 100644 include/grub/tpm2/tcg2.h
> > > > >  create mode 100644 include/grub/tpm2/tpm2.h
> > > > >
> > > > > diff --git a/grub-core/tpm2/buffer.c b/grub-core/tpm2/buffer.c
> > > > > new file mode 100644
> > > > > index 0..cb9f29497
> > > > > --- /dev/null
> > > > > +++ b/grub-core/tpm2/buffer.c
> > > >
> > > > I think this together with other TPM2 driver files should go to t

Re: [PATCH] fs/erofs: Fix EROFS label tests in grub-fs-tester

[Daniel Kiper]

On Wed, Jun 12, 2024 at 03:28:41PM +0800, Yifan Zhao wrote:
> mkfs.erofs with version < 1.6 does not support the -L option.
> Let's detect the version of mkfs.erofs and skip the label tests
> if it is not supported.
>
> Signed-off-by: Yifan Zhao 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 2/2] zfs: Add test for zfs zstd

[Daniel Kiper]

On Mon, Jun 17, 2024 at 02:44:09PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/2] zfs: Support zstd compression

[Daniel Kiper]

On Mon, Jun 17, 2024 at 02:44:08PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 2/2] emu/linux: Fix determination of program name

[Daniel Kiper]

On Mon, Jun 17, 2024 at 03:56:31PM +0300, Vladimir Serbinenko wrote:
> Current code works only if package matches binary name transformation rules.
> It's often true but is no waz guaranteed
>
> Fixes bug #64410
>
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/2] cryptodisk: Fix translatable message

[Daniel Kiper]

On Mon, Jun 17, 2024 at 03:56:30PM +0300, Vladimir Serbinenko wrote:
> Fixes bug #64408.
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] getroot: Unmark 2 strings for translation

[Daniel Kiper]

On Mon, Jun 17, 2024 at 03:59:56PM +0300, Vladimir Serbinenko wrote:
> First they're use macros so they can't be translated as-is.
> Second there is no point in translating them as they're too technical.
>
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v3] loader/efi/fdt: Add fdtdump command to access device tree

[Daniel Kiper]

On Mon, Jun 17, 2024 at 05:10:26PM +0200, Tobias Heider wrote:
> The fdtdump command allows dumping arbitrary device tree properties
> and saving them to a variable similar to the smbios command.
> 
> This is useful in scripts where further actions such as selecting a
> kernel or loading another device tree depend on the compatible or
> model values of the device tree provided by the firmware.
> 
> For now only the root level properties of the dtb are exposed.
> 
> Signed-off-by: Tobias Heider 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/1] util/grub-mkrescue: Check existence of option arguments

[Daniel Kiper]

On Mon, Jun 17, 2024 at 09:03:00PM +0200, Thomas Schmitt via Grub-devel wrote:
> As reported by Victoriia Egorova in bug 65880, grub-mkrescue does not
> verify that the expected argument of an option like -d or -k does really
> exist in argv.
> So check the loop counter before incrementing it inside the loop which
> copies argv to argp_argv. Issue an error message similar to what older
> versions of grub-mkrescue did with a missing argument (e.g 2.02).
>
> Fixes: https://savannah.gnu.org/bugs/index.php?65880
> Signed-off-by: Thomas Schmitt 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v17 11/20] key_protector: Add TPM2 Key Protector

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:44PM +0800, Gary Lin wrote:
> From: Hernan Gatta 
>
> The TPM2 key protector is a module that enables the automatic retrieval
> of a fully-encrypted disk's unlocking key from a TPM 2.0.
>
> The theory of operation is such that the module accepts various
> arguments, most of which are optional and therefore possess reasonable
> defaults. One of these arguments is the keyfile/tpm2key parameter, which
> is mandatory. There are two supported key formats:
>
> 1. Raw Sealed Key (--keyfile)
>When sealing a key with TPM2_Create, the public portion of the sealed
>key is stored in TPM2B_PUBLIC, and the private portion is in
>TPM2B_PRIVATE. The raw sealed key glues the fully marshalled
>TPM2B_PUBLIC and TPM2B_PRIVATE into one file.
>
> 2. TPM 2.0 Key (--tpm2key)
>The following is the ASN.1 definition of TPM 2.0 Key File:
>
>TPMPolicy ::= SEQUENCE {
>  CommandCode   [0] EXPLICIT INTEGER
>  CommandPolicy [1] EXPLICIT OCTET STRING
>}
>
>TPMAuthPolicy ::= SEQUENCE {
>  Name[0] EXPLICIT UTF8STRING OPTIONAL
>  Policy  [1] EXPLICIT SEQUENCE OF TPMPolicy
>}
>
>TPMKey ::= SEQUENCE {
>  typeOBJECT IDENTIFIER
>  emptyAuth   [0] EXPLICIT BOOLEAN OPTIONAL
>  policy  [1] EXPLICIT SEQUENCE OF TPMPolicy OPTIONAL
>  secret  [2] EXPLICIT OCTET STRING OPTIONAL
>  authPolicy  [3] EXPLICIT SEQUENCE OF TPMAuthPolicy OPTIONAL
>  description [4] EXPLICIT UTF8String OPTIONAL,
>  rsaParent   [5] EXPLICIT BOOLEAN OPTIONAL,
>  parent  INTEGER
>  pubkey  OCTET STRING
>  privkey OCTET STRING
>}
>
>   The TPM2 key protector only expects a "sealed" key in DER encoding,
>   so 'type' is always 2.23.133.10.1.5, 'emptyAuth' is 'TRUE', and
>   'secret' is empty. 'policy' and 'authPolicy' are the possible policy
>   command sequences to construst the policy digest to unseal the key.
>   Similar to the raw sealed key, the public portion (TPM2B_PUBLIC) of
>   the sealed key is stored in 'pubkey', and the private portion
>   (TPM2B_PRIVATE) is in 'privkey'.
>
>   For more details: 
> https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html
>
> This sealed key file is created via the grub-protect tool. The tool
> utilizes the TPM's sealing functionality to seal (i.e., encrypt) an
> unlocking key using a Storage Root Key (SRK) to the values of various
> Platform Configuration Registers (PCRs). These PCRs reflect the state
> of the system as it boots. If the values are as expected, the system
> may be considered trustworthy, at which point the TPM allows for a
> caller to utilize the private component of the SRK to unseal (i.e.,
> decrypt) the sealed key file. The caller, in this case, is this key
> protector.
>
> The TPM2 key protector registers two commands:
>
> - tpm2_key_protector_init: Initializes the state of the TPM2 key
>protector for later usage, clearing any
>previous state, too, if any.
>
> - tpm2_key_protector_clear: Clears any state set by tpm2_key_protector_init.
>
> The way this is expected to be used requires the user to, either
> interactively or, normally, via a boot script, initialize/configure
> the key protector and then specify that it be used by the 'cryptomount'
> command (modifications to this command are in a different patch).
>
> For instance, to unseal the raw sealed key file:
>
> tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-1.key
> cryptomount -u  -P tpm2
>
> tpm2_key_protector_init --keyfile=(hd0,gpt1)/efi/grub2/sealed-2.key 
> --pcrs=7,11
> cryptomount -u  -P tpm2
>
> Or, to unseal the TPM 2.0 Key file:
>
> tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub2/sealed-1.tpm
> cryptomount -u  -P tpm2
>
> tpm2_key_protector_init --tpm2key=(hd0,gpt1)/efi/grub2/sealed-2.tpm 
> --pcrs=7,11
> cryptomount -u  -P tpm2
>
> If a user does not initialize the key protector and attempts to use it
> anyway, the protector returns an error.
>
> Before unsealing the key, the TPM2 key protector follows the "TPMPolicy"
> sequences to enforce the TPM policy commands to construct a valid policy
> digest to unseal the key.
>
> For the TPM 2.0 Key files, 'authPolicy' may contain multiple "TPMPolicy"
> sequences, the TPM2 key protector iterates 'authPolicy' to find a valid
> sequence to unseal key. If 'authPolicy' is empty or all sequences in
> 'authPolicy' fail, the protector tries the one from 'policy'. In case
> 'policy' is also empty, the protector creates a "TPMPolicy" sequence
> based on the given PCR selection.
>
> For the raw sealed key, the TPM2 key protector treats the key file as a
> TPM 2.0 Key file without 'authPolicy' and 'policy', so the "TPMPolicy"
> sequence is always based on the PCR selection from the command
> parameters.
>
> This commit only supports one policy command: TPM2_PolicyPCR. The
> command set will be extended to support advanced features, such as
> authorized policy, in the later 

Re: [PATCH v17 10/20] tpm2: Add TPM Software Stack (TSS)

[Daniel Kiper via Grub-devel]

On Wed, Jun 19, 2024 at 02:41:13PM +0800, Gary Lin wrote:
> On Tue, Jun 18, 2024 at 03:30:03PM +0200, Daniel Kiper wrote:
> > On Fri, Jun 14, 2024 at 02:45:43PM +0800, Gary Lin wrote:
> > > From: Hernan Gatta 
> > >
> > > A Trusted Platform Module (TPM) Software Stack (TSS) provides logic to
> > > compose and submit TPM commands and parse reponses.
> > >
> > > A limited number of TPM commands may be accessed via the EFI TCG2
> > > protocol. This protocol exposes functionality that is primarily geared
> > > toward TPM usage within the context of Secure Boot. For all other TPM
> > > commands, however, such as sealing and unsealing, this protocol does not
> > > provide any help, with the exception of passthrough command submission.
> > >
> > > The SubmitCommand method allows a caller to send raw commands to the
> > > system's TPM and to receive the corresponding response. These
> > > command/response pairs are formatted using the TPM wire protocol. To
> > > construct commands in this way, and to parse the TPM's response, it is
> > > necessary to, first, possess knowledge of the various TPM structures, and,
> > > second, of the TPM wire protocol itself.
> > >
> > > As such, this patch includes a set of header files that define the
> > > necessary TPM structures and TSS functions, implementations of various
> > > TPM2_* functions (inventoried below), and logic to write and read command
> > > and response buffers, respectively, using the TPM wire protocol.
> > >
> > > Functions: TPM2_Create, TPM2_CreatePrimary, TPM2_EvictControl,
> > > TPM2_FlushContext, TPM2_Load, TPM2_PCR_Read, TPM2_PolicyGetDigest,
> > > TPM2_PolicyPCR, TPM2_ReadPublic, TPM2_StartAuthSession, TPM2_Unseal,
> > > TPM2_LoadExternal, TPM2_Hash, TPM2_VerifySignature,
> > > TPM2_PolicyAuthorize, TPM2_TestParms
> > >
> > > Signed-off-by: Hernan Gatta 
> > > Signed-off-by: Gary Lin 
> > > Reviewed-by: Stefan Berger 
> > > ---
> > >  grub-core/tpm2/buffer.c|  145 +++
> > >  grub-core/tpm2/mu.c| 1168 
> > >  grub-core/tpm2/tcg2.c  |  143 +++
> > >  grub-core/tpm2/tpm2.c  | 1048 +
> > >  include/grub/tpm2/buffer.h |   65 ++
> > >  include/grub/tpm2/internal/functions.h |  156 
> > >  include/grub/tpm2/internal/structs.h   |  768 
> > >  include/grub/tpm2/internal/types.h |  403 
> > >  include/grub/tpm2/mu.h |  396 
> > >  include/grub/tpm2/tcg2.h   |   34 +
> > >  include/grub/tpm2/tpm2.h   |   34 +
> > >  11 files changed, 4360 insertions(+)
> > >  create mode 100644 grub-core/tpm2/buffer.c
> > >  create mode 100644 grub-core/tpm2/mu.c
> > >  create mode 100644 grub-core/tpm2/tcg2.c
> > >  create mode 100644 grub-core/tpm2/tpm2.c
> > >  create mode 100644 include/grub/tpm2/buffer.h
> > >  create mode 100644 include/grub/tpm2/internal/functions.h
> > >  create mode 100644 include/grub/tpm2/internal/structs.h
> > >  create mode 100644 include/grub/tpm2/internal/types.h
> > >  create mode 100644 include/grub/tpm2/mu.h
> > >  create mode 100644 include/grub/tpm2/tcg2.h
> > >  create mode 100644 include/grub/tpm2/tpm2.h
> > >
> > > diff --git a/grub-core/tpm2/buffer.c b/grub-core/tpm2/buffer.c
> > > new file mode 100644
> > > index 0..cb9f29497
> > > --- /dev/null
> > > +++ b/grub-core/tpm2/buffer.c
> >
> > I think this together with other TPM2 driver files should go to the
> > grub-core/commands/efi/tpm2 directory.
> >
> The TPM2 stack is not EFI only. The only EFI related code is in

Ah, right... Then I think we should have two GRUB modules. One TPM2
generic and one strictly EFI which depends on generic one.

> grub-core/tpm2/tcg2.c which mainly implements how the TPM2 commands to
> be submitted. I'd propose to move them to grub-core/commands/tpm2 and
> rename tcg2.c to tcg2-efi.c.

One should land in the grub-core/commands/tss2 directory and another in
the grub-core/commands/efi or grub-core/commands/efi/tmp2 if needed.

[...]

> > > diff --git a/grub-core/tpm2/mu.c b/grub-core/tpm2/mu.c
> > > new file mode 100644
> > > index 0..10ed71c04
> > > --- /dev/null
> > > +++ b/grub-core/tpm2/mu.c
> >
> > I can imagine where it comes from but I think it should be efi.c instead
> >

Re: [PATCH v17 10/20] tpm2: Add TPM Software Stack (TSS)

[Daniel Kiper via Grub-devel]

On Wed, Jun 19, 2024 at 02:43:08PM +0800, Gary Lin wrote:
> On Tue, Jun 18, 2024 at 05:41:13PM +0200, Daniel Kiper wrote:
> > On Fri, Jun 14, 2024 at 02:45:43PM +0800, Gary Lin wrote:
> > > From: Hernan Gatta 
> > >
> > > A Trusted Platform Module (TPM) Software Stack (TSS) provides logic to
> > > compose and submit TPM commands and parse reponses.
> > >
> > > A limited number of TPM commands may be accessed via the EFI TCG2
> > > protocol. This protocol exposes functionality that is primarily geared
> > > toward TPM usage within the context of Secure Boot. For all other TPM
> > > commands, however, such as sealing and unsealing, this protocol does not
> > > provide any help, with the exception of passthrough command submission.
> > >
> > > The SubmitCommand method allows a caller to send raw commands to the
> > > system's TPM and to receive the corresponding response. These
> > > command/response pairs are formatted using the TPM wire protocol. To
> > > construct commands in this way, and to parse the TPM's response, it is
> > > necessary to, first, possess knowledge of the various TPM structures, and,
> > > second, of the TPM wire protocol itself.
> > >
> > > As such, this patch includes a set of header files that define the
> > > necessary TPM structures and TSS functions, implementations of various
> > > TPM2_* functions (inventoried below), and logic to write and read command
> > > and response buffers, respectively, using the TPM wire protocol.
> > >
> > > Functions: TPM2_Create, TPM2_CreatePrimary, TPM2_EvictControl,
> > > TPM2_FlushContext, TPM2_Load, TPM2_PCR_Read, TPM2_PolicyGetDigest,
> > > TPM2_PolicyPCR, TPM2_ReadPublic, TPM2_StartAuthSession, TPM2_Unseal,
> > > TPM2_LoadExternal, TPM2_Hash, TPM2_VerifySignature,
> > > TPM2_PolicyAuthorize, TPM2_TestParms
> > >
> > > Signed-off-by: Hernan Gatta 
> > > Signed-off-by: Gary Lin 
> > > Reviewed-by: Stefan Berger 
> > > ---
> > >  grub-core/tpm2/buffer.c|  145 +++
> > >  grub-core/tpm2/mu.c| 1168 
> > >  grub-core/tpm2/tcg2.c  |  143 +++
> > >  grub-core/tpm2/tpm2.c  | 1048 +
> > >  include/grub/tpm2/buffer.h |   65 ++
> > >  include/grub/tpm2/internal/functions.h |  156 
> > >  include/grub/tpm2/internal/structs.h   |  768 
> > >  include/grub/tpm2/internal/types.h |  403 
> > >  include/grub/tpm2/mu.h |  396 
> > >  include/grub/tpm2/tcg2.h   |   34 +
> > >  include/grub/tpm2/tpm2.h   |   34 +
> > >  11 files changed, 4360 insertions(+)
> > >  create mode 100644 grub-core/tpm2/buffer.c
> > >  create mode 100644 grub-core/tpm2/mu.c
> > >  create mode 100644 grub-core/tpm2/tcg2.c
> > >  create mode 100644 grub-core/tpm2/tpm2.c
> > >  create mode 100644 include/grub/tpm2/buffer.h
> > >  create mode 100644 include/grub/tpm2/internal/functions.h
> > >  create mode 100644 include/grub/tpm2/internal/structs.h
> > >  create mode 100644 include/grub/tpm2/internal/types.h
> > >  create mode 100644 include/grub/tpm2/mu.h
> > >  create mode 100644 include/grub/tpm2/tcg2.h
> > >  create mode 100644 include/grub/tpm2/tpm2.h
> >
> > And I think this patch can be broken up to smaller parts...
> >
> Then I'll try to split the patch to 3 patches: buffer, mu/structs, and
> TPM2 commands.

Makes sense for me...

Daniel

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

Re: [PATCH v17 10/20] tpm2: Add TPM Software Stack (TSS)

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:43PM +0800, Gary Lin wrote:
> From: Hernan Gatta 
>
> A Trusted Platform Module (TPM) Software Stack (TSS) provides logic to
> compose and submit TPM commands and parse reponses.
>
> A limited number of TPM commands may be accessed via the EFI TCG2
> protocol. This protocol exposes functionality that is primarily geared
> toward TPM usage within the context of Secure Boot. For all other TPM
> commands, however, such as sealing and unsealing, this protocol does not
> provide any help, with the exception of passthrough command submission.
>
> The SubmitCommand method allows a caller to send raw commands to the
> system's TPM and to receive the corresponding response. These
> command/response pairs are formatted using the TPM wire protocol. To
> construct commands in this way, and to parse the TPM's response, it is
> necessary to, first, possess knowledge of the various TPM structures, and,
> second, of the TPM wire protocol itself.
>
> As such, this patch includes a set of header files that define the
> necessary TPM structures and TSS functions, implementations of various
> TPM2_* functions (inventoried below), and logic to write and read command
> and response buffers, respectively, using the TPM wire protocol.
>
> Functions: TPM2_Create, TPM2_CreatePrimary, TPM2_EvictControl,
> TPM2_FlushContext, TPM2_Load, TPM2_PCR_Read, TPM2_PolicyGetDigest,
> TPM2_PolicyPCR, TPM2_ReadPublic, TPM2_StartAuthSession, TPM2_Unseal,
> TPM2_LoadExternal, TPM2_Hash, TPM2_VerifySignature,
> TPM2_PolicyAuthorize, TPM2_TestParms
>
> Signed-off-by: Hernan Gatta 
> Signed-off-by: Gary Lin 
> Reviewed-by: Stefan Berger 
> ---
>  grub-core/tpm2/buffer.c|  145 +++
>  grub-core/tpm2/mu.c| 1168 
>  grub-core/tpm2/tcg2.c  |  143 +++
>  grub-core/tpm2/tpm2.c  | 1048 +
>  include/grub/tpm2/buffer.h |   65 ++
>  include/grub/tpm2/internal/functions.h |  156 
>  include/grub/tpm2/internal/structs.h   |  768 
>  include/grub/tpm2/internal/types.h |  403 
>  include/grub/tpm2/mu.h |  396 
>  include/grub/tpm2/tcg2.h   |   34 +
>  include/grub/tpm2/tpm2.h   |   34 +
>  11 files changed, 4360 insertions(+)
>  create mode 100644 grub-core/tpm2/buffer.c
>  create mode 100644 grub-core/tpm2/mu.c
>  create mode 100644 grub-core/tpm2/tcg2.c
>  create mode 100644 grub-core/tpm2/tpm2.c
>  create mode 100644 include/grub/tpm2/buffer.h
>  create mode 100644 include/grub/tpm2/internal/functions.h
>  create mode 100644 include/grub/tpm2/internal/structs.h
>  create mode 100644 include/grub/tpm2/internal/types.h
>  create mode 100644 include/grub/tpm2/mu.h
>  create mode 100644 include/grub/tpm2/tcg2.h
>  create mode 100644 include/grub/tpm2/tpm2.h

And I think this patch can be broken up to smaller parts...

Daniel

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

Re: [PATCH v17 10/20] tpm2: Add TPM Software Stack (TSS)

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:43PM +0800, Gary Lin wrote:
> From: Hernan Gatta 
>
> A Trusted Platform Module (TPM) Software Stack (TSS) provides logic to
> compose and submit TPM commands and parse reponses.
>
> A limited number of TPM commands may be accessed via the EFI TCG2
> protocol. This protocol exposes functionality that is primarily geared
> toward TPM usage within the context of Secure Boot. For all other TPM
> commands, however, such as sealing and unsealing, this protocol does not
> provide any help, with the exception of passthrough command submission.
>
> The SubmitCommand method allows a caller to send raw commands to the
> system's TPM and to receive the corresponding response. These
> command/response pairs are formatted using the TPM wire protocol. To
> construct commands in this way, and to parse the TPM's response, it is
> necessary to, first, possess knowledge of the various TPM structures, and,
> second, of the TPM wire protocol itself.
>
> As such, this patch includes a set of header files that define the
> necessary TPM structures and TSS functions, implementations of various
> TPM2_* functions (inventoried below), and logic to write and read command
> and response buffers, respectively, using the TPM wire protocol.
>
> Functions: TPM2_Create, TPM2_CreatePrimary, TPM2_EvictControl,
> TPM2_FlushContext, TPM2_Load, TPM2_PCR_Read, TPM2_PolicyGetDigest,
> TPM2_PolicyPCR, TPM2_ReadPublic, TPM2_StartAuthSession, TPM2_Unseal,
> TPM2_LoadExternal, TPM2_Hash, TPM2_VerifySignature,
> TPM2_PolicyAuthorize, TPM2_TestParms
>
> Signed-off-by: Hernan Gatta 
> Signed-off-by: Gary Lin 
> Reviewed-by: Stefan Berger 
> ---
>  grub-core/tpm2/buffer.c|  145 +++
>  grub-core/tpm2/mu.c| 1168 
>  grub-core/tpm2/tcg2.c  |  143 +++
>  grub-core/tpm2/tpm2.c  | 1048 +
>  include/grub/tpm2/buffer.h |   65 ++
>  include/grub/tpm2/internal/functions.h |  156 
>  include/grub/tpm2/internal/structs.h   |  768 
>  include/grub/tpm2/internal/types.h |  403 
>  include/grub/tpm2/mu.h |  396 
>  include/grub/tpm2/tcg2.h   |   34 +
>  include/grub/tpm2/tpm2.h   |   34 +
>  11 files changed, 4360 insertions(+)
>  create mode 100644 grub-core/tpm2/buffer.c
>  create mode 100644 grub-core/tpm2/mu.c
>  create mode 100644 grub-core/tpm2/tcg2.c
>  create mode 100644 grub-core/tpm2/tpm2.c
>  create mode 100644 include/grub/tpm2/buffer.h
>  create mode 100644 include/grub/tpm2/internal/functions.h
>  create mode 100644 include/grub/tpm2/internal/structs.h
>  create mode 100644 include/grub/tpm2/internal/types.h
>  create mode 100644 include/grub/tpm2/mu.h
>  create mode 100644 include/grub/tpm2/tcg2.h
>  create mode 100644 include/grub/tpm2/tpm2.h
>
> diff --git a/grub-core/tpm2/buffer.c b/grub-core/tpm2/buffer.c
> new file mode 100644
> index 0..cb9f29497
> --- /dev/null
> +++ b/grub-core/tpm2/buffer.c

I think this together with other TPM2 driver files should go to the
grub-core/commands/efi/tpm2 directory.

> @@ -0,0 +1,145 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022 Microsoft Corporation
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see .
> + */
> +
> +#include 
> +#include 
> +
> +void grub_tpm2_buffer_init (grub_tpm2_buffer_t buffer)
> +{
> +  grub_memset (buffer->data, 0xDD, sizeof (buffer->data));

If you init the buffer->data with 0xDD instead of 0 then it begs for
a comment. And s/0xDD/0xdd/...

> +  buffer->size = 0;
> +  buffer->offset = 0;
> +  buffer->cap = sizeof (buffer->data);
> +  buffer->error = 0;
> +}
> +
> +void
> +grub_tpm2_buffer_pack (grub_tpm2_buffer_t buffer, const void* data,
> +grub_size_t size)
> +{
> +  grub_uint32_t r = buffer->cap - buffer->size;
> +
> +  if (buffer->error)
> +return;
> +
> +  if (size > r)
> +{
> +  buffer->error = 1;
> +  return;
> +}
> +
> +  grub_memcpy (>data[buffer->size], (void*) data, size);
> +  buffer->size += size;
> +}
> +
> +void
> +grub_tpm2_buffer_pack_u8 (grub_tpm2_buffer_t buffer, grub_uint8_t value)
> +{
> +  grub_tpm2_buffer_pack (buffer, (const char*) , sizeof (value));
> +}
> +
> +void
> +grub_tpm2_buffer_pack_u16 (grub_tpm2_buffer_t buffer, grub_uint16_t value)
> +{

Re: [PATCH v17 09/20] key_protector: Add key protectors framework

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:42PM +0800, Gary Lin wrote:
> From: Hernan Gatta 
>
> A key protector encapsulates functionality to retrieve an unlocking key
> for a fully-encrypted disk from a specific source. A key protector
> module registers itself with the key protectors framework when it is
> loaded and unregisters when unloaded. Additionally, a key protector may
> accept parameters that describe how it should operate.
>
> The key protectors framework, besides offering registration and
> unregistration functions, also offers a one-stop routine for finding and
> invoking a key protector by name. If a key protector with the specified
> name exists and if an unlocking key is successfully retrieved by it, the
> function returns to the caller the retrieved key and its length.
>
> Cc: Vladimir Serbinenko 
> Signed-off-by: Hernan Gatta 
> Signed-off-by: Gary Lin 
> Reviewed-by: Stefan Berger 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v17 08/20] libtasn1: Add the documentation

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:41PM +0800, Gary Lin wrote:
> Document libtasn1 in docs/grub-dev.texi and add the upgrade steps.
> Also add the patches to make libtasn1 compatible with grub code.
>
> Signed-off-by: Gary Lin 
> Reviewed-by: Vladimir Serbinenko 
> ---
>  docs/grub-dev.texi | 33 +
>  1 file changed, 33 insertions(+)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index 1276c5930..62856f0f1 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -506,6 +506,7 @@ to update it.
>  * Gnulib::
>  * jsmn::
>  * minilzo::
> +* libtasn1::
>  @end menu
>
>  @node Gnulib
> @@ -596,6 +597,38 @@ cp minilzo-2.10/*.[hc] grub-core/lib/minilzo
>  rm -r minilzo-2.10*
>  @end example
>
> +@node libtasn1
> +@section libtasn1
> +
> +libtasn1 is a library providing Abstract Syntax Notation One (ASN.1, as
> +specified by the X.680 ITU-T recommendation) parsing and structures 
> management,
> +and Distinguished Encoding Rules (DER, as per X.690) encoding and decoding
> +functions.
> +
> +To upgrade to a new version of the libtasn1 library, download the release
> +tarball and copy the files into the target directory:
> +
> +@example
> +curl -L -O https://ftp.gnu.org/gnu/libtasn1/libtasn1-4.19.0.tar.gz
> +tar xf libtasn1-4.19.0.tar.gz
> +rm -rf grub-core/lib/libtasn1/
> +mkdir -p grub-core/lib/libtasn1/lib/
> +cp libtasn1-4.19.0/@lbracechar{}README.md,COPYING@rbracechar{} 
> grub-core/lib/libtasn1/
> +cp 
> libtasn1-4.19.0/lib/@lbracechar{}coding.c,decoding.c,element.c,element.h,errors.c,gstr.c,gstr.h,int.h,parser_aux.c,parser_aux.h,structure.c,structure.h@rbracechar{}
>  grub-core/lib/libtasn1/lib/
> +cp libtasn1-4.19.0/lib/includes/libtasn1.h grub-core/lib/libtasn1/
> +rm -rf libtasn1-4.19.0*

Please add commands for copying libtasn1 test files here too.

> +@end example
> +
> +After upgrading the library, it may be necessary to apply the patches in
> +@file{grub-core/lib/libtasn1-patches/} to adjust the code to be compatible 
> with
> +grub. These patches were needed to use the current version of libtasn1. The

s/grub/GRUB/

> +existing patches may not apply cleanly, apply at all, or even be needed for a
> +newer version of the library, and other patches maybe needed due to changes 
> in

s/maybe/may be/

> +the newer version. If existing patches need to be refreshed to apply cleanly,
> +please include updated patches as part of the a patch set sent to the list.
> +If new patches are needed or existing patches are not needed, also please 
> send
> +additions or removals as part of any patch set upgrading libtasn1.

Daniel

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

Re: [PATCH v17 07/20] asn1_test: test module for libtasn1

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:40PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
> Import tests from libtasn1 that don't use functionality we don't
> import. This test module is integrated into functional_test so that the
> user can run the test in grub shell.
>
> This doesn't test the full decoder but that will be exercised in
> test suites for coming patch sets.
>
> Add testcase target in accordance with
> 5e10be48e5 tests: Add check-native and check-nonnative make targets
>
> Cc: Vladimir Serbinenko 
> Signed-off-by: Daniel Axtens 
> Signed-off-by: Gary Lin 
> ---
>  Makefile.util.def |   6 +
>  grub-core/Makefile.core.def   |  14 ++
>  .../tests/asn1/CVE-2018-1000654-1_asn1_tab.h  |  32 +++
>  .../tests/asn1/CVE-2018-1000654-2_asn1_tab.h  |  36 +++
>  grub-core/tests/asn1/CVE-2018-1000654.c   |  58 +
>  grub-core/tests/asn1/Test_overflow.c  | 134 
>  grub-core/tests/asn1/Test_simple.c| 205 ++
>  grub-core/tests/asn1/Test_strings.c   | 142 
>  grub-core/tests/asn1/asn1_test.c  |  49 +
>  grub-core/tests/asn1/asn1_test.h  |  44 
>  grub-core/tests/asn1/object-id-decoding.c | 109 ++
>  grub-core/tests/asn1/object-id-encoding.c | 114 ++
>  grub-core/tests/asn1/octet-string.c   | 199 +
>  grub-core/tests/asn1/reproducers.c|  80 +++
>  grub-core/tests/lib/functional_test.c |   1 +
>  tests/asn1_test.in|  11 +

If most of these files come from libtasn1 then you should add a list of
commands which allows us recreate them from libtasn1 source. And probably
introduction of original libtasn1 test files should happen in the patch #2.

[...]

> diff --git a/grub-core/tests/asn1/Test_overflow.c 
> b/grub-core/tests/asn1/Test_overflow.c
> new file mode 100644
> index 0..aff6b410a
> --- /dev/null
> +++ b/grub-core/tests/asn1/Test_overflow.c
> @@ -0,0 +1,134 @@
> +/*
> + * Copyright (C) 2012-2014 Free Software Foundation, Inc.
> + *
> + * This file is part of LIBTASN1.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + *
> + */
> +
> +/* Written by Simon Josefsson */
> +
> +#include "asn1_test.h"
> +
> +int
> +test_overflow(void)
> +{
> +  /* Test that values larger than long are rejected.  This has worked
> + fine with all versions of libtasn1. */
> +
> +  {
> +unsigned char der[] = "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF";
> +long l;
> +int len;
> +
> +l = asn1_get_length_der (der, sizeof der, );
> +
> +if (l != -2L)
> +  {
> + grub_printf ("ERROR: asn1_get_length_der bignum (l %ld len %d)\n", l, 
> len);
> + return 1;
> +  }
> +  }
> +
> +  /* Test that values larger than int but smaller than long are
> + rejected.  This limitation was introduced with libtasn1 2.12. */
> +#if (GRUB_LONG_MAX > GRUB_INT_MAX)

This change suggests it is a mixture of libtasn1 source and GRUB specific
patches. Please disaggregate them.

Daniel

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

Re: [PATCH v17 06/20] libtasn1: compile into asn1 module

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:39PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
> Create a wrapper file that specifies the module license.
> Set up the makefile so it is built.
>
> Signed-off-by: Daniel Axtens 
> Signed-off-by: Gary Lin 
> Reviewed-by: Vladimir Serbinenko 

Due to amount of changes to the patch I think this RB should be dropped now.

Though Reviewed-by: Daniel Kiper ...

Two nits below...

> ---
>  autogen.sh | 16 
>  grub-core/Makefile.core.def| 15 +++
>  grub-core/lib/libtasn1_wrap/wrap.c | 26 ++
>  3 files changed, 57 insertions(+)
>  create mode 100644 grub-core/lib/libtasn1_wrap/wrap.c
>
> diff --git a/autogen.sh b/autogen.sh
> index 195daa541..43272722f 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -51,6 +51,22 @@ for x in mpi-asm-defs.h mpih-add1.c mpih-sub1.c 
> mpih-mul1.c mpih-mul2.c mpih-mul
>  cp grub-core/lib/libgcrypt-grub/mpi/generic/"$x" 
> grub-core/lib/libgcrypt-grub/mpi/"$x"
>  done
>
> +echo "Importing libtasn1..."
> +if [ -d grub-core/lib/libtasn1-grub ]; then
> +  rm -rf grub-core/lib/libtasn1-grub
> +fi
> +
> +mkdir -p grub-core/lib/libtasn1-grub/lib
> +cp grub-core/lib/libtasn1/lib/*.[ch] grub-core/lib/libtasn1-grub/lib
> +cp grub-core/lib/libtasn1/libtasn1.h grub-core/lib/libtasn1-grub/
> +
> +for patch in \
> + 0001-libtasn1-disable-code-not-needed-in-grub.patch \
> + 0002-libtasn1-changes-for-grub-compatibility.patch \
> + 0003-libtasn1-fix-the-potential-buffer-overrun.patch ; do
> +  patch -p1 -i grub-core/lib/libtasn1-patches/$patch
> +done
> +
>  echo "Generating Automake input..."
>
>  # Automake doesn't like including files from a path outside the project.
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7fa9446bd..89944004b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2605,3 +2605,18 @@ module = {
>enable = efi;
>depends = part_gpt;
>  };
> +
> +module = {
> +  name = asn1;
> +  common = lib/libtasn1-grub/lib/decoding.c;
> +  common = lib/libtasn1-grub/lib/coding.c;
> +  common = lib/libtasn1-grub/lib/element.c;
> +  common = lib/libtasn1-grub/lib/structure.c;
> +  common = lib/libtasn1-grub/lib/parser_aux.c;
> +  common = lib/libtasn1-grub/lib/gstr.c;
> +  common = lib/libtasn1-grub/lib/errors.c;
> +  common = lib/libtasn1_wrap/wrap.c;
> +  cflags = '$(CFLAGS_POSIX) $(CFLAGS_GNULIB)';
> +  // -Wno-type-limits comes from libtasn1's configure.ac

Please use "/* ... */" for comments.

> +  cppflags = '$(CPPFLAGS_POSIX) $(CPPFLAGS_GNULIB) 
> -I$(srcdir)/lib/libtasn1-grub -I$(srcdir)/lib/libtasn1-grub/lib 
> -Wno-type-limits';
> +};
> diff --git a/grub-core/lib/libtasn1_wrap/wrap.c 
> b/grub-core/lib/libtasn1_wrap/wrap.c
> new file mode 100644
> index 0..622ba942e
> --- /dev/null
> +++ b/grub-core/lib/libtasn1_wrap/wrap.c
> @@ -0,0 +1,26 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2020 IBM Corporation
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include 
> +
> +/*
> + * libtasn1 is provided under LGPL2.1+, which is compatible
> + * with GPL3+. As Grub as a whole is under GPL3+, this module

s/Grub/GRUB/

Daniel

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

Re: [PATCH v17 05/20] libtasn1: fix the potential buffer overrun

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:38PM +0800, Gary Lin wrote:
> In _asn1_tag_der(), the first while loop for the long form may end up
> with a 'k' value with 'ASN1_MAX_TAG_SIZE' and cause the buffer overrun
> in the second while loop. This commit tweaks the conditional check to
> avoid producing a too large 'k'.
>
> This is a quick fix and may differ from the official upstream fix.
>
> libtasn1 issue: https://gitlab.com/gnutls/libtasn1/-/issues/49

This patch does not need Daniel Axtens SB of course. Sorry for
not being precise.

> Signed-off-by: Gary Lin 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v17 04/20] libtasn1: changes for grub compatibility

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:37PM +0800, Gary Lin wrote:
> Based on the patch from "Daniel Axtens "
>
> Do a few things to make libtasn1 compile as part of grub:
>
>  - remove _asn1_strcat and replace strcat with the bound-checked
>_asn1_str_cat except the one inside _asn1_str_cat. That strcat is
>replaced with strcpy.
>
>  - adjust header paths in libtasn1.h
>
>  - replace a 64 bit division with a call to grub_divmod64, preventing
>creation of __udivdi3 calls on 32 bit platforms.

This patch should be split into three separate patches.

I think this and other libtasn1 fixes should have
"Signed-off-by: Daniel Axtens " here.

> Signed-off-by: Gary Lin 

Daniel

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

Re: [PATCH v17 03/20] libtasn1: disable code not needed in grub

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:36PM +0800, Gary Lin wrote:
> Based on the patch from "Daniel Axtens "
>
> We don't expect to be able to write ASN.1, only read it,
> so we can disable some code.
>
> Do that with #if 0/#endif, rather than deletion. This means
> that the difference between upstream and grub is smaller,
> which should make updating libtasn1 easier in the future.
>
> With these exclusions we also avoid the need for minmax.h,
> which is convenient because it means we don't have to
> import it from gnulib.
>
> Signed-off-by: Gary Lin 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v17 02/20] libtasn1: import libtasn1-4.19.0

[Daniel Kiper via Grub-devel]

On Fri, Jun 14, 2024 at 02:45:35PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
> Import a very trimmed-down set of libtasn1 files:
>
> curl -L -O https://ftp.gnu.org/gnu/libtasn1/libtasn1-4.19.0.tar.gz
> tar xf libtasn1-4.19.0.tar.gz
> rm -rf grub-core/lib/libtasn1/
> mkdir -p grub-core/lib/libtasn1/lib/
> cp libtasn1-4.19.0/{README.md,COPYING} grub-core/lib/libtasn1/
> cp 
> libtasn1-4.19.0/lib/{coding.c,decoding.c,element.c,element.h,errors.c,gstr.c,gstr.h,int.h,parser_aux.c,parser_aux.h,structure.c,structure.h}
>  grub-core/libtasn1/lib/
> cp libtasn1-4.19.0/lib/includes/libtasn1.h grub-core/lib/libtasn1/
> rm -rf libtasn1-4.19.0*
>
> Signed-off-by: Daniel Axtens 
> Signed-off-by: Gary Lin 
> Reviewed-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree

[Daniel Kiper]

On Fri, Jun 14, 2024 at 06:26:00PM +0200, Tobias Heider wrote:
> On Fri, Jun 14, 2024 at 06:03:23PM +0200, Daniel Kiper wrote:
> > On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> > > The fdtdump command allows dumping arbitrary device tree properties
> > > and saving them to a variable similar to the smbios command.
> > >
> > > This is useful in scripts where further actions such as selecting a
> > > kernel or loading another device tree depend on the compatible or
> > > model values of the device tree provided by the firmware.
> > >
> > > For now only the root level properties of the dtb are exposed.
> > >
> > > Signed-off-by: Tobias Heider 
> > > ---
> > >  grub-core/loader/efi/fdt.c | 51 +-
> > >  1 file changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > > index 439964b9c..8fa0b3b09 100644
> > > --- a/grub-core/loader/efi/fdt.c
> > > +++ b/grub-core/loader/efi/fdt.c
> > > @@ -1,6 +1,7 @@
> > >  /*
> > >   *  GRUB  --  GRand Unified Bootloader
> > >   *  Copyright (C) 2013-2015  Free Software Foundation, Inc.
> > > + *  Copyright (C) 2024   Canonical, Ltd.
> > >   *
> > >   *  GRUB is free software: you can redistribute it and/or modify
> > >   *  it under the terms of the GNU General Public License as published by
> > > @@ -18,15 +19,18 @@
> > >
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > >  #include 
> > >  #include 
> > >
> > > +static void *fw_fdt;
> > >  static void *loaded_fdt;
> > >  static void *fdt;
> > >
> > > @@ -36,6 +40,13 @@ static void *fdt;
> > >   sizeof (FDT_ADDR_CELLS_STRING) + \
> > >   sizeof (FDT_SIZE_CELLS_STRING))
> > >
> > > +static const struct grub_arg_option options_fdtdump[] = {
> > > +  {"prop",   'p', 0, N_("Get property."), N_("prop"), 
> > > ARG_TYPE_STRING},
> > > +  {"set",   '\0', 0, N_("Store the value in the given variable 
> > > name."),
> > > + N_("variable"), ARG_TYPE_STRING},
> > > +  {0, 0, 0, 0, 0, 0}
> > > +};
> > > +
> > >  void *
> > >  grub_fdt_load (grub_size_t additional_size)
> > >  {
> > > @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
> > >if (loaded_fdt)
> > >  raw_fdt = loaded_fdt;
> > >else
> > > -raw_fdt = grub_efi_get_firmware_fdt();
> > > +raw_fdt = fw_fdt;
> >
> > This change seems suspicious for me. Could you explain why it is needed?
>
> Since I added grub_efi_get_firmware_fdt() to module init function and the
> device tree passed by the firmware is a fairly static property it made
> sense to me to use the address we have instead of rereading it in 
> grub_fdt_load().
>
> If you are more comfortable if I don't touch that code path I can drop that
> change and simply read it twice.

I am OK with this grub_efi_get_firmware_fdt() shuffling but it has to be
explained in the commit message.

Daniel

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

Re: [PATCH] ofnet: Remove 200 ms timeout in get_card_packet to reduce input latency

[Daniel Kiper]

On Mon, May 06, 2024 at 10:34:22AM +0800, Michael Chang via Grub-devel wrote:
> When grub image is netbooted on ppc64le, the keyboard input exhibits
> significant latency, reports even say that characters are processed
> about once per second. This issue makes interactively trying to debug a
> ppc64le config very difficult.
>
> It seems that the latency is largely caused by a 200 ms timeout in the
> idle event loop, during which the network card interface is consistently
> polled for incoming packets. Often, no packets arrive during this
> period, so the timeout nearly always expires, which blocks the response
> to key inputs.
>
> Furthermore, this 200 ms timeout might not need to be enforced at this
> basic layer, considering that grub performs synchronous reads and its
> timeout management is actually handled by higher layers, not directly in
> the card instance. Additionally, the idle polling, which reacts to
> unsolicited packets like ICMP and SLAAC, would be fine at a less
> frequent polling interval, rather than needing a timeout for receiving a
> response.
>
> For these reasons, we believe the timeout in get_card_packet should be
> effectively removed. According to test results, the delay has
> disappeared, and it is now much easier to use interactively.
>
> Signed-Off-by: Michael Chang 
> Tested-by: Tony Jones 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Add --noefistub option for linux

[Daniel Kiper]

On Thu, May 16, 2024 at 09:43:46PM +0300, Vladimir Serbinenko wrote:
> In some cases like loading kernel from native disk (e.g. nvme) not
> supported by EFI in question efi stub is not an option. Allow
> user to disable efi stub and fallback to older protocol

I think this patch should be considered together with NVMe patch.

Missing SOB.

> ---
>  grub-core/loader/efi/linux.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index bfbd95aee..0bf9d9cbb 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -459,10 +459,18 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>grub_file_t file = 0;
>struct linux_arch_kernel_header lh;
>grub_err_t err;
> +  int force_legacy = 0;

I would use bool here.

>grub_dl_ref (my_mod);
>
> -  if (grub_is_shim_lock_enabled () == true)
> +  if (argc > 0 && grub_strcmp(argv[0], "--noefistub") == 0)
> +{
> +  force_legacy = 1;
> +  argv++;
> +  argc--;
> +}
> +
> +  if (grub_is_shim_lock_enabled () == true || force_legacy)

Daniel

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

Re: [PATCH 1/2] zfs: Support zstd compression

[Daniel Kiper]

On Thu, May 16, 2024 at 10:42:25PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 
> ---
>  grub-core/Makefile.core.def |  1 +
>  grub-core/fs/zfs/zfs.c  | 32 
>  include/grub/zfs/zio.h  |  1 +
>  3 files changed, 34 insertions(+)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8e1b1d9f3..2ba4962d5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1596,6 +1596,7 @@ module = {
>common = fs/zfs/zfs_lz4.c;
>common = fs/zfs/zfs_sha256.c;
>common = fs/zfs/zfs_fletcher.c;
> +  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/zstd';
>  };
>
>  module = {
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index b5453e006..b8441faef 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -57,6 +57,8 @@
>  #include 
>  #include 
>
> +#include 
> +
>  GRUB_MOD_LICENSE ("GPLv3+");
>
>  #define  ZPOOL_PROP_BOOTFS   "bootfs"
> @@ -291,6 +293,7 @@ static const char *spa_feature_names[] = {
>"com.delphix:embedded_data",
>"com.delphix:extensible_dataset",
>"org.open-zfs:large_blocks",
> +  "org.freebsd:zstd_compress",
>NULL
>  };
>
> @@ -312,6 +315,34 @@ zlib_decompress (void *s, void *d,
>return grub_errno;
>  }
>
> +static grub_err_t
> +zstd_decompress (void *ibuf, void *obuf, grub_size_t isize,
> +  grub_size_t osize)
> +{
> +  grub_size_t zstd_ret;
> +  grub_uint8_t *byte_buf = (grub_uint8_t *) ibuf;
> +
> +  if (isize < 8)
> +  return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data too 
> short");
> +
> +  grub_uint32_t c_len = grub_be_to_cpu32(grub_get_unaligned32(byte_buf));

May I ask you to define c_len at the beginning of the function?
And please fix coding style for function calls.

> +  if (c_len > isize - 8)
> +  return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data announced 
> size overflow");
> +
> +  /* Fix magic number.  */

I think the fix should be explained why it is needed.

> +  byte_buf[4] = 0x28;
> +  byte_buf[5] = 0xb5;
> +  byte_buf[6] = 0x2f;
> +  byte_buf[7] = 0xfd;
> +  zstd_ret = ZSTD_decompress (obuf, osize, byte_buf + 4, c_len + 4);
> +
> +  if (ZSTD_isError (zstd_ret))
> +return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data corrupted 
> (error %d)", (int) zstd_ret);
> +
> +  return GRUB_ERR_NONE;
> +}

Daniel

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

Re: [RESEND V5] ieee1275/ofdisk: retry on open and read failure

[Daniel Kiper]

On Mon, Jun 10, 2024 at 11:35:11AM +0530, Mukesh Kumar Chaurasiya wrote:
> Sometimes, when booting from a very busy SAN, the access to the
> disk can fail and then GRUB will eventually drop to GRUB prompt.
> This scenario is more frequent when deploying many machines at
> the same time using the same SAN.
> This patch aims to force the ofdisk module to retry the open or
> read function for network disks excluding after it fails. We use

excluding? Something is off here...

> DEFAULT_RETRY_TIMEOUT, which is 15 seconds to specify the time it'll
> retry to access the disk before it definitely fails. The timeout can be
> changed by setting the environment variable ofdisk_retry_timeout.
> If the environment variable fails to read, GRUB will consider the
> default value of 15 seconds.
>
> Signed-off-by: Diego Domingos 
> Signed-off-by: Mukesh Kumar Chaurasiya 
> ---
>  docs/grub.texi   |  8 +++
>  grub-core/disk/ieee1275/ofdisk.c | 91 ++--
>  2 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f3bdc2564..9514271fc 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3308,6 +3308,7 @@ These variables have special meaning to GRUB.
>  * net_default_ip::
>  * net_default_mac::
>  * net_default_server::
> +* ofdisk_retry_timeout::
>  * pager::
>  * prefix::
>  * pxe_blksize::
> @@ -3738,6 +3739,13 @@ The default is the value of @samp{color_normal} 
> (@pxref{color_normal}).
>  @xref{Network}.
>
>
> +@node ofdisk_retry_timeout
> +@subsection ofdisk_retry_timeout
> +
> +The time in seconds till which the GRUB will retry to open or read a disk in
> +case of failure to do so. This value defaults to 15 seconds.
> +
> +
>  @node pager
>  @subsection pager
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..d90b9b70b 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -24,6 +24,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +#define RETRY_DEFAULT_TIMEOUT 15
>
>  static char *last_devpath;
>  static grub_ieee1275_ihandle_t last_ihandle;
> @@ -452,7 +455,7 @@ compute_dev_path (const char *name)
>  }
>
>  static grub_err_t
> -grub_ofdisk_open (const char *name, grub_disk_t disk)
> +grub_ofdisk_open_real (const char *name, grub_disk_t disk)
>  {
>grub_ieee1275_phandle_t dev;
>char *devpath;
> @@ -525,6 +528,61 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>return 0;
>  }
>
> +static grub_uint64_t
> +grub_ofdisk_disk_timeout (grub_disk_t disk)
> +{
> +  grub_uint64_t retry = RETRY_DEFAULT_TIMEOUT;
> +  const char *timeout = grub_env_get ("ofdisk_retry_timeout");
> +  const char *timeout_end;
> +
> +  if (grub_strstr (disk->name, "fibre-channel") != NULL ||
> +  grub_strstr (disk->name, "vfc-client") != NULL)
> +{
> +  if (timeout == NULL)
> +{
> +  return retry;
> +}

Please drop redundant curly braces.

> +  retry = grub_strtoul (timeout, _end, 10);
> +  /* Ignore all errors and return default timeout */
> +  if (grub_errno != GRUB_ERR_NONE ||

You can drop grub_errno check from here.

> +  *timeout == '\0' ||
> +  *timeout_end != '\0')
> +{
> +  return RETRY_DEFAULT_TIMEOUT;
> +}

Please drop redundant curly braces.

> +}
> +  else
> +return 0;
> +
> +  return retry;
> +}
> +
> +static grub_err_t
> +grub_ofdisk_open (const char *name, grub_disk_t disk)
> +{
> +  grub_err_t err;
> +  grub_uint64_t timeout = grub_get_time_ms () + (grub_ofdisk_disk_timeout 
> (disk) * 1000);
> +  grub_uint16_t inc = 0;
> +
> +  do
> +{
> +  err = grub_ofdisk_open_real (name, disk);
> +  if (err == GRUB_ERR_UNKNOWN_DEVICE)
> +{
> +  grub_dprintf ("ofdisk", "Failed to open disk %s.\n", name);
> +}

Please drop redundant curly braces here and below in similar cases...

> +  if (grub_get_time_ms () >= timeout)
> +break;
> +  grub_dprintf ("ofdisk", "Retry to open disk %s.\n", name);
> +  /*
> +   * Increase in wait time for subsequent requests

s/in //

> +   * Cur time is used as a random number here

s/Cur time is used as a random number here/Current time is used as a source of 
randomness./

> +   */
> +  grub_millisleep ((32 << ++inc) * (grub_get_time_ms () % 32));
> +} while (1);
> +  return err;
> +}
> +
>  static void
>  grub_ofdisk_close (grub_disk_t disk)
>  {
> @@ -568,8 +626,8 @@ grub_ofdisk_prepare (grub_disk_t disk, grub_disk_addr_t 
> sector)
>  }
>
>  static grub_err_t
> -grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> -   grub_size_t size, char *buf)
> +grub_ofdisk_read_real (grub_disk_t disk, grub_disk_addr_t sector,
> +   grub_size_t size, char *buf)
>  {
>grub_err_t err;
>grub_ssize_t actual;
> @@ -587,6 +645,33 @@ grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t 
> 

Re: [RESEND V2] ieee1275/ofdisk: vscsi lun handling on lun len

[Daniel Kiper]

On Mon, Jun 10, 2024 at 11:29:56AM +0530, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk details
> with pairs of memory addresses and lengths.
>
>   8 bytes 8 bytes
> lun-addr  --->     8 bytes
> ^|  buf-addr | lun-count| > -
> |   |   lun |
> ||  buf-addr | lun-count| | -
>  "len"    | |  ...  |
> ||...   | | -
> | | |   lun |
> ||  buf-addr | lun-count| | -
> V |
>   |---> -
> |   lun |
> -
> |  ...  |
> -
> |   lun |
> -
> The way the expression (args.table + 4 + 8 * i) is used is incorrect and
> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> while (*ptr). Usually, this loop doesn't process any LUNs because it ends
> before checking any as first reported LUN is likely to be 0. The list of
> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya 
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 27 ---
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..1618544a8 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>   grub_ieee1275_cell_t table;
>}
>args;
> +  struct lun_buf {
> +grub_uint64_t *buf_addr;

Again, this is wrong taking into account diagram above. I think it
should be "grub_uint64_t buf_addr". Then you should convert it to the
pointer. Though please be cautious on the 32-bit platforms...

> +grub_uint64_t lun_count;
> +  } *tbl;
>char *buf, *bufptr;
> -  unsigned i;
> +  unsigned int i, j;
>
>if (grub_ieee1275_open (alias->path, ))
>   return;
> @@ -248,17 +252,18 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>   return;
>bufptr = grub_stpcpy (buf, alias->path);
>
> +  tbl = (struct lun_len *) args.table;
>for (i = 0; i < args.nentries; i++)
> - {
> -   grub_uint64_t *ptr;
> -
> -   ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
> -   while (*ptr)
> - {
> -   grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> -   dev_iterate_real (buf, buf);
> - }
> - }
> +{
> +  grub_uint64_t *ptr;
> +
> +  ptr = (grub_uint64_t *)(grub_addr_t) tbl[i].buf_addr;
> +  for (j = 0; j < tbl[i].lun_count; j++)
> +   {
> + grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> + dev_iterate_real (buf, buf);
> +   }
> +}
>grub_ieee1275_close (ihandle);
>grub_free (buf);
>return;

Daniel

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

Re: [PATCH 2/2] docs: document fdtdump command

[Daniel Kiper]

On Wed, Jun 12, 2024 at 01:12:29PM +0200, Tobias Heider wrote:
> Signed-off-by: Tobias Heider 
> ---
>  docs/grub.texi | 26 ++
>  1 file changed, 26 insertions(+)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f3bdc2564..a050dc0fc 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4373,6 +4373,7 @@ you forget a command, you can run the command 
> @command{help}
>  * eval::Evaluate agruments as GRUB commands
>  * export::  Export an environment variable
>  * false::   Do nothing, unsuccessfully
> +* fdtdump:: Retrieve device tree information
>  * fwsetup:: Reboot into the firmware setup menu
>  * gdbinfo:: Provide info for debugging with GDB
>  * gettext:: Translate a string
> @@ -4904,6 +4905,31 @@ such as @code{if} and @code{while} (@pxref{Shell-like 
> scripting}).
>  @end deffn
>
>
> +@node fdtdump
> +@subsection fdtdump
> +
> +@deffn Command fdtdump @
> + [@option{--prop} @var{prop}] @
> + [@option{--set} @var{variable}]
> +Retrieve device tree information.
> +
> +The @command{fdtdump} command returns the value of a property in the device
> +tree provided by the firmware.  The @option{--prop} option determines which
> +property to select.
> +
> +The default action is to print the value of the requested field to the 
> console,
> +but a variable name can be specified with @option{--set} to store the value
> +instead of printing it.
> +
> +For example, this will store and then display the model string.
> +
> +@example
> +fdtdump --prop model --set machine_model
> +echo $machine_model
> +@end example
> +@end deffn

Please merge the doc update with patch #1.

Daniel

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

Re: [PATCH 1/2] loader/efi/fdt: Add fdtdump command to access device tree

[Daniel Kiper]

On Wed, Jun 12, 2024 at 01:12:28PM +0200, Tobias Heider wrote:
> The fdtdump command allows dumping arbitrary device tree properties
> and saving them to a variable similar to the smbios command.
>
> This is useful in scripts where further actions such as selecting a
> kernel or loading another device tree depend on the compatible or
> model values of the device tree provided by the firmware.
>
> For now only the root level properties of the dtb are exposed.
>
> Signed-off-by: Tobias Heider 
> ---
>  grub-core/loader/efi/fdt.c | 51 +-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index 439964b9c..8fa0b3b09 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -1,6 +1,7 @@
>  /*
>   *  GRUB  --  GRand Unified Bootloader
>   *  Copyright (C) 2013-2015  Free Software Foundation, Inc.
> + *  Copyright (C) 2024   Canonical, Ltd.
>   *
>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> @@ -18,15 +19,18 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>
> +static void *fw_fdt;
>  static void *loaded_fdt;
>  static void *fdt;
>
> @@ -36,6 +40,13 @@ static void *fdt;
>   sizeof (FDT_ADDR_CELLS_STRING) + \
>   sizeof (FDT_SIZE_CELLS_STRING))
>
> +static const struct grub_arg_option options_fdtdump[] = {
> +  {"prop",   'p', 0, N_("Get property."), N_("prop"), ARG_TYPE_STRING},
> +  {"set",   '\0', 0, N_("Store the value in the given variable name."),
> + N_("variable"), ARG_TYPE_STRING},
> +  {0, 0, 0, 0, 0, 0}
> +};
> +
>  void *
>  grub_fdt_load (grub_size_t additional_size)
>  {
> @@ -51,7 +62,7 @@ grub_fdt_load (grub_size_t additional_size)
>if (loaded_fdt)
>  raw_fdt = loaded_fdt;
>else
> -raw_fdt = grub_efi_get_firmware_fdt();
> +raw_fdt = fw_fdt;

This change seems suspicious for me. Could you explain why it is needed?

>if (raw_fdt)
>size = grub_fdt_get_totalsize (raw_fdt);
> @@ -167,10 +178,47 @@ out:
>return grub_errno;
>  }
>
> +static grub_err_t
> +grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> + int argc __attribute__ ((unused)),
> + char **argv __attribute__ ((unused)))
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  const char *value = NULL;
> +
> +  if (fw_fdt == NULL)
> +  return grub_error (GRUB_ERR_IO,
> + N_("No device tree found"));
> +
> +  if (state[0].set)
> +{
> +  value = grub_fdt_get_prop(fw_fdt, 0, state[0].arg, NULL);

Missing space before "(".

> +}

Please drop redundant curly braces.

> +  if (value == NULL)
> +return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +   N_("failed to retrieve the prop field"));
> +
> +  if (state[1].set)
> +grub_env_set (state[1].arg, value);
> +  else
> +grub_printf ("%s\n", value);
> +
> +  return GRUB_ERR_NONE;
> +}

Daniel

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

Re: [PATCH v1 2/2] mkimage: adding sbat data into sbat ELF Note on powerpc

[Daniel Kiper]

On Thu, Jun 06, 2024 at 09:44:10PM +0530, Sudhakar Kuppusamy wrote:
> it reads the SBAT data from sbat.csv and create the ELF Note for it then
> store the SBAT data on it while generate image with -s option

May I ask you to use proper English sentences in the commit messages?

> Signed-off-by: Sudhakar Kuppusamy 
> Co-authored-by: Daniel Axtens 
> ---
>  util/mkimage.c | 19 +++
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/util/mkimage.c b/util/mkimage.c
> index 5b7e977ee..ca1718f4d 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -954,8 +954,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 cab be added into powerpc-ieee1275 
> images only"));

s/cab/can/

>
>if (disable_shim_lock)
>  total_module_size += sizeof (struct grub_module_header);
> @@ -1828,6 +1829,16 @@ grub_install_generate_image (const char *dir, const 
> char *prefix,
>  case IMAGE_I386_IEEE1275:
>{
>   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);

Please use tabs and spaces to properly format the code. E.g. one line above is 
correct.

> + grub_util_load_image (sbat_path, sbat);
> +layout.sbat_size = sbat_size;

Ditto.

> +   }
> +
>   if (image_target->id == IMAGE_LOONGSON_ELF)
> {
>   if (comp == GRUB_COMPRESSION_NONE)
> @@ -1839,10 +1850,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, appsig_size, 
> _img, _size,
> +   grub_mkimage_generate_elf32 (image_target, note, appsig_size, sbat, 
> _img, _size,
>  target_addr, );
>   else
> -   grub_mkimage_generate_elf64 (image_target, note, appsig_size, 
> _img, _size,
> +   grub_mkimage_generate_elf64 (image_target, note, appsig_size, sbat, 
> _img, _size,
>  target_addr, );
>}
>break;

I have a feeling this hunk should go to the first patch.

Daniel

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

Re: [PATCH] tests: Switch to requiring exfatprogs from exfat-utils

[Daniel Kiper]

On Sat, Jun 08, 2024 at 11:42:43PM -0500, Glenn Washburn wrote:
> The current Debian stable, now 12, has dropped the exfat-utils package
> that the exfat filesystem test requires to run. There is an exfatprogs
> package that replaces exfat-utils, though it is not a drop-in replacement
> because mkfs.exfat has differing command line option names. Note, that
> we're not yet switching to using the exfat kernel module because this
> allows the testings on kernels that do not have the module.
>
> Update mkfs.exfat usage to adhere to the different exfatprogs usage. Also,
> the exfatprogs mkfs.exfat, following the exfat specification more closely,
> only allows a maximum of 22 bytes of UTF-16 characters in the volume label
> compared to 30 bytes from exfat-utils. So the exfat label test is updated
> accordingly.
>
> Update documentation to not that exfatprogs is now needed and also

s/to not/to note/?

> exfat-fuse, which is needed do the fuse mount.
>
> Signed-off-by: Glenn Washburn 

Otherwise Reviewed-by: Daniel Kiper ...

Daniel

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

Re: [PATCH] tests/util/grub-shell-luks-tester: Fix detached header test getting wrong header path

[Daniel Kiper]

On Sat, Jun 08, 2024 at 11:22:05PM -0500, Glenn Washburn wrote:
> When $detached_header was set 1, $luksdiskfile was set to the luks header
> file path with "${detached_header:-$luksfile}" appended, which evaluates
> to "1". Fix this by using two statements to set $luksdiskfile. The first
> sets it to the header file if $detached_header is set, otherwise leave it
> unset. The second statement sets it to itself if it is already set,
> otherwise it is set to $luksfile.
>
> Fixes: a7b540e6e (tests: Add cryptomount functional test)
> Signed-off-by: Glenn Washburn 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 0/2] Better UEFI firmware handling in tests

[Daniel Kiper]

On Sat, Jun 08, 2024 at 09:42:33PM -0500, Glenn Washburn wrote:
> The first patch moves away from using -bios which appears to be a best
> practice. And the second patch allows the firmware to be found in the
> source directory or system directory, in case the tester does not want
> to use system firmwares or is not on a Debian system.
>
> Glenn
>
> Glenn Washburn (2):
>   tests/util/grub-shell: Use pflash instead of -bios to load UEFI
> firmware
>   tests/util/grub-shell: Add flexibility in QEMU firmware handling

For both patches Reviewed-by: Daniel Kiper ...

Daniel

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

Re: [PATCH] tests/util/grub-shell: Print gdbinfo if on EFI platform

[Daniel Kiper]

On Sat, Jun 08, 2024 at 09:22:31PM -0500, Glenn Washburn wrote:
> Allow using GDB to debug a failing QEMU test. This output does not cause
> issues for tests because it happens before the trim line, and so will be
> ignored.
>
> Signed-off-by: Glenn Washburn 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] configure: Add Debian/Ubuntu dejavu font path

[Daniel Kiper]

On Sat, Jun 08, 2024 at 05:44:15PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] term/ns8250-spcr: Add one more 16550 debug type.

[Daniel Kiper]

On Sat, Jun 08, 2024 at 12:42:21PM -0500, Glenn Washburn wrote:
> On Fri, 7 Jun 2024 23:57:14 +0200
> Udo Steinberg  wrote:
>
> > Hi all,
> >
> > below is a patch for picking up the correct UART address from the ACPI SPCR
> > table on modern AWS bare-metal instances.
> >
> > CC: Benjamin and Glenn for review.
> >
> > Cheers,
> > Udo
>
> LGTM for the changes itself. I don't think you used git to send the

Yeah, it would be perfect if you use git to send patches.

> patch, but this is small enough that perhaps Daniel won't have an issue
> applying it.
>
> Reviewed-by: Glenn Washburn 

Though this time Reviewed-by: Daniel Kiper ...

Daniel

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

Re: [PATCH v16 03/20] libtasn1: disable code not needed in grub

[Daniel Kiper]

On Tue, Jun 11, 2024 at 03:10:56PM +0800, Gary Lin via Grub-devel wrote:
> On Fri, Jun 07, 2024 at 04:14:54PM +0200, Daniel Kiper wrote:
> > On Fri, Jun 07, 2024 at 11:07:31AM +0800, Gary Lin wrote:
> > > On Wed, Jun 05, 2024 at 05:18:32PM +0200, Daniel Kiper wrote:
> > > > On Wed, May 15, 2024 at 01:06:55PM +0800, Gary Lin wrote:
> > > > > From: Daniel Axtens 
> > > > >
> > > > > We don't expect to be able to write ASN.1, only read it,
> > > > > so we can disable some code.
> > > > >
> > > > > Do that with #if 0/#endif, rather than deletion. This means
> > > > > that the difference between upstream and grub is smaller,
> > > > > which should make updating libtasn1 easier in the future.
> > > > >
> > > > > With these exclusions we also avoid the need for minmax.h,
> > > > > which is convenient because it means we don't have to
> > > > > import it from gnulib.
> > > >
> > > > This and two following patches should be put in separate files in the
> > > > grub-core/lib/libtasn1-patches directory. The gnulib is good example
> > > > how it should be done.
> > > >
> > > Those patches are added to libtasn1-patches in another patch with the
> > > title "libtasn1: Add the documentation" which mentions how libtasn1 is
> > > updated.
> >
> > OK, then use them as gnulib does. Of course add the patches one by one
> > to the GRUB source code.
> >
> Does it mean that I should use bootstrap to check out libtasn1 and then
> apply the patches?

No, for time being it is enough to have static libtasn1 with the patches
applied on top of that lib during bootstrap run.

Daniel

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

Re: [PATCH 3/3] kern/efi/mm: Detect calls to grub_efi_drop_alloc() with wrong page counts

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:14:21PM +0100, Mate Kukri wrote:
> Silently keeping entries in the list if the address matches, but the
> page count doesn't is a bad idea, and can lead to double frees.
>
> grub_efi_free_pages() have already freed parts of this block by this
> point, and thus keeping the whole block in the list and freeing it again
> at exit can lead to double frees.
>
> Signed-off-by: Mate Kukri 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 2/3] kern/efi/mm: Change grub_efi_allocate_pages_real() to call semantically correct free function

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:10:50PM +0100, Mate Kukri wrote:
> If the firmware happens to return 0 as an address of allocated pages,
> grub_efi_allocate_pages_real() tries to allocate a new set of pages,
> and then free the ones at address 0.
>
> However at that point grub_efi_store_alloc() wasn't yet called, so
> freeing the pages at 0 using grub_efi_free_pages() which calls
> grub_efi_drop_alloc() isn't necessary, so let's call b->free_pages()
> instead.
>
> The call to grub_efi_drop_alloc() doesn't seem particularly harmful,
> because it seems to do nothing if the allocation it is asked to drop
> isn't on the list, but the call to it is obviously unnecessary here.
>
> Signed-off-by: Mate Kukri 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/3] kern/efi/mm: Change grub_efi_mm_add_regions() to keep track of map allocation size

[Daniel Kiper]

On Wed, Jun 12, 2024 at 04:10:49PM +0100, Mate Kukri wrote:
> If the map was too big for the initial allocation, it was freed and replaced
> with a bigger one, but the free call still used the hard-coded size.
>
> Seems like this wasn't hit for a long time, because most firmware maps
> fit into 12K.
>
> This bug was trigerred on Project Mu firmware with a big memory map, and
> results in the heap getting trashed and the firmware ASSERTING on
> corrupted heap guard values when GRUB exits.
>
> Signed-off-by: Mate Kukri 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v16 03/20] libtasn1: disable code not needed in grub

[Daniel Kiper via Grub-devel]

On Fri, Jun 07, 2024 at 11:07:31AM +0800, Gary Lin wrote:
> On Wed, Jun 05, 2024 at 05:18:32PM +0200, Daniel Kiper wrote:
> > On Wed, May 15, 2024 at 01:06:55PM +0800, Gary Lin wrote:
> > > From: Daniel Axtens 
> > >
> > > We don't expect to be able to write ASN.1, only read it,
> > > so we can disable some code.
> > >
> > > Do that with #if 0/#endif, rather than deletion. This means
> > > that the difference between upstream and grub is smaller,
> > > which should make updating libtasn1 easier in the future.
> > >
> > > With these exclusions we also avoid the need for minmax.h,
> > > which is convenient because it means we don't have to
> > > import it from gnulib.
> >
> > This and two following patches should be put in separate files in the
> > grub-core/lib/libtasn1-patches directory. The gnulib is good example
> > how it should be done.
> >
> Those patches are added to libtasn1-patches in another patch with the
> title "libtasn1: Add the documentation" which mentions how libtasn1 is
> updated.

OK, then use them as gnulib does. Of course add the patches one by one
to the GRUB source code.

Daniel

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

Re: [PATCH] Add Fedora-specific font paths

[Daniel Kiper]

On Thu, May 16, 2024 at 10:03:29PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 
> ---
>  configure.ac | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 84a202c6e..c76a29af4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1847,7 +1847,7 @@ if test "x$with_dejavufont" = x; then
># search in well-known directories
>if test x"$starfield_excuse" = x; then
>   for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz; do
> -   for dir in . /usr/src /usr/share/fonts/X11/misc 
> /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu 
> /usr/share/fonts/truetype /usr/pkg/share/fonts/X11/TTF 
> /usr/local/share/fonts/dejavu /usr/X11R6/lib/X11/fonts/TTF; do
> +   for dir in . /usr/src /usr/share/fonts/X11/misc 
> /usr/share/fonts/truetype/ttf-dejavu /usr/share/fonts/dejavu 
> /usr/share/fonts/truetype /usr/pkg/share/fonts/X11/TTF 
> /usr/local/share/fonts/dejavu /usr/X11R6/lib/X11/fonts/TTF 
> /usr/share/fonts/dejavu-sans-fonts; do
>if test -f "$dir/DejaVuSans.$ext"; then
>  DJVU_FONT_SOURCE="$dir/DejaVuSans.$ext"
>  break 2
> @@ -1875,7 +1875,7 @@ AC_ARG_WITH([unifont],
>
>  if test "x$with_unifont" = x; then
># search in well-known directories
> -  for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz; do
> +  for ext in pcf pcf.gz bdf bdf.gz ttf ttf.gz otf otf.gz; do

I have just realized that this change should be in separate patch. And
even I think the otf fonts should be added for DejaVu fonts as well.
So, I have dropped this hunk. Please repost this part of the patch with
proper commit message.

Daniel

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

Re: [PATCH v16 03/20] libtasn1: disable code not needed in grub

[Daniel Kiper via Grub-devel]

On Wed, May 15, 2024 at 01:06:55PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
> We don't expect to be able to write ASN.1, only read it,
> so we can disable some code.
>
> Do that with #if 0/#endif, rather than deletion. This means
> that the difference between upstream and grub is smaller,
> which should make updating libtasn1 easier in the future.
>
> With these exclusions we also avoid the need for minmax.h,
> which is convenient because it means we don't have to
> import it from gnulib.

This and two following patches should be put in separate files in the
grub-core/lib/libtasn1-patches directory. The gnulib is good example
how it should be done.

Daniel

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

Re: [PATCH v16 02/20] libtasn1: import libtasn1-4.19.0

[Daniel Kiper via Grub-devel]

On Wed, May 15, 2024 at 01:06:54PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
> Import a very trimmed-down set of libtasn1 files:

I hope you merge the latest one...

> pushd /tmp

I would create tmp dir in the GRUB source code and extract libtasn1
there. Then I would remove the tmp dir.

> wget https://ftp.gnu.org/gnu/libtasn1/libtasn1-4.19.0.tar.gz
> tar -xf libtasn1-4.19.0.tar.gz

The "-" is not needed. Please drop it.

> popd
> pushd grub-core/lib
> rm -rf libtasn1
> mkdir libtasn1
> cp /tmp/libtasn1-4.19.0/{README.md,COPYING} libtasn1/
> mkdir libtasn1/lib
> cp 
> /tmp/libtasn1-4.19.0/lib/{coding.c,decoding.c,element.c,element.h,errors.c,gstr.c,gstr.h,int.h,parser_aux.c,parser_aux.h,structure.c,structure.h}
>  libtasn1/lib
> cp /tmp/libtasn1-4.19.0/lib/includes/libtasn1.h ../../include/grub/
> git add libtasn1/ ../../include/grub/libtasn1.h

I am not OK with adding libtasn1.h to the include/grub. Please do not do
that. I think good example how it should be done is in miniLZO lib.

Daniel

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

Re: [PATCH v16 01/20] posix_wrap: tweaks in preparation for libtasn1

[Daniel Kiper via Grub-devel]

On Wed, May 15, 2024 at 01:06:53PM +0800, Gary Lin wrote:
> From: Daniel Axtens 
>
>  - Define SIZEOF_UNSIGNED_LONG_INT, it's the same as
>SIZEOF_UNSIGNED_LONG.
>
>  - Define WORD_BIT, the size in bits of an int. This is a defined
>in the Single Unix Specification and in gnulib's limits.h. gnulib
>assumes it's 32 bits on all our platforms, including 64 bit
>platforms, so we also use that value.
>
>  - Provide strto[u]l[l] preprocessor macros that resolve to
>grub_strto[u]l[l]. To avoid gcrypt redefining strtoul, we
>also define HAVE_STRTOUL here.
>
>  - Implement c-ctype.h and the functions defined in the header.
>
>  - Implement strncat in string.h.
>
> Cc: Vladimir Serbinenko 
> Signed-off-by: Daniel Axtens 
> Signed-off-by: Gary Lin 

Reviewed-by: Daniel Kiper 

A nit below...

> diff --git a/grub-core/lib/posix_wrap/string.h 
> b/grub-core/lib/posix_wrap/string.h
> index 1adb450b5..b0c5928d2 100644
> --- a/grub-core/lib/posix_wrap/string.h
> +++ b/grub-core/lib/posix_wrap/string.h
> @@ -84,6 +84,27 @@ memchr (const void *s, int c, grub_size_t n)
>return grub_memchr (s, c, n);
>  }
>
> +static inline char *
> +strncat(char *dest, const char *src, grub_size_t n)

Missing space before "(".

Daniel

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

Re: [PATCH] cli_lock: Added build option to block command line interface

[Daniel Kiper via Grub-devel]

On Wed, Jan 24, 2024 at 06:26:37AM +, Alec Brown wrote:
> Added functionality to disable command line interface access and editing of 
> GRUB
> menu entries if GRUB image is built with --disable-cli.
>
> Signed-off-by: Alec Brown 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v1 1/1] Fix missing measurements on confidential computing enabled platform

[Daniel Kiper]

On Mon, Jun 03, 2024 at 11:36:25PM +0200, Hector Cao wrote:
> The measurements for confidential computing has been introduced in the commit
> 4c76565b6 (efi/tpm: Add EFI_CC_MEASUREMENT_PROTOCOL support). Recently
> this patch 30708dfe3 (tpm: Disable the tpm verifier if the TPM device
> is not present) has been introduced to optimize the memory usage when
> TPM device is not available on the platform. This patch will prevent the
> tpm module to be loaded on confidential computing platform (for example
> Intel TDX) where no TPM device is available.
>
> In this patch, we propose to load the tpm module for this use case
> by generalizing the tpm feature detection in order to cover CC platforms.
> Basically, do we it by detecting the availability of the EFI protocol
> EFI_CC_MEASUREMENT_PROTOCOL.
>
> Fixes bug : https://savannah.gnu.org/bugs/?65821
>
> Signed-off-by: Hector Cao 

Reviewed-by: Daniel Kiper  but a nit below...

> ---
>  grub-core/commands/efi/tpm.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
> index f250c30db..40845af7a 100644
> --- a/grub-core/commands/efi/tpm.c
> +++ b/grub-core/commands/efi/tpm.c
> @@ -292,6 +292,13 @@ grub_tpm_present (void)
>  {
>grub_efi_handle_t tpm_handle;
>grub_efi_uint8_t protocol_version;
> +  grub_efi_cc_protocol_t *cc;
> +
> +  /* if confidential computing measurement protocol is enabled
> + we consider TPM is present */

This is still not in line with the GRUB coding style. Though I will fix
it for you this time.

> +  cc = grub_efi_locate_protocol (_measurement_guid, NULL);
> +  if (cc != NULL)
> +return 1;
>
>if (!grub_tpm_handle_find (_handle, _version))
>  return 0;

Daniel

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

Re: [PATCH vRESEND] MULTIBOOT: Fix handling of errors in broken aout-kludge

[Daniel Kiper]

On Fri, May 17, 2024 at 10:53:27AM +0300, Vladimir Serbinenko wrote:
> Current code in some codepaths neither discards nor reports
> errors. Properly surface the error
>
> While on it split 2 cases of unrelated variables both named err.
>
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Add missing cast in compile-time byteswaps

[Daniel Kiper]

On Mon, Jun 03, 2024 at 06:33:04PM +0200, Daniel Kiper wrote:
> On Thu, May 16, 2024 at 10:22:58PM +0300, Vladimir Serbinenko wrote:
> > Without them 0x80LL is 32-bit byte-swapped to 0x8000 instead
> > of correct 0x8000

I think it should be added "on 64-bit target"... Right?

> > Signed-off-by: Vladimir Serbinenko 
>
> Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Mark vdev_zaps_v2 and head_errlog as supported

[Daniel Kiper]

On Thu, May 16, 2024 at 10:27:41PM +0300, Vladimir Serbinenko wrote:
> We don't need any actual adjustments as we don't use the affected
> structures
>
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Add missing cast in compile-time byteswaps

[Daniel Kiper]

On Thu, May 16, 2024 at 10:22:58PM +0300, Vladimir Serbinenko wrote:
> Without them 0x80LL is 32-bit byte-swapped to 0x8000 instead
> of correct 0x8000
>
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Add convenience TARGET_CROSS

[Daniel Kiper]

On Thu, May 16, 2024 at 10:07:12PM +0300, Vladimir Serbinenko wrote:
> This allows to set up cross environment with just 3 parameters: target,
> platform and TARGET_CROSS

May I ask you to document this in the INSTALL file?

Daniel

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

Re: [PATCH] Add Fedora-specific font paths

[Daniel Kiper]

On Thu, May 16, 2024 at 10:03:29PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] bfs: Fix improper free() on non-existing files

[Daniel Kiper]

On Thu, May 16, 2024 at 09:37:49PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] grub-mkpasswd-pbkdf2: Simplify the main function implementation

[Daniel Kiper]

On Mon, May 27, 2024 at 08:42:04PM +0800, Tianjia Zhang wrote:
> Allocate memory if needed, while saving the corresponding release
> operation, reducing the amount of code and code complexity.
>
> Signed-off-by: Tianjia Zhang 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] Fix missing measurements on confidential computing enabled platform

[Daniel Kiper]

On Fri, May 31, 2024 at 02:42:38PM +0200, Hector Cao wrote:
> The measurements for confidential computing has been introduced in the commit
> 4c76565b6 (efi/tpm: Add EFI_CC_MEASUREMENT_PROTOCOL support). Recently
> this patch 30708dfe3 (tpm: Disable the tpm verifier if the TPM device
> is not present) has been introduced to optimize the memory usage when
> TPM device is not available on the platform. This patch will prevent the
> tpm module to be loaded on confidential computing platform (for example
> Intel TDX) where no TPM device is available.
>
> In this patch, we propose to load the tpm module for this use case
> by generalizing the tpm feature detection in order to cover CC platforms.
> Basically, do we it by detecting the availability of the EFI protocol
> EFI_CC_MEASUREMENT_PROTOCOL.
>
> Fixes bug : https://savannah.gnu.org/bugs/?65821

Missing SOB...

> ---
>  grub-core/commands/efi/tpm.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
> index f250c30db..386ea3d66 100644
> --- a/grub-core/commands/efi/tpm.c
> +++ b/grub-core/commands/efi/tpm.c
> @@ -292,6 +292,13 @@ grub_tpm_present (void)
>  {
>grub_efi_handle_t tpm_handle;
>grub_efi_uint8_t protocol_version;
> +  grub_efi_cc_protocol_t *cc;
> +
> +  // if confidential computing measurement protocol is enabled
> +  // we consider TPM is present

Please be in line with the GRUB coding style [1].

Otherwise patch LGTM.

> +  cc = grub_efi_locate_protocol (_measurement_guid, NULL);
> +  if (cc != NULL)
> +return 1;

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

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

Re: [PATCH v3 5/5] keccak: Disable acceleration with SSE asm

[Daniel Kiper]

Same comments as for earlier patches...

Thank you for moving this to separate patch.

On Fri, May 24, 2024 at 08:30:06PM +0300, Vladimir Serbinenko wrote:
> ---
>  .../lib/libgcrypt-patches/02_keccak_sse.diff  | 19 +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 grub-core/lib/libgcrypt-patches/02_keccak_sse.diff
>
> diff --git a/grub-core/lib/libgcrypt-patches/02_keccak_sse.diff 
> b/grub-core/lib/libgcrypt-patches/02_keccak_sse.diff
> new file mode 100644
> index 0..980ebb2b7
> --- /dev/null
> +++ b/grub-core/lib/libgcrypt-patches/02_keccak_sse.diff
> @@ -0,0 +1,19 @@
> +commit b0cf06271da5fe20360953a53a47c69da89669cd
> +Author: Vladimir Serbinenko 
> +Date:   Sun Apr 7 06:33:11 2024 +0300
> +
> +keccak: Disable acceleration with SSE asm

Could you explain in the commit message here and above why this change
is needed?

> +diff --git a/grub-core/lib/libgcrypt/cipher/keccak.c 
> b/grub-core/lib/libgcrypt/cipher/keccak.c
> +index 11e64b3e7..8b570263b 100644
> +--- a/grub-core/lib/libgcrypt/cipher/keccak.c
>  b/grub-core/lib/libgcrypt/cipher/keccak.c
> +@@ -251,7 +251,7 @@ keccak_absorb_lane32bi(u32 *lane, u32 x0, u32 x1)
> + /* Construct generic 64-bit implementation. */
> + #ifdef USE_64BIT
> +
> +-#if __GNUC__ >= 4 && defined(__x86_64__)
> ++#if __GNUC__ >= 4 && defined(__x86_64__) && 0
> +
> + static inline void absorb_lanes64_8(u64 *dst, const byte *in)
> + {

Daniel

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

Re: [PATCH v3 4/5] Add DSA and RSA SEXP tests

[Daniel Kiper]

Same comments as for earlier patches...

On Fri, May 24, 2024 at 08:30:05PM +0300, Vladimir Serbinenko wrote:
> ---
>  grub-core/tests/dsa_sexp_test.c | 107 
>  grub-core/tests/rsa_sexp_test.c |  81 
>  2 files changed, 188 insertions(+)
>  create mode 100644 grub-core/tests/dsa_sexp_test.c
>  create mode 100644 grub-core/tests/rsa_sexp_test.c
>
> diff --git a/grub-core/tests/dsa_sexp_test.c b/grub-core/tests/dsa_sexp_test.c
> new file mode 100644
> index 0..ea233bf0c
> --- /dev/null
> +++ b/grub-core/tests/dsa_sexp_test.c
> @@ -0,0 +1,107 @@

Should not we add a license here?

> +#include 
> +#include 
> +#include 
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static char pubkey_dump[] = {
> +  0x28, 0x31, 0x30, 0x3a, 0x70, 0x75, 0x62, 0x6c,

Spaces at the end here and below seems redundant.
The git complains about them.

> +  0x69, 0x63, 0x2d, 0x6b, 0x65, 0x79, 0x28, 0x33,
> +  0x3a, 0x64, 0x73, 0x61, 0x28, 0x31, 0x3a, 0x70,
> +  0x31, 0x32, 0x39, 0x3a, 0x00, 0xc0, 0x50, 0x14,
> +  0x4c, 0x97, 0x10, 0x69, 0x07, 0xa7, 0xe9, 0x2b,
> +  0xe5, 0xc6, 0x88, 0xe1, 0x6d, 0xd8, 0x38, 0x28,
> +  0x09, 0x49, 0x5b, 0xe8, 0xa3, 0x04, 0xb8, 0xc4,
> +  0x6e, 0x98, 0xc1, 0xc2, 0xb0, 0x2a, 0xe0, 0xe2,
> +  0x1a, 0x30, 0xd2, 0xdb, 0x45, 0x1a, 0x88, 0x80,
> +  0x28, 0x24, 0xb0, 0xbf, 0xc2, 0xbd, 0xe9, 0xf6,
> +  0x9d, 0xa2, 0x01, 0x94, 0xe6, 0x7f, 0xa0, 0xb6,
> +  0xe4, 0x39, 0xfc, 0x54, 0xba, 0x99, 0xb6, 0xbe,
> +  0x39, 0xee, 0xa5, 0xd9, 0xa0, 0x35, 0x3c, 0x2d,
> +  0x3e, 0x96, 0xc3, 0x96, 0xa5, 0x0d, 0x2b, 0xbf,
> +  0x3b, 0xa3, 0xe2, 0xe8, 0x89, 0xed, 0x60, 0xe0,
> +  0x43, 0x61, 0xb6, 0x73, 0xf6, 0xa7, 0xb4, 0x56,
> +  0x76, 0x04, 0xf7, 0x8b, 0xf1, 0x84, 0xaa, 0x3e,
> +  0xe0, 0x08, 0xad, 0xdd, 0xc2, 0x36, 0xfd, 0x3d,
> +  0xd0, 0xad, 0xf4, 0x3a, 0x7e, 0x80, 0x8c, 0x52,
> +  0x2b, 0x04, 0xa8, 0x03, 0x27, 0x29, 0x28, 0x31,
> +  0x3a, 0x71, 0x32, 0x31, 0x3a, 0x00, 0xd5, 0x34,
> +  0xd2, 0xc5, 0x1c, 0x26, 0xdf, 0xb0, 0xba, 0x78,
> +  0x75, 0xe5, 0xe9, 0x36, 0x6b, 0x04, 0x03, 0xe2,
> +  0x57, 0x3f, 0x29, 0x28, 0x31, 0x3a, 0x67, 0x31,
> +  0x32, 0x38, 0x3a, 0x3b, 0xa0, 0xac, 0xa3, 0xa1,
> +  0xd1, 0x04, 0x23, 0x5f, 0x9f, 0xbc, 0x6d, 0x9e,
> +  0x88, 0x2a, 0x28, 0xc1, 0x48, 0xaf, 0xa5, 0x17,
> +  0x59, 0x3a, 0x17, 0x33, 0x56, 0xaa, 0x8d, 0x27,
> +  0x64, 0xfe, 0x8e, 0x8a, 0x2e, 0xba, 0xf2, 0x66,
> +  0xcc, 0x66, 0xbd, 0xa4, 0xfe, 0xa9, 0x07, 0x0d,
> +  0xae, 0x8c, 0x9f, 0x70, 0xf7, 0x87, 0xaa, 0x01,
> +  0x47, 0x6b, 0xf9, 0x0f, 0x09, 0x18, 0x42, 0x76,
> +  0xc4, 0xa3, 0xb9, 0x55, 0x11, 0x8d, 0xa3, 0xa5,
> +  0x69, 0x30, 0x91, 0xb7, 0x03, 0xef, 0x7f, 0x12,
> +  0xe6, 0xb9, 0x78, 0x73, 0xe0, 0xc0, 0x4f, 0xc6,
> +  0xd9, 0x43, 0x99, 0x95, 0x0b, 0x4d, 0x58, 0xd3,
> +  0x6b, 0x76, 0xb0, 0x6a, 0xcf, 0x68, 0x6d, 0xf0,
> +  0xd9, 0xc1, 0x88, 0x43, 0x9d, 0xf9, 0x04, 0xcb,
> +  0xc9, 0x82, 0x6c, 0xee, 0xd4, 0x9c, 0xbd, 0x1c,
> +  0x4d, 0x54, 0x29, 0x83, 0xa9, 0x5e, 0xaa, 0x10,
> +  0xa7, 0xc1, 0x04, 0x29, 0x28, 0x31, 0x3a, 0x79,
> +  0x31, 0x32, 0x39, 0x3a, 0x00, 0x82, 0x33, 0xf1,
> +  0x91, 0xe3, 0xf2, 0x12, 0x93, 0x5a, 0xed, 0x0c,
> +  0x9d, 0xec, 0x67, 0xaa, 0xa7, 0x97, 0x7f, 0x9f,
> +  0x5e, 0xef, 0x6a, 0x3e, 0xa4, 0x7f, 0x9b, 0xed,
> +  0x65, 0xd7, 0xba, 0x40, 0x6d, 0xe1, 0xde, 0xc1,
> +  0x14, 0x4c, 0x9b, 0x28, 0x5c, 0x03, 0x8e, 0x1a,
> +  0xd4, 0x1b, 0x80, 0x1b, 0x07, 0xd6, 0x84, 0x04,
> +  0x49, 0x6c, 0x1b, 0x08, 0x84, 0x15, 0x54, 0x62,
> +  0xca, 0xd5, 0x75, 0xff, 0xc8, 0xb3, 0x81, 0x76,
> +  0x82, 0x91, 0x35, 0x80, 0x20, 0x73, 0x2a, 0x21,
> +  0xca, 0x22, 0x06, 0xa7, 0x73, 0x99, 0x75, 0x7e,
> +  0x5e, 0xa6, 0x09, 0x59, 0x66, 0x2c, 0xcd, 0xb1,
> +  0x8d, 0x3b, 0xc0, 0x68, 0xc5, 0x41, 0xa0, 0x9d,
> +  0x82, 0x15, 0xc4, 0xdd, 0x47, 0x1c, 0x5b, 0xa9,
> +  0x74, 0x18, 0xaf, 0x72, 0x63, 0x6b, 0x0a, 0x4e,
> +  0x95, 0x09, 0x7a, 0xb5, 0x4b, 0x98, 0x85, 0xb9,
> +  0x6d, 0x9d, 0x3b, 0x73, 0x8c, 0x29, 0x29, 0x29,
> +};
> +
> +static char sig_dump[] = {
> +  0x28, 0x37, 0x3a, 0x73, 0x69, 0x67, 0x2d, 0x76,
> +  0x61, 0x6c, 0x28, 0x33, 0x3a, 0x64, 0x73, 0x61,
> +  0x28, 0x31, 0x3a, 0x72, 0x32, 0x30, 0x3a, 0xb6,
> +  0x60, 0x37, 0xef, 0x02, 0x7c, 0x7c, 0x6e, 0x4f,
> +  0x66, 0x8c, 0x7c, 0x26, 0x77, 0xd9, 0x33, 0x90,
> +  0xba, 0x7c, 0xfb, 0x29, 0x28, 0x31, 0x3a, 0x73,
> +  0x32, 0x30, 0x3a, 0x83, 0xc0, 0x84, 0x72, 0xc6,
> +  0x1c, 0x85, 0x6f, 0x8b, 0x9b, 0xb0, 0x38, 0x38,
> +  0xb2, 0xb6, 0xdf, 0x1c, 0x52, 0x96, 0x1b, 0x29,
> +  0x29, 0x29,
> +};
> +
> +extern gcry_pk_spec_t _gcry_pubkey_spec_dsa;
> +
> +static void
> +dsa_sexp_test (void)
> +{
> +  gcry_sexp_t sign_parms, sign_parms_invalid, pubkey, sig;
> +  int rc;
> +  grub_size_t errof;
> +
> +  rc = _gcry_sexp_build (_parms_invalid, ,
> + "(data (value \"hi\"))\n");
> +  grub_test_assert (rc == 0, "sexp build failed");
> +
> +  rc = _gcry_sexp_build (_parms, ,
> + "(data (value \"hello\"))\n");
> +  grub_test_assert (rc == 0, "sexp build failed");
> +  rc = _gcry_sexp_new (, 

Re: [PATCH v3 3/5] Adjust import script, definitions and API users for libgcrypt 1.10

[Daniel Kiper]

Again, missing commit message and SOB.

Could you add to the GRUB Developers Manual description how to upgrade
the libgcrypt?

On Fri, May 24, 2024 at 08:30:04PM +0300, Vladimir Serbinenko wrote:
> ---
>  autogen.sh |   5 +
>  conf/Makefile.common   |   4 +-
>  grub-core/Makefile.core.def|  36 ++-
>  grub-core/commands/hashsum.c   |   2 +-
>  grub-core/commands/legacycfg.c |   6 +-
>  grub-core/commands/pgp.c   | 114 +++--
>  grub-core/commands/xnu_uuid.c  |   2 +-
>  grub-core/disk/cryptodisk.c|   2 +-
>  grub-core/io/gzio.c|   2 +-
>  grub-core/io/lzopio.c  |   2 +-
>  grub-core/lib/adler32.c|  21 +-
>  grub-core/lib/crc64.c  |  24 +-
>  grub-core/lib/crypto.c | 245 ++-
>  grub-core/lib/libgcrypt-patches/01_md.diff | 141 +++

s/01_md.diff/01-md.patch/ We use "patch" extension for the patches.
And should not this be a separate patch in the series?

>  grub-core/lib/libgcrypt/src/gcrypt-int.h   |  51 
>  grub-core/lib/libgcrypt_wrap/cipher_wrap.h |  12 +-
>  grub-core/lib/libgcrypt_wrap/mem.c |  83 ++-
>  grub-core/lib/xzembed/xz_dec_stream.c  |  18 +-
>  include/grub/crypto.h  | 261 +
>  include/grub/gcrypt/gpg-error.h|   9 +
>  util/grub-fstest.c |   2 +-
>  util/import_gcry.py| 160 -
>  util/import_gcrypt_inth.sed|  17 ++
>  util/import_gcrypth.sed|   2 -
>  24 files changed, 941 insertions(+), 280 deletions(-)
>  create mode 100644 grub-core/lib/libgcrypt-patches/01_md.diff
>  create mode 100644 util/import_gcrypt_inth.sed

[...]

> diff --git a/util/import_gcry.py b/util/import_gcry.py
> index 2b3322d3a..02f727fca 100644
> --- a/util/import_gcry.py
> +++ b/util/import_gcry.py

[...]

> @@ -148,14 +188,10 @@ for cipher_file in cipher_files:
>  iscipher = False
>  iscryptostart = False
>  iscomma = False
> -isglue = False
>  skip_statement = False
> +skip_comma = False
>  if isc:
> -modname = cipher_file [0:len(cipher_file) - 2]
> -if re.match (".*-glue$", modname):
> -modname = modname.replace ("-glue", "")
> -isglue = True
> -modname = "gcry_%s" % modname
> +modname = "gcry_%s" % 
> cipher_file.removesuffix(".c").removesuffix("-glue").replace("-", "_")

The removesuffix is not support by older Python than 3.9.

Additionally, GCC 12.2.0 complains:

  gcc -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 
-I./include -DGRUB_FILE=\"grub-core/lib/libgcrypt-grub/src/const-time.c\" -I. 
-I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  
-I./grub-core/lib/libgcrypt_wrap -I./grub-core/lib/posix_wrap 
-D_GCRYPT_IN_LIBGCRYPT=1 -D_GCRYPT_CONFIG_H_INCLUDED=1 -DHAVE_STRTOUL=1 
-I./include/grub/gcrypt -D_FILE_OFFSET_BITS=64 -std=gnu99 -fno-common  -Wall -W 
-Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment 
-Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal 
-Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit 
-Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces 
-Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type 
-Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas 
-Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  
-Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes 
-Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self 
-Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull 
-Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros 
-Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs 
-Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  
-Wno-error=sign-compare -Wno-error=shift-count-overflow 
-Wno-missing-field-initializers -Wno-redundant-decls -Wno-undef -fno-builtin  
-MT grub-core/lib/libgcrypt-grub/src/libgrubgcry_a-const-time.o -MD -MP -MF 
grub-core/lib/libgcrypt-grub/src/.deps-util/libgrubgcry_a-const-time.Tpo -c -o 
grub-core/lib/libgcrypt-grub/src/libgrubgcry_a-const-time.o `test -f 
'grub-core/lib/libgcrypt-grub/src/const-time.c' || echo 
'./'`grub-core/lib/libgcrypt-grub/src/const-time.c
  In file included from grub-core/lib/libgcrypt-grub/src/const-time.c:22:
  ./grub-core/lib/posix_wrap/stdlib.h:70: error: "HAVE_STRTOUL" redefined 
[-Werror]
 70 | #define HAVE_STRTOUL
|
  : note: this is the location of the previous definition

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org

Re: [PATCH v3 2/5] Import b64dec from gpg-error

[Daniel Kiper]

Why this file is needed? From which GPG version this file come from?
And missing SOB...

On Fri, May 24, 2024 at 08:30:03PM +0300, Vladimir Serbinenko wrote:
> ---
>  grub-core/lib/b64dec.c | 293 +
>  1 file changed, 293 insertions(+)
>  create mode 100644 grub-core/lib/b64dec.c

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

Re: [PATCH v3 1/5] Import libgcrypt 1.10.3

[Daniel Kiper]

Could you add your SOB and a few words to the commit message explaining
why this patch is needed.

It would be nice if you add a cover letter to this patch set too.

Daniel

On Fri, May 24, 2024 at 08:30:02PM +0300, Vladimir Serbinenko wrote:
> ---
>  grub-core/lib/libgcrypt/AUTHORS   |   264 +
>  grub-core/lib/libgcrypt/COPYING   |   340 +
>  grub-core/lib/libgcrypt/COPYING.LIB   |   510 +
>  grub-core/lib/libgcrypt/LICENSES  |   287 +
>  grub-core/lib/libgcrypt/README|   280 +
>  grub-core/lib/libgcrypt/README.GIT|49 +
>  grub-core/lib/libgcrypt/THANKS|   168 +
>  grub-core/lib/libgcrypt/VERSION   | 1 +
>  grub-core/lib/libgcrypt/cipher/ChangeLog  |  3990 ---
>  grub-core/lib/libgcrypt/cipher/ChangeLog-2011 |40 +-
>  grub-core/lib/libgcrypt/cipher/Makefile.am|   271 +-
>  grub-core/lib/libgcrypt/cipher/Manifest   |73 -
>  grub-core/lib/libgcrypt/cipher/ac.c   |  3301 --

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

Re: [PATCH v2] ieee1275 radix support added for KVM on power

[Daniel Kiper via Grub-devel]

On Thu, May 23, 2024 at 06:43:14PM +0530, Avnish Chouhan wrote:
> This patch adds support for Radix, Xive and Radix_gtse in Options
> vector5 which is required for KVM LPARs. KVM LPARs ONLY support
> Radix and not the Hash. Not enabling Radix on any PowerVM KVM LPARs
> will result in boot failure.
>
> Signed-off-by: Avnish Chouhan 
> Reviewed-by: Daniel Kiper 

I am OK with this RB now. However, you should not add any tags on
behalf of anybody without their explicit permission.

Daniel

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

Re: [PATCH] ieee1275 radix support added for KVM on power

[Daniel Kiper]

On Mon, Dec 18, 2023 at 08:02:34PM +0530, Avnish Chouhan wrote:
> This patch adds support for Radix, Xive and Radix_gtse in Options
> vector5 which is required for KVM LPARs. KVM LPARs ONLY support
> Radix and not the Hash. Not enabling Radix on any PowerVM KVM LPARs
> will result in boot failure.
>
> Signed-off-by: Avnish Chouhan 
> ---
>  grub-core/kern/ieee1275/init.c | 68 
> +-
>  1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
> index fb7d1a3..94bbf86 100644
> --- a/grub-core/kern/ieee1275/init.c
> +++ b/grub-core/kern/ieee1275/init.c
> @@ -113,6 +113,17 @@ grub_addr_t grub_ieee1275_original_stack;
>  #define DRC_INFO0x40
>  #define BYTE22  (DY_MEM_V2 | DRC_INFO)
>
> +/* For ibm,arch-vec-5-platform-support */
> +
> +#define XIVE_INDEX   0x17
> +#define MMU_INDEX0x18
> +#define RADIX_GTSE_INDEX 0x1a
> +#define RADIX_ENABLED0x40
> +#define XIVE_ENABLED 0x40
> +#define HASH_ENABLED 0x0

This looks like a mistake. Is not it? If it is correct I would do s/0x0/0x00/.

> +#define MAX_SUPPORTED0xC0
> +#define RADIX_GTSE_ENABLED   0x40
> +
>  void
>  grub_exit (void)
>  {
> @@ -737,6 +748,10 @@ struct option_vector5
>grub_uint32_t platform_facilities;
>grub_uint8_t sub_processors;
>grub_uint8_t byte22;
> +  grub_uint8_t xive;
> +  grub_uint8_t mmu;
> +  grub_uint8_t hpt_ext;
> +  grub_uint8_t radix_gtse;
>  } GRUB_PACKED;
>
>  struct pvr_entry
> @@ -775,6 +790,13 @@ grub_ieee1275_ibm_cas (void)
>  {
>int rc;
>grub_ieee1275_ihandle_t root;
> +  grub_uint8_t ibm_arch_platform_support[8];
> +  grub_ssize_t actual;
> +  grub_uint8_t xive_support = 0;
> +  grub_uint8_t mmu_support = 0;
> +  grub_uint8_t radix_gtse_support = 0;
> +  int i=0;
> +  int prop_len=8;

Wrong coding style, some missing spaces before and after "=".

>struct cas_args
>{
>  struct grub_ieee1275_common_hdr common;
> @@ -783,6 +805,50 @@ grub_ieee1275_ibm_cas (void)
>  grub_ieee1275_cell_t cas_addr;
>  grub_ieee1275_cell_t result;
>} args;
> +
> +  grub_ieee1275_get_integer_property (grub_ieee1275_chosen,
> +  "ibm,arch-vec-5-platform-support",
> +  ibm_arch_platform_support,
> +  sizeof (ibm_arch_platform_support),
> +  );
> +
> +  for (i=0; i +{
> +  switch (ibm_arch_platform_support[i])
> +{
> +  case XIVE_INDEX:
> +if (ibm_arch_platform_support[i+1] & MAX_SUPPORTED)

Similar thing here...

> +  xive_support = XIVE_ENABLED;
> +else
> +  xive_support = 0;
> +break;
> +
> +  case MMU_INDEX:
> +if (ibm_arch_platform_support[i+1] & MAX_SUPPORTED)

Ditto.

> +  mmu_support = RADIX_ENABLED;
> +else
> +  mmu_support = HASH_ENABLED;
> +break;
> +
> +  case RADIX_GTSE_INDEX:
> +if (mmu_support == RADIX_ENABLED)
> +  {
> +radix_gtse_support = ibm_arch_platform_support[i+1] & 
> RADIX_GTSE_ENABLED;

Ditto. And you can drop curly braces here and below.

> +  }
> +else
> +  {
> +radix_gtse_support = 0;
> +  }
> +break;
> +
> +  default:
> +/* Ignoring the other indexes of ibm,arch-vec-5-platform-support 
> */
> +break;
> +}
> +  /* Skipping the property value */
> +  i++;
> +}
> +
>struct cas_vector vector =
>{
>  .pvr_list = { { 0x, 0x } }, /* any processor */
> @@ -799,7 +865,7 @@ grub_ieee1275_ibm_cas (void)
>  .vec4 = 0x0001, /* set required minimum capacity % to the lowest value */
>  .vec5_size = 1 + sizeof (struct option_vector5) - 2,
>  .vec5 = {
> -  0, BYTE2, 0, CMO, ASSOCIATIVITY, BIN_OPTS, 0, 0, MAX_CPU, 0, 0, 
> PLATFORM_FACILITIES, SUB_PROCESSORS, BYTE22
> +  0, BYTE2, 0, CMO, ASSOCIATIVITY, BIN_OPTS, 0, 0, MAX_CPU, 0, 0, 
> PLATFORM_FACILITIES, SUB_PROCESSORS, BYTE22, xive_support, mmu_support, 0, 
> radix_gtse_support
>  }
>};

Daniel

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

Re: [PATCH v2][Bugfix] util/grub.d/00_header.in: quote background image pathname in output

[Daniel Kiper]

On Sun, May 19, 2024 at 05:50:10PM +0200, Pascal Hambourg wrote:
> This is required if the pathname contains spaces or grub shell
> metacharacters, else the generated config file check will fail.
>
> Signed-off-by: Pascal Hambourg 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH 1/2 v2] LVM Cachevol and Integrity volumes break entire LVM VG

[Daniel Kiper]

On Fri, Apr 26, 2024 at 08:59:21PM -0400, Patrick Plenefisch wrote:
> From 42252f253ac685bbc7cea1f5c89146eeeaa364f0 Mon Sep 17 00:00:00 2001
> From: Patrick Plenefisch 
> Date: Sun, 18 Feb 2024 18:29:43 -0500
> Subject: [PATCH 1/2] disk/lvm: Make cache_lv more generic as
>  ignored_feature_lv

Please explain in the commit message why this patch is needed.

Additionally, patch set misses cover letter.

Daniel

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

Re: [PATCH 2/2 v2] LVM Cachevol and Integrity volumes break entire LVM VG

[Daniel Kiper]

On Fri, Apr 26, 2024 at 09:00:08PM -0400, Patrick Plenefisch wrote:
> From 8cfb6dbb011d3773b90a3cbb8561616a2fb5955f Mon Sep 17 00:00:00 2001
> From: Patrick Plenefisch 
> Date: Sun, 18 Feb 2024 18:36:05 -0500
> Subject: [PATCH 2/2] lvm: Add support for cachevol and integrity lv

May I ask you to use git "send-email" to send patches?

> lv matching must be done after processing, as integrity

After processing of what? Could you expand this commit message?

> volumes may have several levels that the segments must be
> shifted through
>
> pv matching must be completely finished before validating a
> volume, otherwise referenced raid stripes may not have pv
> data applied yet
>
> Signed-off-by: Patrick Plenefisch 
> ---
>  grub-core/disk/diskfilter.c |  6 ++-
>  grub-core/disk/lvm.c| 83 +++--
>  2 files changed, 56 insertions(+), 33 deletions(-)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 21e239511..dc3bd943b 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -966,8 +966,6 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>
>for (lv = vg->lvs; lv; lv = lv->next)
>  {
> -  grub_err_t err;
> -
>/* RAID 1 and single-disk RAID 0 don't use a chunksize but code
>   assumes one so set one. */
>for (i = 0; i < lv->segment_count; i++)
> @@ -979,6 +977,10 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>&& lv->segments[i].stripe_size == 0)
>  lv->segments[i].stripe_size = 64;
>   }
> +}
> +  for (lv = vg->lvs; lv; lv = lv->next)
> +{
> +  grub_err_t err;

I am OK with this change but it does not seem to belong to this patch.

>err = validate_lv(lv);
>if (err)
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 10bc965a4..7615a507a 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -805,13 +805,27 @@ grub_lvm_detect (grub_disk_t disk,
>seg->nodes[seg->node_count - 1].name = tmp;
>   }
>  }
> +  /* Cache and integrity LVs have extra parts that
> + we can ignore for our read-only access */

Please fix this comment [1].

>else if (grub_memcmp (p, "cache\"",
> -   sizeof ("cache\"") - 1) == 0)
> +   sizeof ("cache\"") - 1) == 0
> +   || grub_memcmp (p, "cache+CACHE_USES_CACHEVOL\"",
> +   sizeof ("cache+CACHE_USES_CACHEVOL\"") - 1) == 0
> +   || grub_memcmp (p, "integrity\"",
> +   sizeof ("integrity\"") - 1) == 0)
>  {
>struct ignored_feature_lv *ignored_feature = NULL;
>
>char *p2, *p3;
>grub_size_t sz;
> +#ifdef GRUB_UTIL
> +  p2 = grub_strchr (p, '"');
> +  if (p2)
> + *p2 = 0;

I think this would be more clear:
  *p2 = '\0';

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

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

Re: [PATCH 3/3] keccak: Disable acceleration with SSE asm

[Daniel Kiper]

On Fri, May 17, 2024 at 02:24:50PM +0300, Vladimir 'phcoder' Serbinenko wrote:
> Le ven. 17 mai 2024, 14:15, Daniel Kiper  a écrit :
>  Why is this patch needed? Should not we disable SSE using compiler
>  flags?
> We do but the code in question uses SSE on every x64 platform. So we need to
> disable this acceleration. At least for now. If we want to enable SSE (maybe 
> we
> should) we can revisit this but this is a separate discussion and separate
> patchset

Please explain the problem with SSE in the commit message. Otherwise we
will shortly forget why it has been disabled.

Additionally, I think all libgcrypt patches like this one should
land in the grub-core/lib/libgcrypt-patches and be applied during
bootstrap or so. This will make us in line with gnulib and gettext
patching mechanism.

Daniel

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

Re: [PATCH 0/2] Two small fixes to gzip

[Daniel Kiper]

Hey,

On Sat, May 18, 2024 at 02:52:20PM +1000, Daniel Axtens wrote:
> Hi Daniel,
>
> > Reviewed-by: Daniel Kiper  for both patches...
>
> Thank you!
>
> > I assume I can add your SOB on your behalf...
>
> Oh! Yes! Sorry, 2 years of closed source development have apparently caused 
> me to lose my 'git commit -s' habits!
>
> The sign-off for both patches is:
> Signed-off-by: Daniel Axtens 

No worries. Thank you!

Daniel

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

Re: [PATCH v14 NEW ALT 1/3] safemath: Add ALIGN_UP_OVF() that checks for {over,under}flow

[Daniel Kiper via Grub-devel]

On Sun, May 19, 2024 at 11:48:24AM +0800, Gao Xiang wrote:
> The following EROFS patch will use this helper to handle overflow
> ALIGN_UP() cases.
>
> Signed-off-by: Gao Xiang 

I think Vladimir is right.

Reviewed-by: Daniel Kiper 

Please post v15 an I will merge it this week.

Thank you for adding EROFS support!

Daniel

> ---
> Hi Daniel,
>
> I've also made another patch to conform Vladimir's and
> Boyang's comments as much as I can.
>
> Thanks,
> Gao Xiang
>
>  include/grub/safemath.h | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/grub/safemath.h b/include/grub/safemath.h
> index fbd9b5925..bb8a5b39c 100644
> --- a/include/grub/safemath.h
> +++ b/include/grub/safemath.h
> @@ -32,6 +32,22 @@
>
>  #define grub_cast(a, res)grub_add ((a), 0, (res))
>
> +/*
> + * It's caller's responsibility that `align` should not equal to 0 and
> + * it must be a power of 2.
> + */
> +#define ALIGN_UP_OVF(v, align, res)  \
> +({   \
> +  bool __failed; \
> +  typeof(v) __a = ((typeof(v))(align) - 1);  \
> + \
> +  __failed = grub_add (v, __a, res); \
> +  if (__failed == false) \
> +{\
> +  *(res) &= ~__a;\
> +}\
> +__failed;})
> +
>  #else
>  #error gcc 5.1 or newer or clang 8.0 or newer is required
>  #endif

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

Re: [PATCH] docs/grub.texi: fix spelling mistakes

[Daniel Kiper via Grub-devel]

On Mon, May 20, 2024 at 08:58:24AM +0100, Jonathan Davies wrote:
> Signed-off-by: Jonathan Davies 

Reviewed-by: Daniel Kiper 

Thank you for fixing these mistakes.

Daniel

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

Re: [PATCH 3/3] keccak: Disable acceleration with SSE asm

[Daniel Kiper]

Why is this patch needed? Should not we disable SSE using compiler flags?

Daniel

On Thu, May 16, 2024 at 09:27:43PM +0300, Vladimir Serbinenko wrote:
> ---
>  grub-core/lib/libgcrypt/cipher/keccak.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/lib/libgcrypt/cipher/keccak.c 
> b/grub-core/lib/libgcrypt/cipher/keccak.c
> index 11e64b3e7..8b570263b 100644
> --- a/grub-core/lib/libgcrypt/cipher/keccak.c
> +++ b/grub-core/lib/libgcrypt/cipher/keccak.c
> @@ -251,7 +251,7 @@ keccak_absorb_lane32bi(u32 *lane, u32 x0, u32 x1)
>  /* Construct generic 64-bit implementation. */
>  #ifdef USE_64BIT
>
> -#if __GNUC__ >= 4 && defined(__x86_64__)
> +#if __GNUC__ >= 4 && defined(__x86_64__) && 0
>
>  static inline void absorb_lanes64_8(u64 *dst, const byte *in)
>  {

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

Re: [PATCH 2/3] Adjust import script, definitions and API users for libgcrypt 1.10

[Daniel Kiper]

Please add a commit message and SOB...

Additionally, please expand "Updating external code" in GRUB dev
documentation with a description how to update libgcrypt.

On Thu, May 16, 2024 at 09:27:42PM +0300, Vladimir Serbinenko wrote:
> ---
>  autogen.sh |   1 +

Should not autogen.sh remove libgcrypt-grub when it exists?
If yes then it should be separate patch.

>  conf/Makefile.common   |   4 +-
>  grub-core/Makefile.core.def|  36 +-
>  grub-core/commands/hashsum.c   |   2 +-
>  grub-core/commands/legacycfg.c |   6 +-
>  grub-core/commands/pgp.c   | 114 +---
>  grub-core/commands/xnu_uuid.c  |   2 +-
>  grub-core/disk/cryptodisk.c|   2 +-
>  grub-core/io/gzio.c|   2 +-
>  grub-core/io/lzopio.c  |   2 +-
>  grub-core/lib/adler32.c|  21 +-
>  grub-core/lib/b64dec.c | 293 +
>  grub-core/lib/crc64.c  |  24 +-
>  grub-core/lib/crypto.c | 245 +++-
>  grub-core/lib/libgcrypt/src/gcrypt-int.h   |  51 --
>  grub-core/lib/libgcrypt_wrap/cipher_wrap.h |  12 +-
>  grub-core/lib/libgcrypt_wrap/md.c  | 687 +
>  grub-core/lib/libgcrypt_wrap/mem.c |  83 ++-
>  grub-core/lib/xzembed/xz_dec_stream.c  |  18 +-
>  grub-core/tests/dsa_sexp_test.c| 107 
>  grub-core/tests/rsa_sexp_test.c|  81 +++

I would add these tests in separate patches.

>  include/grub/crypto.h  | 261 ++--
>  include/grub/gcrypt/gpg-error.h|   9 +
>  util/grub-fstest.c |   2 +-
>  util/import_gcry.py| 122 ++--
>  util/import_gcrypt_inth.sed|  17 +
>  util/import_gcrypth.sed|   2 -
>  27 files changed, 1942 insertions(+), 264 deletions(-)
>  create mode 100644 grub-core/lib/b64dec.c
>  create mode 100644 grub-core/lib/libgcrypt_wrap/md.c
>  create mode 100644 grub-core/tests/dsa_sexp_test.c
>  create mode 100644 grub-core/tests/rsa_sexp_test.c
>  create mode 100644 util/import_gcrypt_inth.sed

Hmmm... Could not we split further this patch set?

Daniel

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

Re: [PATCH 1/3] Import libgcrypt 1.10.3

[Daniel Kiper]

Adding a few folks who are probably interested in this series...

May I ask you to add a few words to the commit message saying why this
change is needed? And please do not forget to add your SOB to this and
other patches.

Ah, and missing cover letter...

Daniel

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

Re: [PATCH 0/2] Two small fixes to gzip

[Daniel Kiper]

On Mon, May 13, 2024 at 12:32:07AM +1000, Daniel Axtens wrote:
> I've been fuzzing gzip a bit. So far nothing super exciting, but it's helpful
> to add some code to bail early on EOF (patch 1) and to avoid some 
> uninitialised
> data warnings from valgrind (patch 2). I'm not aware of any security 
> implications
> of either change, and the gzip compression test still passes.
>
> Daniel Axtens (2):
>   gzio: abort early when get_byte reads nothing
>   gzio: Properly init a table

Reviewed-by: Daniel Kiper  for both patches...

I assume I can add your SOB on your behalf...

Daniel

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

Re: [PATCH v12 1/2] fs/erofs: Add support for EROFS

[Daniel Kiper via Grub-devel]

On Fri, May 17, 2024 at 12:40:53PM +0800, Gao Xiang wrote:
> From: Yifan Zhao 
>
> EROFS [1] is a lightweight read-only filesystem designed for performance
> which has already been shipped in most Linux distributions as well as widely
> used in several scenarios, such as Android system partitions, container
> images, and rootfs for embedded devices.
>
> This patch brings EROFS uncompressed support. Now, it's possible to boot
> directly through GRUB with an EROFS rootfs.
>
> EROFS compressed files will be supported later since it has more work to
> polish.
>
> [1] https://erofs.docs.kernel.org
>
> Signed-off-by: Yifan Zhao 
> Tested-by: Daniel Axtens  # fuzz testing only
> Signed-off-by: Gao Xiang 
> ---

[...]

> +static grub_err_t
> +erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
> + struct grub_erofs_map_blocks *map)
> +{
> +  grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.e.i_u.c.format);
> +  grub_uint64_t unit, pos, chunknr, blkaddr;
> +  grub_uint8_t chunkbits;
> +  grub_err_t err;
> +
> +  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
> +unit = sizeof (struct grub_erofs_inode_chunk_index);
> +  else
> +unit = EROFS_BLOCK_MAP_ENTRY_SIZE;
> +
> +  chunkbits = node->data->sb.log2_blksz + (chunk_format & 
> EROFS_CHUNK_FORMAT_BLKBITS_MASK);
> +  if (chunkbits > 63)
> +return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode %" 
> PRIuGRUB_UINT64_T,
> +chunkbits, node->ino);
> +
> +  chunknr = map->m_la >> chunkbits;
> +
> +  if (grub_add (erofs_iloc (node), erofs_inode_size (node), ))
> +return grub_error (GRUB_ERR_OUT_OF_RANGE, "chunkmap position overflow 
> when adding inode size");
> +
> +  if (grub_add (pos, erofs_inode_xattr_ibody_size (node), ))
> +return grub_error (GRUB_ERR_OUT_OF_RANGE, "chunkmap position overflow 
> when adding xattr size");
> +
> +  /* pos = ALIGN_UP(pos, unit) */
> +  if (grub_add (pos, unit - 1, ))
> +return grub_error (GRUB_ERR_OUT_OF_RANGE, "position overflow when 
> seeking at the start of chunkmap");
> +  pos &= ~(unit - 1);

Please create a macro as I asked you earlier. Be careful with underflows too.

Otherwise patch set LGTM...

Daniel

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

Re: [PATCH 2/2 v2] LVM Cachevol and Integrity volumes break entire LVM VG

[Daniel Kiper]

Patrick,

On Thu, May 16, 2024 at 03:38:17PM -0400, Patrick Plenefisch wrote:
> Daniel,
>
> I haven't heard any update about my patch from you since early February. Is
> there anything I need to do or is this good to go? I'm not too familiar with
> actually submitting patches to mailing-list based development, so let me know
> if everything is all set or I need to do something else.

Sorry, I forgot about your patch. I will take a look at it next week.

Daniel

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

Re: [PATCH v11 2/2] fs/erofs: Add tests for EROFS in grub-fs-tester

[Daniel Kiper via Grub-devel]

On Fri, May 10, 2024 at 08:52:56AM +0800, Gao Xiang wrote:
> From: Yifan Zhao 
>
> In this patch, three tests of EROFS are introduced and they cover
> compact, extended and chunk-based inodes, respectively.
>
> Signed-off-by: Yifan Zhao 
> Reviewed-by: Glenn Washburn 
> Signed-off-by: Gao Xiang 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH v11 1/2] fs/erofs: Add support for EROFS

[Daniel Kiper via Grub-devel]

On Fri, May 10, 2024 at 08:52:55AM +0800, Gao Xiang wrote:
> From: Yifan Zhao 
>
> EROFS [1] is a lightweight read-only filesystem designed for performance
> which has already been shipped in most Linux distributions as well as widely
> used in several scenarios, such as Android system partitions, container
> images, and rootfs for embedded devices.
>
> This patch brings EROFS uncompressed support. Now, it's possible to boot
> directly through GRUB with an EROFS rootfs.
>
> EROFS compressed files will be supported later since it has more work to
> polish.
>
> [1] https://erofs.docs.kernel.org
>
> Signed-off-by: Yifan Zhao 
> Tested-by: Daniel Axtens  # fuzz testing only
> Signed-off-by: Gao Xiang 

In general patch LGTM except some nits...

> ---
> Tested-by Link: 
> https://lists.gnu.org/archive/html/grub-devel/2024-05/msg1.html
>
>  INSTALL |8 +-
>  Makefile.util.def   |1 +
>  docs/grub.texi  |3 +-
>  grub-core/Makefile.core.def |5 +
>  grub-core/fs/erofs.c| 1008 +++
>  5 files changed, 1020 insertions(+), 5 deletions(-)
>  create mode 100644 grub-core/fs/erofs.c
>
> diff --git a/INSTALL b/INSTALL
> index 8d9207c84..84030c9f4 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -77,15 +77,15 @@ Prerequisites for make-check:
>
>  * If running a Linux kernel the following modules must be loaded:
>- fuse, loop
> -  - btrfs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, nilfs2,
> +  - btrfs, erofs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, 
> nilfs2,
>  reiserfs, udf, xfs
>- On newer kernels, the exfat kernel modules may be used instead of the
>  exfat FUSE filesystem
>  * The following are Debian named packages required mostly for the full
>suite of filesystem testing (but some are needed by other tests as well):
> -  - btrfs-progs, dosfstools, e2fsprogs, exfat-utils, f2fs-tools, genromfs,
> -hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs, squashfs-tools,
> -reiserfsprogs, udftools, xfsprogs, zfs-fuse
> +  - btrfs-progs, dosfstools, e2fsprogs, erofs-utils, exfat-utils, f2fs-tools,
> +genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
> +squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse
>- exfat-fuse, if not using the exfat kernel module
>- gzip, lzop, xz-utils
>- attr, cpio, g++, gawk, parted, recode, tar, util-linux
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 9432365a9..8d3bc107f 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -98,6 +98,7 @@ library = {
>common = grub-core/fs/cpio_be.c;
>common = grub-core/fs/odc.c;
>common = grub-core/fs/newc.c;
> +  common = grub-core/fs/erofs.c;
>common = grub-core/fs/ext2.c;
>common = grub-core/fs/fat.c;
>common = grub-core/fs/exfat.c;
> diff --git a/docs/grub.texi b/docs/grub.texi
> index d32266f69..b198d963d 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -353,6 +353,7 @@ blocklist notation. The currently supported filesystem 
> types are @dfn{Amiga
>  Fast FileSystem (AFFS)}, @dfn{AtheOS fs}, @dfn{BeFS},
>  @dfn{BtrFS} (including raid0, raid1, raid10, gzip and lzo),
>  @dfn{cpio} (little- and big-endian bin, odc and newc variants),
> +@dfn{EROFS} (only uncompressed support for now),
>  @dfn{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
>  @dfn{exFAT}, @dfn{F2FS}, @dfn{HFS}, @dfn{HFS+},
>  @dfn{ISO9660} (including Joliet, Rock-ridge and multi-chunk files),
> @@ -6276,7 +6277,7 @@ assumed to be encoded in UTF-8.
>  NTFS, JFS, UDF, HFS+, exFAT, long filenames in FAT, Joliet part of
>  ISO9660 are treated as UTF-16 as per specification. AFS and BFS are read
>  as UTF-8, again according to specification. BtrFS, cpio, tar, squash4, minix,
> -minix2, minix3, ROMFS, ReiserFS, XFS, ext2, ext3, ext4, FAT (short names),
> +minix2, minix3, ROMFS, ReiserFS, XFS, EROFS, ext2, ext3, ext4, FAT (short 
> names),
>  F2FS, RockRidge part of ISO9660, nilfs2, UFS1, UFS2 and ZFS are assumed
>  to be UTF-8. This might be false on systems configured with legacy charset
>  but as long as the charset used is superset of ASCII you should be able to
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8e1b1d9f3..7fa9446bd 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1442,6 +1442,11 @@ module = {
>common = fs/odc.c;
>  };
>
> +module = {
> +  name = erofs;
> +  common = fs/erofs.c;
> +};
> +
>  module = {
>name = ext2;
>common = fs/ext2.c;
> diff --git a/grub-core/fs/erofs.c b/grub-core/fs/erofs.c
> new file mode 100644
> index 0..14c86f435
> --- /dev/null
> +++ b/grub-core/fs/erofs.c
> @@ -0,0 +1,1008 @@
> +/* erofs.c - Enhanced Read-Only File System */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under 

Re: [PATCH v10 0/2] Introduce EROFS support

[Daniel Kiper]

Hey,

On Thu, May 02, 2024 at 03:01:37PM +0800, Yifan Zhao wrote:
> EROFS [1] is a lightweight read-only filesystem designed for performance
> which has already been shipped in most Linux distributions as well as widely
> used in several scenarios, such as Android system partitions, container
> images, and rootfs for embedded devices.
>
> This patch brings EROFS uncompressed support together with related tests.
> Now, it's possible to boot directly through GRUB with an EROFS rootfs.
>
> EROFS compressed files will be supported later since it has more work to
> polish.
>
> [1] https://erofs.docs.kernel.org

Sadly Coverity complains about the EROFS code. Please take a look below...

Daniel


*** CID 460612:(BAD_SHIFT)
/grub-core/fs/erofs.c: 418 in erofs_map_blocks_chunkmode()
412   pos &= ~(unit - 1);
413
414   /* no overflow for multiplication as chunkbits >= 9 and sizeof(unit) 
<= 8 */
415   if (grub_add (pos, chunknr * unit, ))
416 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
417
>>> CID 460612:(BAD_SHIFT)
>>> In expression "chunknr << chunkbits", left shifting by more than 63 
>>> bits has undefined behavior.  The shift amount, "chunkbits", is as much as 
>>> 64.
418   map->m_la = chunknr << chunkbits;
419
420   if (grub_sub (erofs_inode_file_size (node), map->m_la, >m_plen))
421 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
422   map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
423   ALIGN_UP (map->m_plen, erofs_blocksz 
(node->data)));
/grub-core/fs/erofs.c: 403 in erofs_map_blocks_chunkmode()
397
398   chunkbits = node->data->sb.log2_blksz + (chunk_format & 
EROFS_CHUNK_FORMAT_BLKBITS_MASK);
399   if (chunkbits > 64)
400 return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode 
%" PRIuGRUB_UINT64_T,
401chunkbits, node->ino);
402
>>> CID 460612:(BAD_SHIFT)
>>> In expression "map->m_la >> chunkbits", right shifting by more than 63 
>>> bits has undefined behavior.  The shift amount, "chunkbits", is as much as 
>>> 64.
403   chunknr = map->m_la >> chunkbits;
404
405   if (grub_add (erofs_iloc (node), erofs_inode_size (node), ) ||
406   grub_add (pos, erofs_inode_xattr_ibody_size (node), ))
407 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
408
/grub-core/fs/erofs.c: 422 in erofs_map_blocks_chunkmode()
416 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
417
418   map->m_la = chunknr << chunkbits;
419
420   if (grub_sub (erofs_inode_file_size (node), map->m_la, >m_plen))
421 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>>> CID 460612:(BAD_SHIFT)
>>> In expression "1UL << chunkbits", left shifting by more than 63 bits 
>>> has undefined behavior.  The shift amount, "chunkbits", is as much as 64.
422   map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
423   ALIGN_UP (map->m_plen, erofs_blocksz 
(node->data)));
424
425   if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
426 {
427   struct grub_erofs_inode_chunk_index idx;


*** CID 460611:  Error handling issues  (CHECKED_RETURN)
/grub-core/fs/erofs.c: 793 in erofs_dir_iter()
787 {
788   struct grub_erofs_dir_ctx *ctx = data;
789   struct grub_dirhook_info info = {0};
790
791   if (!node->inode_loaded)
792 {
>>> CID 460611:  Error handling issues  (CHECKED_RETURN)
>>> Calling "erofs_read_inode" without checking return value (as is done 
>>> elsewhere 6 out of 7 times).
793   erofs_read_inode (ctx->data, node);
794   grub_errno = GRUB_ERR_NONE;
795 }
796
797   if (node->inode_loaded)
798 {


*** CID 460610:  Resource leaks  (RESOURCE_LEAK)
/grub-core/fs/erofs.c: 957 in grub_erofs_label()
951   *label = NULL;
952   return grub_errno;
953 }
954
955   *label = grub_strndup ((char *) data->sb.volume_name, sizeof 
(data->sb.volume_name));
956   if (!*label)
>>> CID 460610:  Resource leaks  (RESOURCE_LEAK)
>>> Variable "data" going out of scope leaks the storage it points to.
957 return grub_errno;
958
959   grub_free (data);
960
961   return GRUB_ERR_NONE;
962 }


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

Re: Release signing key still uses SHA1

[Daniel Kiper]

On Fri, Apr 26, 2024 at 12:13:21AM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Apr 25, 2024 at 11:27:53PM +0200, Daniel Kiper wrote:
> > Hey,
> >
> > On Tue, Mar 12, 2024 at 05:13:24AM +0100, Marek Marczykowski-Górecki wrote:
> > > Hi,
> > >
> > > The key used to sign release tarballs and git tags still uses SHA1 for
> > > its self-signature. Is updated key somewhere already?
> >
> > I have just updated it. You can find it at
> >   
> > https://keys.openpgp.org/vks/v1/by-fingerprint/BE5C23209ACDDACEB20DB0A28C8189F1988C2166
> >
> > Please drop me a line it works or not...
>
> Thanks, looks good now :)

Great! Thanks!

Daniel

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

Re: [PATCH] efi: mm: Fix incorrect free size

[Daniel Kiper via Grub-devel]

Your patch makes sens but the commit message is wrong. In general we
will be leaking memory for short time. Not big deal here but worth
fixing. The memory will not be freed twice as you say in the commit
message. May I ask you to fix the commit message?

On Tue, Apr 23, 2024 at 09:04:34AM +0800, Zhou Jianfeng wrote:
> From: Zhou JianFeng 
>
> Memory freed by function grub_efi_free_pages() with wrong pages will
> not be removed from list efi_allocated_memory by function
> grub_efi_drop_alloc(), it will be freed again by function

At least "again" should be dropped from here. Though in general this
commit message does not parse well and should be rephrased.

> grub_efi_memory_fini() which is called by function
> grub_machine_fini()/grub_exit() when exit grub.
> Freeing memory twice may lead to unpredictable result.
>
> Cc: daniel.ki...@oracle.com
> Cc: alexander.burmas...@oracle.com
> Cc: phco...@gmail.com
> Signed-off-by: Zhou JianFeng 
> ---
>  grub-core/kern/efi/mm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 6a6fba891..49daa976f 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -643,7 +643,7 @@ grub_efi_mm_add_regions (grub_size_t required_bytes, 
> unsigned int flags)
>
>/* Release the memory maps.  */
>grub_efi_free_pages ((grub_addr_t) memory_map,
> -2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> +2 * BYTES_TO_PAGES (map_size));
>
>return GRUB_ERR_NONE;
>  }

Daniel

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

Re: [PATCH v5] efi: Fix stack protector issues

[Daniel Kiper]

On Mon, Apr 29, 2024 at 03:03:50PM +0200, Ard Biesheuvel wrote:
> On Sat, 27 Apr 2024 at 15:08, Glenn Washburn
>  wrote:
> >
> > From: Ard Biesheuvel 
> >
> > The 'ground truth' stack protector cookie value is kept in a global
> > variable, and loaded in every function prologue and epilogue to store
> > it into resp. compare it with the stack slot holding the cookie.
> >
> > If the comparison fails, the program aborts, and this might occur
> > spuriously when the global variable changes values between the entry and
> > exit of a function. This implies that assigning the global variable at
> > boot should not involve any instrumented function calls, unless special
> > care is taken to ensure that the live call stack is synchronized, which
> > is non-trivial.
> >
> > So avoid any function calls, including grub_memcpy(), which is
> > unnecessary given that the stack cookie is always a suitably aligned
> > variable of the native word size.
> >
> > While at it, leave the last byte 0x0 to avoid inadvertent unbounded
> > strings on the stack.
> >
> > Note that the use of __attribute__((optimize)) is described as
> > unsuitable for production use in the GCC documentation, so let's drop
> > this as well now that it is no longer needed.
> >
> > Signed-off-by: Ard Biesheuvel 
> > Reviewed-by: Glenn Washburn 

I think this RB does not make a lot of sens if we have your SOB below.
I will drop the RB.

> > Signed-off-by: Glenn Washburn 
>
> Thanks for taking care of this.

Yeah, thanks a lot Glenn!

> I'd ack it but that would make the signoff chain look even weirder :-)

:-)

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] lvm: Grub2 fails to detect LVM volumes due to an incorrect computation of mda_end

[Daniel Kiper via Grub-devel]

Adding Marta...

On Mon, May 06, 2024 at 03:18:45PM -0500, Glenn Washburn wrote:
> From: Rogier 
>
> When handling a regular LVM volume, Grub can fail with the message:
> error: disk `lvmid/**------
> /**------**' not found.
>
> If the condition which triggers this exists, grub-probe will report the
> error mentioned above. Similarly, the grub boot code will fail to detect
> LVM volumes, resulting in a failure to boot off of LVM disks/partitions.
> The condition can be created on any LVM VG by an LVM configuration change,
> so any system with /boot on LVM can become unbootable at 'any' time (after
> any LVM configuration change).
>
> The problem is caused by an incorrect computation of mda_end in lvm.c, when
> the metadata area wraps around. Apparently, this can start happening at
> around 220 metadata changes to the VG.
>
> Fixes: 879c4a834 (lvm: Fix two more potential data-dependent alloc overflows)
> Fixes: https://savannah.gnu.org/bugs/?61620
>
> Signed-off-by: Rogier 
> Signed-off-by: Glenn Washburn 

Reviewed-by: Daniel Kiper 

Marta, may I ask you to test this patch?

> ---
> I have done no testing of this patch. I've only created a suitable patch
> for review. This seems like a fairly serious issue that might one day bite
> me, so I think it deserves a review.
>
> Glenn
> ---
>  grub-core/disk/lvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 794248540cd3..8535d5a5863a 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -290,7 +290,7 @@ grub_lvm_detect (grub_disk_t disk,
>
>p = q = (char *)ptr;
>
> -  if (grub_add ((grub_size_t)metadatabuf, (grub_size_t)mda_size, ))
> +  if (grub_add (ptr, (grub_size_t)grub_le_to_cpu64 (rlocn->size), ))
>  goto error_parsing_metadata;
>
>mda_end = (char *)ptr;

Daniel

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

Re: [PATCH v5] cryptodisk: allow user to retry failed passphrase

[Daniel Kiper via Grub-devel]

On Mon, May 06, 2024 at 05:07:30PM -0700, Forest wrote:
> Give the user a chance to re-enter their cryptodisk passphrase after a typo,
> rather than immediately failing (and likely dumping them into a grub shell).
>
> By default, we allow 3 tries before giving up. A value in the
> cryptodisk_passphrase_tries environment variable will override this default.
>
> The user can give up early by entering an empty passphrase, just as they
> could before this patch.
>
> Signed-off-by: Forest 

Reviewed-by: Daniel Kiper 

Daniel

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

Re: [PATCH] disk/mdraid1x_linux: Prevent infinite recursion

[Daniel Kiper via Grub-devel]

On Mon, Apr 29, 2024 at 04:38:03PM +, Lidong Chen wrote:
> The test corpus for version-1 RAID generated an infinite recursion
> in grub_partition_iterate() while attempting to read the superblock.
> The reason for the issue was that the data region overlapped with
> the superblock.
>
> The infinite call loop looks like this:
> grub_partition_iterate() -> partmap->iterate() ->
>   -> grub_disk_read() -> grub_disk_read_small() ->
>   -> grub_disk_read_small_real() -> grub_diskfilter_read() ->
>   -> read_lv() -> read_segment() -> grub_diskfilter_read_node() ->
>   -> grub_disk_read() -> grub_disk_read_small() ->...
>
> The fix adds checks for both the superblock region and the data
> region when parsing the superblock metadata in grub_mdraid_detect().
>
> Signed-off-by: Lidong Chen 

Reviewed-by: Daniel Kiper 

Daniel

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