[RFC PATCH] kern/dl: Add module version check

[RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time. That module will
be rejected if mismatch is found. This will prevent loading modules from
different versions of GRUB.

It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).

Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
---
 grub-core/Makefile.am         |  2 +-
 grub-core/genmod.sh.in        | 28 +++++++++++++++++-----------
 grub-core/kern/dl.c           | 18 ++++++++++++++++++
 util/grub-module-verifierXX.c | 10 ++++++++++
 4 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 80e7a83ed..d76aa80f4 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
 CLEANFILES += gentrigtables$(BUILD_EXEEXT)
 
 build-grub-module-verifier$(BUILD_EXEEXT): $(top_srcdir)/util/grub-module-verifier.c $(top_srcdir)/util/grub-module-verifier32.c $(top_srcdir)/util/grub-module-verifier64.c $(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
-	$(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
+	$(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" -DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^
 CLEANFILES += build-grub-module-verifier$(BUILD_EXEEXT)
 
 # trigtables.c
diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index e57c4d920..58a4cdcbe 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
 rm -f $tmpfile $outfile
 
 if test x@TARGET_APPLE_LINKER@ != x1; then
-    # stripout .modname and .moddeps sections from input module
-    @TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
+    # stripout .modname and .moddeps and .modver sections from input module
+    @TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile $tmpfile
 
-    # Attach .modname and .moddeps sections
+    # Attach .modname and .moddeps and .modver sections
     t1=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
     printf "$modname\0" >$t1
 
     t2=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
     for dep in $deps; do printf "$dep\0" >> $t2; done
 
+    t3=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+    printf "@PACKAGE_VERSION@\0" >$t3
+
     if test -n "$deps"; then
-	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 $tmpfile
+	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 --add-section .modver=$t3 $tmpfile
     else
-	@TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
+	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .modver=$t3 $tmpfile
     fi
-    rm -f $t1 $t2
+    rm -f $t1 $t2 $t3
 
 	if test x@platform@ != xemu; then
 	    @TARGET_STRIP@ --strip-unneeded \
@@ -71,23 +74,26 @@ else
     tmpfile2=${outfile}.tmp2
     t1=${outfile}.t1.c
     t2=${outfile}.t2.c
+    t3=${outfile}.t3.c
 
     # remove old files if any
-    rm -f $t1 $t2
+    rm -f $t1 $t2 $t3
 
     cp $infile $tmpfile
 
-    # Attach .modname and .moddeps sections
+    # Attach .modname and .moddeps and .modver sections
     echo "char modname[]  __attribute__ ((section(\"_modname, _modname\"))) = \"$modname\";" >$t1
 
     for dep in $deps; do echo "char moddep_$dep[] __attribute__ ((section(\"_moddeps, _moddeps\"))) = \"$dep\";" >>$t2; done
 
+    echo "char modver[]  __attribute__ ((section(\"_modver, _modver\"))) = \"@PACKAGE_VERSION@\";" >$t3
+
     if test -n "$deps"; then
-	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 $t2 $tmpfile -Wl,-r
+	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 $t2 $t3 $tmpfile -Wl,-r
     else
-	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 $tmpfile -Wl,-r
+	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 $t3 $tmpfile -Wl,-r
     fi
-    rm -f $t1 $t2 $tmpfile
+    rm -f $t1 $t2 $t3 $tmpfile
     mv $tmpfile2 $tmpfile
 
 	cp $tmpfile $tmpfile.bin
diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index e447fd0fa..5fbcfc147 100644
--- a/grub-core/kern/dl.c
+++ b/grub-core/kern/dl.c
@@ -475,6 +475,23 @@ grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
 		     (char *) e + s->sh_offset);
 }
 
+static grub_err_t
+grub_dl_check_version (grub_dl_t mod, Elf_Ehdr *e)
+{
+  Elf_Shdr *s = grub_dl_find_section (e, ".modver");
+
+  if (s == NULL)
+    return grub_error (GRUB_ERR_BAD_MODULE,
+		       N_("no version section in module %.63s"), mod->name);
+
+  if (grub_strcmp ((char *) e + s->sh_offset, PACKAGE_VERSION) != 0)
+    return grub_error (GRUB_ERR_BAD_MODULE,
+		       N_("version mismatch in module %.63s: %.63s"), mod->name,
+		       (char *) e + s->sh_offset);
+
+  return GRUB_ERR_NONE;
+}
+
 static grub_err_t
 grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
 {
@@ -650,6 +667,7 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
      Be sure to understand your license obligations.
   */
   if (grub_dl_resolve_name (mod, e)
+      || grub_dl_check_version (mod, e)
       || grub_dl_check_license (mod, e)
       || grub_dl_resolve_dependencies (mod, e)
       || grub_dl_load_segments (mod, e)
diff --git a/util/grub-module-verifierXX.c b/util/grub-module-verifierXX.c
index d5907f268..fde111016 100644
--- a/util/grub-module-verifierXX.c
+++ b/util/grub-module-verifierXX.c
@@ -491,8 +491,18 @@ SUFFIX(grub_module_verify) (const char * const filename,
   check_license (filename, arch, e);
 
   Elf_Shdr *s;
+  const char *modver;
   const char *modname;
 
+  s = find_section (arch, e, ".modver");
+  if (!s)
+    grub_util_error ("%s: no module version found", filename);
+
+  modver = (const char *) e + grub_target_to_host (s->sh_offset);
+
+  if (strcmp (modver, PACKAGE_VERSION) != 0)
+    grub_util_error ("%s: bad module version", filename);
+
   s = find_section (arch, e, ".modname");
   if (!s)
     grub_util_error ("%s: no module name found", filename);
-- 
2.30.2

Re: [RFC PATCH] kern/dl: Add module version check

[Glenn Washburn]

On Mon, 19 Dec 2022 23:33:29 +0800
Zhang Boyang <zhangboyang.id@gmail.com> wrote:

> This patch add version information to GRUB modules. Specifically,
> PACKAGE_VERSION is embedded as null-terminated string in .modver
> section. This string is checked at module loading time. That module
> will be rejected if mismatch is found. This will prevent loading
> modules from different versions of GRUB.
> 
> It should be pointed out that the version string is only consisted of
> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is
> not noticed by version check (e.g. configure options).

Is this solving a real world problem? I've been loading modules from
other GRUB versions for years and have yet to notice an issue. Though,
I do see where issues could occur.

There are two use cases that I understand this patch to potentially
break, which I think should continue to be supported. The first use
case is using isos which support the semi-standard loopback.cfg[1]. The
vast majority, if not all, of the uses of the loopback.cfg for loopback
mounted isos (eg. autoiso.cfg and isodetect.cfg) will create menu
entries where $root is set to the loopbacked iso file and then the
loopback.cfg is run. This patch could cause these created menu entries
to be broken if the modules needed for the commands in the loopback.cfg
have not been loaded ahead of time. This is because the $root is now
pointing to the iso device and, assuming $prefix has no device
component (usually the case), then modules will be attempted to be
loaded from the iso, not from the running grub install.

The other use case, which I use to boot my system, is having one GRUB
install load the grub.cfg of another GRUB install. Similar to the first
use case, the issue is that $root changed from the device of the
running GRUB to the device where the grub.cfg to be loaded is located.
So module loading, again, will attempted for modules that are not for
the running GRUB.

I'm not opposed in principle to adding a module version check, if these
issues can be resolved/mitigated. At a minimum it should be easy to
disable at build time (ie. a configure option to disable), and perhaps
even having it disabled by default for a release cycle. Another, more
flexible solution is to allow it to be enabled at runtime through a
special variable check (eg. grub_module_check=1).

Glenn

[1] https://www.supergrubdisk.org/wiki/Loopback.cfg

> 
> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>  grub-core/Makefile.am         |  2 +-
>  grub-core/genmod.sh.in        | 28 +++++++++++++++++-----------
>  grub-core/kern/dl.c           | 18 ++++++++++++++++++
>  util/grub-module-verifierXX.c | 10 ++++++++++
>  4 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index 80e7a83ed..d76aa80f4 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
>  CLEANFILES += gentrigtables$(BUILD_EXEEXT)
>  
>  build-grub-module-verifier$(BUILD_EXEEXT):
> $(top_srcdir)/util/grub-module-verifier.c
> $(top_srcdir)/util/grub-module-verifier32.c
> $(top_srcdir)/util/grub-module-verifier64.c
> $(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
> -	$(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
> $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
> -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
> +	$(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
> $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
> -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\"
> -DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^ CLEANFILES +=
> build-grub-module-verifier$(BUILD_EXEEXT) # trigtables.c diff --git
> a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in index
> e57c4d920..58a4cdcbe 100644 --- a/grub-core/genmod.sh.in
> +++ b/grub-core/genmod.sh.in
> @@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
>  rm -f $tmpfile $outfile
>  
>  if test x@TARGET_APPLE_LINKER@ != x1; then
> -    # stripout .modname and .moddeps sections from input module
> -    @TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
> +    # stripout .modname and .moddeps and .modver sections from input
> module
> +    @TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile
> $tmpfile 
> -    # Attach .modname and .moddeps sections
> +    # Attach .modname and .moddeps and .modver sections
>      t1=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
>      printf "$modname\0" >$t1
>  
>      t2=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
>      for dep in $deps; do printf "$dep\0" >> $t2; done
>  
> +    t3=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
> +    printf "@PACKAGE_VERSION@\0" >$t3
> +
>      if test -n "$deps"; then
> -	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .moddeps=$t2 $tmpfile
> +	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .moddeps=$t2 --add-section .modver=$t3 $tmpfile else
> -	@TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
> +	@TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .modver=$t3 $tmpfile fi
> -    rm -f $t1 $t2
> +    rm -f $t1 $t2 $t3
>  
>  	if test x@platform@ != xemu; then
>  	    @TARGET_STRIP@ --strip-unneeded \
> @@ -71,23 +74,26 @@ else
>      tmpfile2=${outfile}.tmp2
>      t1=${outfile}.t1.c
>      t2=${outfile}.t2.c
> +    t3=${outfile}.t3.c
>  
>      # remove old files if any
> -    rm -f $t1 $t2
> +    rm -f $t1 $t2 $t3
>  
>      cp $infile $tmpfile
>  
> -    # Attach .modname and .moddeps sections
> +    # Attach .modname and .moddeps and .modver sections
>      echo "char modname[]  __attribute__ ((section(\"_modname,
> _modname\"))) = \"$modname\";" >$t1 
>      for dep in $deps; do echo "char moddep_$dep[] __attribute__
> ((section(\"_moddeps, _moddeps\"))) = \"$dep\";" >>$t2; done 
> +    echo "char modver[]  __attribute__ ((section(\"_modver,
> _modver\"))) = \"@PACKAGE_VERSION@\";" >$t3 +
>      if test -n "$deps"; then
> -	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t2 $tmpfile -Wl,-r
> +	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t2 $t3 $tmpfile -Wl,-r else
> -	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $tmpfile -Wl,-r
> +	@TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t3 $tmpfile -Wl,-r fi
> -    rm -f $t1 $t2 $tmpfile
> +    rm -f $t1 $t2 $t3 $tmpfile
>      mv $tmpfile2 $tmpfile
>  
>  	cp $tmpfile $tmpfile.bin
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index e447fd0fa..5fbcfc147 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -475,6 +475,23 @@ grub_dl_check_license (grub_dl_t mod, Elf_Ehdr
> *e) (char *) e + s->sh_offset);
>  }
>  
> +static grub_err_t
> +grub_dl_check_version (grub_dl_t mod, Elf_Ehdr *e)
> +{
> +  Elf_Shdr *s = grub_dl_find_section (e, ".modver");
> +
> +  if (s == NULL)
> +    return grub_error (GRUB_ERR_BAD_MODULE,
> +		       N_("no version section in module %.63s"),
> mod->name); +
> +  if (grub_strcmp ((char *) e + s->sh_offset, PACKAGE_VERSION) != 0)
> +    return grub_error (GRUB_ERR_BAD_MODULE,
> +		       N_("version mismatch in module %.63s:
> %.63s"), mod->name,
> +		       (char *) e + s->sh_offset);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  static grub_err_t
>  grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
>  {
> @@ -650,6 +667,7 @@ grub_dl_load_core_noinit (void *addr, grub_size_t
> size) Be sure to understand your license obligations.
>    */
>    if (grub_dl_resolve_name (mod, e)
> +      || grub_dl_check_version (mod, e)
>        || grub_dl_check_license (mod, e)
>        || grub_dl_resolve_dependencies (mod, e)
>        || grub_dl_load_segments (mod, e)
> diff --git a/util/grub-module-verifierXX.c
> b/util/grub-module-verifierXX.c index d5907f268..fde111016 100644
> --- a/util/grub-module-verifierXX.c
> +++ b/util/grub-module-verifierXX.c
> @@ -491,8 +491,18 @@ SUFFIX(grub_module_verify) (const char * const
> filename, check_license (filename, arch, e);
>  
>    Elf_Shdr *s;
> +  const char *modver;
>    const char *modname;
>  
> +  s = find_section (arch, e, ".modver");
> +  if (!s)
> +    grub_util_error ("%s: no module version found", filename);
> +
> +  modver = (const char *) e + grub_target_to_host (s->sh_offset);
> +
> +  if (strcmp (modver, PACKAGE_VERSION) != 0)
> +    grub_util_error ("%s: bad module version", filename);
> +
>    s = find_section (arch, e, ".modname");
>    if (!s)
>      grub_util_error ("%s: no module name found", filename);

Re: [RFC PATCH] kern/dl: Add module version check

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]

Zhang Boyang <zhangboyang.id@gmail.com> writes:

> This patch add version information to GRUB modules. Specifically,
> PACKAGE_VERSION is embedded as null-terminated string in .modver
> section. This string is checked at module loading time. That module will
> be rejected if mismatch is found. This will prevent loading modules from
> different versions of GRUB.
>
> It should be pointed out that the version string is only consisted of
> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
> noticed by version check (e.g. configure options).

Right now, this only affects non-secureboot scenarios (because we don't
have external signed module support).  I would want to see a resolution
to the external module signing question first so that we don't paint
ourselves into a corner with something like this.

I share Glenn's confusion about what real-world problems this addresses:
my understanding is that grub modules mostly register callbacks, so the
possibility of disaster is low (unless the callback interfaces change of
course, but that generally has not happened).

The combination of those two things leads me to suspect this is not the
right approach.  It seems likely that if we want to down the road of
versionlocking, something like the kernel's ephemeral key approach would
be better suited - and if we want external modules (which I don't think
we do), full SBAT support.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [RFC PATCH] kern/dl: Add module version check

[Pete Batard]

Hello all,

On 2022.12.20 22:58, Robbie Harwood wrote:
> Zhang Boyang <zhangboyang.id@gmail.com> writes:
> 
>> This patch add version information to GRUB modules. Specifically,
>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>> section. This string is checked at module loading time. That module will
>> be rejected if mismatch is found. This will prevent loading modules from
>> different versions of GRUB.
>>
>> It should be pointed out that the version string is only consisted of
>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
>> noticed by version check (e.g. configure options).
> 
> Right now, this only affects non-secureboot scenarios (because we don't
> have external signed module support).  I would want to see a resolution
> to the external module signing question first so that we don't paint
> ourselves into a corner with something like this.
> 
> I share Glenn's confusion about what real-world problems this addresses:
> my understanding is that grub modules mostly register callbacks, so the
> possibility of disaster is low (unless the callback interfaces change of
> course, but that generally has not happened).

I'll add a note that integrating this proposal will make life A LOT more 
difficult for Rufus [1] users (and potentially users of other boot media 
creation software) when booting in BIOS/CSM mode.

The reason is that, unlike what is the case for UEFI, one can not expect 
to be able to pick all the GRUB core files needed to convert a GRUB 
based ISO bootable media to a GRUB based USB bootable media when using 
the kind File System Transposition one needs to apply, in order to avoid 
end-users, especially Windows ones, being left confused as to why their 
USB media is not showing the content they expect. Typically, one of the 
missing files will be a 'core.img' that can work for USB BIOS boot of a 
MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid 
image will not readily provide.

As a result, whereas it can also write a GRUB base ISOHybrid image in DD 
mode, Rufus usually recommends to write such images in what it calls 
"ISO mode" (shorthand for "ISO extraction mode") with the ISO content 
extracted to a FAT or NTFS file system and with a GRUB core.img 
bootloader being installed by the application itself rather than being 
provided by the source image. This core.img, which is usually recompiled 
from GRUB dot releases to include support for MBR partition scheme, FAT, 
NTFS and potentially other required elements, then calls into the GRUB 
modules that were extracted from the ISO image.

And whereas, on paper, this may look a recipe for disaster, ~8 years of 
boot media creation for GRUB 2.x, using this default method of mixing a 
GRUB core.img that wasn't built alongside the modules it is using, shows 
the method to actually work quite well, *even* when most distros take it 
upon themselves to cherry-pick an extra selection of GRUB patches to 
their source while waiting for the next dot release.

However, if this proposal gets applied for BIOS modules, this will break 
the mechanism described above thereby penalising users who have come to 
rely on Rufus on account that most distros, and especially the ones that 
add patches to their GRUB sources, tend to use a distro specific version 
for GRUB (e.g. "2.06~unbuntu_xyz").

Thus, right at the time where we are finally seeing light at the end of 
the tunnel for UEFI, with regards to the root of the problem we are 
trying to address (about Windows users thinking their boot media is not 
created properly when using DD mode, and therefore not even trying to 
boot to Linux -- this is a VERY REAL issue for which I have provided a 
large number of examples already), by ensuring that GRUB will properly 
support File System Transposition for UEFI moving forward (with my 
thanks to Daniel for having reviewed the respective patch series just 
today!) this proposal is now planning to undo the whole File System 
Transposition support *equivalent* we spent a lot of time trying to find 
a workaround for, for BIOS...

I therefore have to second asking what kind of real-world problems this 
is meant to address, especially as I can vouch that, if GRUB goes 
through with this proposal for BIOS, this will most certainly result in 
real-world problems for users trying to create GRUB based BIOS bootable 
media on Windows.

One thing I will point out however is that I have no issue with the 
module version check proposal if it is to be restricted to UEFI boot 
only (or with any other means to validate the UEFI trust/compatibility 
chain), as the underlying problem of not being able to find all the GRUB 
data we need for File System Transposition only applies to BIOS boot.

Regards,

/Pete

[1] https://github.com/pbatard/rufus

> The combination of those two things leads me to suspect this is not the
> right approach.  It seems likely that if we want to down the road of
> versionlocking, something like the kernel's ephemeral key approach would
> be better suited - and if we want external modules (which I don't think
> we do), full SBAT support.
> 
> Be well,
> --Robbie
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

(sorry for duplicate emails because I forgot to add CCs)

On 2022/12/21 06:58, Robbie Harwood wrote:
> Zhang Boyang <zhangboyang.id@gmail.com> writes:
> 
>> This patch add version information to GRUB modules. Specifically,
>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>> section. This string is checked at module loading time. That module will
>> be rejected if mismatch is found. This will prevent loading modules from
>> different versions of GRUB.
>>
>> It should be pointed out that the version string is only consisted of
>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
>> noticed by version check (e.g. configure options).
> 
> Right now, this only affects non-secureboot scenarios (because we don't
> have external signed module support).  I would want to see a resolution
> to the external module signing question first so that we don't paint
> ourselves into a corner with something like this.
> 

This feature is a prerequisite for external signed module support (and 
it is mainly designed for this purpose). SBAT itself is not sufficient 
to protect version mismatch attacks. Consider this scenario:

1) GRUB 2.XX is free of vulnerabilities

2) GRUB 2.YY is also free of vulnerabilities

3) So GRUB 2.XX shares same SBAT numbers with GRUB 2.YY

4) So if there is no version check, it's possible to load GRUB 2.YY 
modules into GRUB 2.XX (and vice versa)

5) However, due to some changes in API or ABI, there is possibility that 
there are vulnerabilities when using GRUB 2.YY modules with GRUB 2.XX.

> I share Glenn's confusion about what real-world problems this addresses:
> my understanding is that grub modules mostly register callbacks, so the
> possibility of disaster is low (unless the callback interfaces change of
> course, but that generally has not happened).
> 

Please see my reply to Glenn for a example that mismatched modules 
crashes GRUB.

> The combination of those two things leads me to suspect this is not the
> right approach.  It seems likely that if we want to down the road of
> versionlocking, something like the kernel's ephemeral key approach would
> be better suited - and if we want external modules (which I don't think
> we do), full SBAT support.
> 

Sorry but I'm not familiar with the kernel's ephemeral key approach. 
Could you please give me some links to this?

By the way, I figured out why shim doesn't enforce SBAT checks for files 
if they are not directly loaded by shim AND have no SBAT information. 
According to 
https://github.com/rhboot/shim/commit/a2da05fcb8972628bec08e4adfc13abbafc319ad 
  , it's vendor's responsibility to revoke all files without SBAT 
information and only sign files with SBAT information at all time.

My grub-wrap patch 
(https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00070.html ) 
already has support for embedding SBAT information to PE wrappers, 
although it not enforced. With this patch, it should be easy to 
implement external modules if wanted (yes, with full SBAT support, if 
vendor always add SBAT information to modules).

Best Regards,
Zhang Boyang

> Be well,
> --Robbie

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 03:04, Glenn Washburn wrote:
> On Mon, 19 Dec 2022 23:33:29 +0800
> Zhang Boyang <zhangboyang.id@gmail.com> wrote:
> 
>> This patch add version information to GRUB modules. Specifically,
>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>> section. This string is checked at module loading time. That module
>> will be rejected if mismatch is found. This will prevent loading
>> modules from different versions of GRUB.
>>
>> It should be pointed out that the version string is only consisted of
>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is
>> not noticed by version check (e.g. configure options).
> 
> Is this solving a real world problem? I've been loading modules from
> other GRUB versions for years and have yet to notice an issue. Though,
> I do see where issues could occur.
> 

The main purpose is to implement truly loadable modules in secure boot 
environment (please see my reply to Robbie for details, 
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html  ). 
But I did encountered strange crashes with mismatched modules, when 
using stock debian grub modules with latest git grub, on both UEFI and 
BIOS. If you are interested, here are reproducible steps:

1) Please install a fresh Debian (without GUI) into a virtual machine 
from debian-11.6.0-amd64-netinst.iso (Secure boot need to be disabled if 
using UEFI)

2) Build latest GRUB from git using these commands
sudo apt update
sudo apt install build-essential git
sudo apt build-dep grub2
git clone https://git.savannah.gnu.org/git/grub.git
cd grub
./bootstrap
./configure --prefix=/usr/local --with-platform=efi  # or 
--with-platform=pc if using BIOS
make -j2
sudo make install

3) Backup stock debian GRUB modules
sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.debian  # replace 
x86_64-efi with i386-pc if using BIOS, the same below

4) Install our build
sudo /usr/local/sbin/grub-install  # add /dev/sda or similar if using BIOS

5) Replace modules with stock modules
sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.mybuild
sudo mv /boot/grub/x86_64-efi.debian /boot/grub/x86_64-efi

6) Reboot. GRUB should crash with messages like this:
Loading Linux 5.10.0-20-amd64 ...
452: out of range pointer: 0x3ff8da10

Aborted. Press any key to exit.


I ran `git bisect` and it reports this problem is introduced by 
052e6068be62 ("mm: When adding a region, merge with region after as well 
as before"). This commit changed the layout of `struct grub_mm_region`, 
which is both used in main program and "relocator.mod", so the module 
will read the wrong field and crashes GRUB.

> There are two use cases that I understand this patch to potentially
> break, which I think should continue to be supported. The first use
> case is using isos which support the semi-standard loopback.cfg[1]. The
> vast majority, if not all, of the uses of the loopback.cfg for loopback
> mounted isos (eg. autoiso.cfg and isodetect.cfg) will create menu
> entries where $root is set to the loopbacked iso file and then the
> loopback.cfg is run. This patch could cause these created menu entries
> to be broken if the modules needed for the commands in the loopback.cfg
> have not been loaded ahead of time. This is because the $root is now
> pointing to the iso device and, assuming $prefix has no device
> component (usually the case), then modules will be attempted to be
> loaded from the iso, not from the running grub install.
> 
> The other use case, which I use to boot my system, is having one GRUB
> install load the grub.cfg of another GRUB install. Similar to the first
> use case, the issue is that $root changed from the device of the
> running GRUB to the device where the grub.cfg to be loaded is located.
> So module loading, again, will attempted for modules that are not for
> the running GRUB.
> 

There seems no API/ABI compatibility guarantees in GRUB. So I don't 
think it is a good idea to mix different versions of main program and 
modules, and it should be consider unsupported (although it works in 
most situations). It is possible to avoid this by preloading modules 
before changing $root and/or $prefix (insmod will be no-op if given 
module name is already exists).

> I'm not opposed in principle to adding a module version check, if these
> issues can be resolved/mitigated. At a minimum it should be easy to
> disable at build time (ie. a configure option to disable), and perhaps
> even having it disabled by default for a release cycle. Another, more
> flexible solution is to allow it to be enabled at runtime through a
> special variable check (eg. grub_module_check=1).
> 

Since this check is mainly designed for secure boot environments, I 
think the best way to revise this patch is to only enforce this check if 
grub is in lockdown mode (e.g. secure boot enabled). If not in lockdown 
mode, emit a warning but still load such mismatched module. Would you 
mind this solution (a ugly warning will be displayed if mismatched 
module is loaded) ?

Best Regards,
Zhang Boyang

> Glenn
> 
> [1] https://www.supergrubdisk.org/wiki/Loopback.cfg

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 10:43, Pete Batard via Grub-devel wrote:
> Hello all,
> 
> On 2022.12.20 22:58, Robbie Harwood wrote:
>> Zhang Boyang <zhangboyang.id@gmail.com> writes:
>>
>>> This patch add version information to GRUB modules. Specifically,
>>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>>> section. This string is checked at module loading time. That module will
>>> be rejected if mismatch is found. This will prevent loading modules from
>>> different versions of GRUB.
>>>
>>> It should be pointed out that the version string is only consisted of
>>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
>>> noticed by version check (e.g. configure options).
>>
>> Right now, this only affects non-secureboot scenarios (because we don't
>> have external signed module support).  I would want to see a resolution
>> to the external module signing question first so that we don't paint
>> ourselves into a corner with something like this.
>>
>> I share Glenn's confusion about what real-world problems this addresses:
>> my understanding is that grub modules mostly register callbacks, so the
>> possibility of disaster is low (unless the callback interfaces change of
>> course, but that generally has not happened).
> 
> I'll add a note that integrating this proposal will make life A LOT more 
> difficult for Rufus [1] users (and potentially users of other boot media 
> creation software) when booting in BIOS/CSM mode.
> 
> The reason is that, unlike what is the case for UEFI, one can not expect 
> to be able to pick all the GRUB core files needed to convert a GRUB 
> based ISO bootable media to a GRUB based USB bootable media when using 
> the kind File System Transposition one needs to apply, in order to avoid 
> end-users, especially Windows ones, being left confused as to why their 
> USB media is not showing the content they expect. Typically, one of the 
> missing files will be a 'core.img' that can work for USB BIOS boot of a 
> MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid 
> image will not readily provide.
> 

The situation is, for BIOS boot, there is no grub core file in ISO 
because it's written to sectors instead of files? Did I understand 
correctly?

> As a result, whereas it can also write a GRUB base ISOHybrid image in DD 
> mode, Rufus usually recommends to write such images in what it calls 
> "ISO mode" (shorthand for "ISO extraction mode") with the ISO content 
> extracted to a FAT or NTFS file system and with a GRUB core.img 
> bootloader being installed by the application itself rather than being 
> provided by the source image. This core.img, which is usually recompiled 
> from GRUB dot releases to include support for MBR partition scheme, FAT, 
> NTFS and potentially other required elements, then calls into the GRUB 
> modules that were extracted from the ISO image.
> 
> And whereas, on paper, this may look a recipe for disaster, ~8 years of 
> boot media creation for GRUB 2.x, using this default method of mixing a 
> GRUB core.img that wasn't built alongside the modules it is using, shows 
> the method to actually work quite well, *even* when most distros take it 
> upon themselves to cherry-pick an extra selection of GRUB patches to 
> their source while waiting for the next dot release.
> 

Please see my reply to Glenn for a real example of mismatched modules 
crashes GRUB ( 
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html ). I 
strongly recommend test whether that commit (052e6068be62) breaks Rufus 
(e.g. Rufus with latest git GRUB, to boot Debian ISO).

> However, if this proposal gets applied for BIOS modules, this will break 
> the mechanism described above thereby penalising users who have come to 
> rely on Rufus on account that most distros, and especially the ones that 
> add patches to their GRUB sources, tend to use a distro specific version 
> for GRUB (e.g. "2.06~unbuntu_xyz").
> 

This patch lives in core, so even it's enforced (I will not enforce this 
check in next patch reversion, see below), Rufus can still ship its own 
unpatched version of grub core and thing will still work.

> Thus, right at the time where we are finally seeing light at the end of 
> the tunnel for UEFI, with regards to the root of the problem we are 
> trying to address (about Windows users thinking their boot media is not 
> created properly when using DD mode, and therefore not even trying to 
> boot to Linux -- this is a VERY REAL issue for which I have provided a 
> large number of examples already), by ensuring that GRUB will properly 
> support File System Transposition for UEFI moving forward (with my 
> thanks to Daniel for having reviewed the respective patch series just 
> today!) this proposal is now planning to undo the whole File System 
> Transposition support *equivalent* we spent a lot of time trying to find 
> a workaround for, for BIOS...
> 

Will overwriting .mod files in USB drive with Rufus's build helps?

> I therefore have to second asking what kind of real-world problems this 
> is meant to address, especially as I can vouch that, if GRUB goes 
> through with this proposal for BIOS, this will most certainly result in 
> real-world problems for users trying to create GRUB based BIOS bootable 
> media on Windows.
> 

The main propose is to implement external modules for UEFI secure boot 
and it's not related to BIOS boot though. Please see my replies to Glenn 
& Robbie for details.

> One thing I will point out however is that I have no issue with the 
> module version check proposal if it is to be restricted to UEFI boot 
> only (or with any other means to validate the UEFI trust/compatibility 
> chain), as the underlying problem of not being able to find all the GRUB 
> data we need for File System Transposition only applies to BIOS boot.
> 

I will make next reversion emit a warning but still load such module if 
grub is not in lockdown mode (e.g. UEFI secure boot enabled). Would you 
mind a ugly warning will be displayed if mismatched module is loaded?

Best Regards,
Zhang Boyang

> Regards,
> 
> /Pete
> 
> [1] https://github.com/pbatard/rufus
> 
>> The combination of those two things leads me to suspect this is not the
>> right approach.  It seems likely that if we want to down the road of
>> versionlocking, something like the kernel's ephemeral key approach would
>> be better suited - and if we want external modules (which I don't think
>> we do), full SBAT support.
>>
>> Be well,
>> --Robbie
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Thomas Schmitt]

Hi,

Pete Batard wrote:
> > unlike what is the case for UEFI, one can not expect
> > to be able to pick all the GRUB core files needed to convert a GRUB
> > based ISO bootable media to a GRUB based USB bootable media [...]
> > Typically, one of the
> > missing files will be a 'core.img' that can work for USB BIOS boot of a
> > MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
> > image will not readily provide.

Zhang Boyang wrote:
> The situation is, for BIOS boot, there is no grub core file in ISO
> because it's written to sectors instead of files? Did I understand
> correctly?

I think it is rather about the fact that GRUB equipped hybrid ISOs boot
on legacy BIOS from USB stick via the MBR code (~440 byte) to the
El Torito boot image, which is in charge for booting from optical media.
In grub-mkrescue ISOs it is called
  /boot/grub/i386-pc/eltorito.img
The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].

So no core.img is needed and the filesystem module set can be sparse,
because the filesystem type is known from which GRUB will read its further
software.


Pete Batard wrote:
> > Rufus usually recommends to write such images in what it calls
> > "ISO mode" (shorthand for "ISO extraction mode") with the ISO content
> > extracted to a FAT or NTFS file system and with a GRUB core.img
> > bootloader

Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
equipment that is needed to make bootable a USB stick with FAT or NTFS.
(The filesystem modules are included in Pete Batard's proposal for the
 /EFI/BOOT tree. But the disk MBR image and core.img are not.)


Zhang Boyang wrote:
> test whether that commit (052e6068be62) breaks Rufus
> (e.g. Rufus with latest git GRUB, to boot Debian ISO).

Debian ISOs still boot on legacy BIOS via ISOLINUX.
Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs for
both, legacy BIOS and EFI.

I think that in any case an ISO made by grub-mkrescue should be tested.
Maybe a distro developer is here who uses it for making a full fledged
installation ISO or a live ISO.


Have a nice day :)

Thomas

Re: [RFC PATCH] kern/dl: Add module version check

[Didier Spaier]

Le 21/12/2022 à 09:43, Thomas Schmitt a écrit :
> I think that in any case an ISO made by grub-mkrescue should be tested.
> Maybe a distro developer is here who uses it for making a full fledged
> installation ISO or a live ISO.

I do for making a full fledged installation ISO in this function, with the
options you suggested privately:

make_iso() {
export MKRESCUE_SED_PARTNO=2
export MKRESCUE_SED_MODE=mjg
grub-mkrescue --locales='' --themes='' --compress=xz -V "$ISOLABEL" \
-o $PATHTOISO -iso_mbr_part_type 0x83 -partition_offset 16 -J \
--xorriso=./grub/grub-mkrescue-sed.sh $ISODIR
}

I'd be glad to help tetsing, if given detailed instructions about what to test
and how, as I am not a developer. However I can't test in Secure Boot enabled
context.

Cheers,
Didier

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 17:43, Thomas Schmitt wrote:
> Hi,
> 
> Pete Batard wrote:
>>> unlike what is the case for UEFI, one can not expect
>>> to be able to pick all the GRUB core files needed to convert a GRUB
>>> based ISO bootable media to a GRUB based USB bootable media [...]
>>> Typically, one of the
>>> missing files will be a 'core.img' that can work for USB BIOS boot of a
>>> MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
>>> image will not readily provide.
> 
> Zhang Boyang wrote:
>> The situation is, for BIOS boot, there is no grub core file in ISO
>> because it's written to sectors instead of files? Did I understand
>> correctly?
> 
> I think it is rather about the fact that GRUB equipped hybrid ISOs boot
> on legacy BIOS from USB stick via the MBR code (~440 byte) to the
> El Torito boot image, which is in charge for booting from optical media.
> In grub-mkrescue ISOs it is called
>    /boot/grub/i386-pc/eltorito.img
> The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
> in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].
> 
> So no core.img is needed and the filesystem module set can be sparse,
> because the filesystem type is known from which GRUB will read its further
> software.
> 
> 

Thanks for explaining! So I think the best solution for Rufus is to 
overwrite distro's .mod files with its owns (if mismatched modules is a 
problem).

> Pete Batard wrote:
>>> Rufus usually recommends to write such images in what it calls
>>> "ISO mode" (shorthand for "ISO extraction mode") with the ISO content
>>> extracted to a FAT or NTFS file system and with a GRUB core.img
>>> bootloader
> 
> Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
> tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
> equipment that is needed to make bootable a USB stick with FAT or NTFS.
> (The filesystem modules are included in Pete Batard's proposal for the
>   /EFI/BOOT tree. But the disk MBR image and core.img are not.)
> 
> 
> Zhang Boyang wrote:
>> test whether that commit (052e6068be62) breaks Rufus
>> (e.g. Rufus with latest git GRUB, to boot Debian ISO).
> 
> Debian ISOs still boot on legacy BIOS via ISOLINUX.
> Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs for
> both, legacy BIOS and EFI.
> 

Oh, yes, Debian seems using isolinux instead of grub.... So please use 
Ubuntu ISOs for testing, which uses grub in BIOS mode. By the way, I 
also tried Arch Linux, it also uses isolinux (but it has a theme looks 
like grub).

I did reversed tests (i.e. old GRUB core + new GRUB modules) just now:

1) I used Rufus 3.21 and ubuntu-22.04.1-desktop-amd64.iso (which uses 
GRUB 2.06) to create a USB stick. To simulate mismatched modules, I 
overwrited "\boot\grub\i386-pc\relocator.mod" (on USB stick) with my 
latest git build. Then that USB stick failed to boot in BIOS mode, 
showing "452: out of range pointer: 0x100010" error.

2) I tested supergrub2-2.06s1-beta2-multiarch-CD.iso with latest Arch 
Linux (BIOS mode, grub 2:2.06.r403.g7259d55ff-1). It doesn't have 
problems booting Arch Linux because it has $prefix properly set to its 
own GRUB path in CDROM, so it is using its own modules. If I manually 
run "set prefix=(hd0,msdos1)/boot/grub", it will failed to boot, showing 
"error: symbol `grub_debug_malloc' not found."

By the way, I googled "452: out of range pointer" and there seems a lot 
of them. Probably the root cause of many of them is mismatched modules, 
so I think giving a clear message to user should be helpful (if similar 
things happens in future).

Best Regards,
Zhang Boyang

> I think that in any case an ISO made by grub-mkrescue should be tested.
> Maybe a distro developer is here who uses it for making a full fledged
> installation ISO or a live ISO.
> 
> 
> Have a nice day :)
> 
> Thomas
> 

Re: [RFC PATCH] kern/dl: Add module version check

[Pete Batard]

Hi Zhang,

On 2022.12.21 11:38, Zhang Boyang wrote:
> Hi,
> 
> On 2022/12/21 17:43, Thomas Schmitt wrote:
>> Hi,
>>
>> Pete Batard wrote:
>>>> unlike what is the case for UEFI, one can not expect
>>>> to be able to pick all the GRUB core files needed to convert a GRUB
>>>> based ISO bootable media to a GRUB based USB bootable media [...]
>>>> Typically, one of the
>>>> missing files will be a 'core.img' that can work for USB BIOS boot of a
>>>> MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
>>>> image will not readily provide.
>>
>> Zhang Boyang wrote:
>>> The situation is, for BIOS boot, there is no grub core file in ISO
>>> because it's written to sectors instead of files? Did I understand
>>> correctly?
>>
>> I think it is rather about the fact that GRUB equipped hybrid ISOs boot
>> on legacy BIOS from USB stick via the MBR code (~440 byte) to the
>> El Torito boot image, which is in charge for booting from optical media.
>> In grub-mkrescue ISOs it is called
>>    /boot/grub/i386-pc/eltorito.img
>> The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
>> in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].
>>
>> So no core.img is needed and the filesystem module set can be sparse,
>> because the filesystem type is known from which GRUB will read its 
>> further
>> software.
>>
>>
> 
> Thanks for explaining! So I think the best solution for Rufus is to 
> overwrite distro's .mod files with its owns (if mismatched modules is a 
> problem).

I'm sorry but that is not an option because, whereas when distro 
maintainers apply patches to GRUB to be able to boot their distribution, 
core.img is usually "safe" from major alterations, meaning that we are 
able to use a core.img that wasn't specifically compiled alongside the 
modules, the modules are usually the elements that are modified.

So, we can NOT rely on vanilla GRUB modules working at all for booting a 
distro that has had GRUB patches applied. Unlike what is the case for 
core.img, where we very seldom observe breakage, and as you actually 
observed below, this will break boot in a lot of cases.

Plus, since we cannot embed GRUB modules in a 1 MB application that we 
purposefully designed to have a small download footprint, you would now 
be forcing users to download ~10 MB of data, which introduces other 
issues (it's already slightly problematic to have users download a 
core.img since we only embed the one from the latest GRUB release in Rufus).

I do understand that what I am reporting is problematic for the solution 
you are proposing, and that you would like to be able to brush it away 
with a simple "Just do it this other way then", but I can assure you, it 
is not that simple, and, to avoid users running into issues, core.img 
replacement is the much better solution.

>> Pete Batard wrote:
>>>> Rufus usually recommends to write such images in what it calls
>>>> "ISO mode" (shorthand for "ISO extraction mode") with the ISO content
>>>> extracted to a FAT or NTFS file system and with a GRUB core.img
>>>> bootloader
>>
>> Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
>> tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
>> equipment that is needed to make bootable a USB stick with FAT or NTFS.
>> (The filesystem modules are included in Pete Batard's proposal for the
>>   /EFI/BOOT tree. But the disk MBR image and core.img are not.)

Yeah, that was really the only other *remotely viable* alternative I 
considered at the time.

This however requires a few factors:
- ISOHybrid to be used (not guaranteed. One great plus of Rufus is that, 
in most case, it can make bootable media of non ISOHybrids, including 
GRUB based ones)
- grub-mkrescue to be used to generate the ISOHybrid (The maintainers 
may choose to use a different method)
- Distro maintainers to be cooperative with the idea of supporting an 
alternate mode of creation of bootable USB media compared to DD (which, 
sadly, is not a given).

>> Zhang Boyang wrote:
>>> test whether that commit (052e6068be62) breaks Rufus
>>> (e.g. Rufus with latest git GRUB, to boot Debian ISO).
>>
>> Debian ISOs still boot on legacy BIOS via ISOLINUX.
>> Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs 
>> for
>> both, legacy BIOS and EFI.
>>
> 
> Oh, yes, Debian seems using isolinux instead of grub.... So please use 
> Ubuntu ISOs for testing, which uses grub in BIOS mode. By the way, I 
> also tried Arch Linux, it also uses isolinux (but it has a theme looks 
> like grub).
> 
> I did reversed tests (i.e. old GRUB core + new GRUB modules) just now:
> 
> 1) I used Rufus 3.21 and ubuntu-22.04.1-desktop-amd64.iso (which uses 
> GRUB 2.06) to create a USB stick. To simulate mismatched modules, I 
> overwrited "\boot\grub\i386-pc\relocator.mod" (on USB stick) with my 
> latest git build. Then that USB stick failed to boot in BIOS mode, 
> showing "452: out of range pointer: 0x100010" error.
> 
> 2) I tested supergrub2-2.06s1-beta2-multiarch-CD.iso with latest Arch 
> Linux (BIOS mode, grub 2:2.06.r403.g7259d55ff-1). It doesn't have 
> problems booting Arch Linux because it has $prefix properly set to its 
> own GRUB path in CDROM, so it is using its own modules. If I manually 
> run "set prefix=(hd0,msdos1)/boot/grub", it will failed to boot, showing 
> "error: symbol `grub_debug_malloc' not found."
> 
> By the way, I googled "452: out of range pointer" and there seems a lot 
> of them. Probably the root cause of many of them is mismatched modules, 
> so I think giving a clear message to user should be helpful (if similar 
> things happens in future).

Zhang, you said:

 > The main purpose is to implement truly loadable modules in secure 
boot environment

and I said:

 > I have no issue with the module version check proposal if it is to be 
restricted to UEFI boot only

So I think we can have a compromise that works for the both of us if you 
do restrict your proposal for UEFI only (which, apparently and 
realistically is all we should care about, as trying to secure BIOS boot 
is a fool's errand anyway).

Is there any reason why you couldn't submit a proposal that only adds 
module version check for UEFI boot?

Regards,

/Pete

> 
> Best Regards,
> Zhang Boyang
> 
>> I think that in any case an ISO made by grub-mkrescue should be tested.
>> Maybe a distro developer is here who uses it for making a full fledged
>> installation ISO or a live ISO.
>>
>>
>> Have a nice day :)
>>
>> Thomas
>>

Re: [RFC PATCH] kern/dl: Add module version check

[Glenn Washburn]

On Wed, 21 Dec 2022 16:07:27 +0800
Zhang Boyang <zhangboyang.id@gmail.com> wrote:

> Hi,
> 
> On 2022/12/21 03:04, Glenn Washburn wrote:
> > On Mon, 19 Dec 2022 23:33:29 +0800
> > Zhang Boyang <zhangboyang.id@gmail.com> wrote:
> > 
> >> This patch add version information to GRUB modules. Specifically,
> >> PACKAGE_VERSION is embedded as null-terminated string in .modver
> >> section. This string is checked at module loading time. That module
> >> will be rejected if mismatch is found. This will prevent loading
> >> modules from different versions of GRUB.
> >>
> >> It should be pointed out that the version string is only consisted
> >> of PACKAGE_VERSION. Any difference not reflected in
> >> PACKAGE_VERSION is not noticed by version check (e.g. configure
> >> options).
> > 
> > Is this solving a real world problem? I've been loading modules from
> > other GRUB versions for years and have yet to notice an issue.
> > Though, I do see where issues could occur.
> > 
> 
> The main purpose is to implement truly loadable modules in secure
> boot environment (please see my reply to Robbie for details, 
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
> ). But I did encountered strange crashes with mismatched modules,
> when using stock debian grub modules with latest git grub, on both
> UEFI and BIOS. If you are interested, here are reproducible steps:
> 
> 1) Please install a fresh Debian (without GUI) into a virtual machine 
> from debian-11.6.0-amd64-netinst.iso (Secure boot need to be disabled
> if using UEFI)
> 
> 2) Build latest GRUB from git using these commands
> sudo apt update
> sudo apt install build-essential git
> sudo apt build-dep grub2
> git clone https://git.savannah.gnu.org/git/grub.git
> cd grub
> ./bootstrap
> ./configure --prefix=/usr/local --with-platform=efi  # or 
> --with-platform=pc if using BIOS
> make -j2
> sudo make install
> 
> 3) Backup stock debian GRUB modules
> sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.debian  # replace 
> x86_64-efi with i386-pc if using BIOS, the same below
> 
> 4) Install our build
> sudo /usr/local/sbin/grub-install  # add /dev/sda or similar if using
> BIOS
> 
> 5) Replace modules with stock modules
> sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.mybuild
> sudo mv /boot/grub/x86_64-efi.debian /boot/grub/x86_64-efi
> 
> 6) Reboot. GRUB should crash with messages like this:
> Loading Linux 5.10.0-20-amd64 ...
> 452: out of range pointer: 0x3ff8da10
> 
> Aborted. Press any key to exit.
> 
> 
> I ran `git bisect` and it reports this problem is introduced by 
> 052e6068be62 ("mm: When adding a region, merge with region after as
> well as before"). This commit changed the layout of `struct
> grub_mm_region`, which is both used in main program and
> "relocator.mod", so the module will read the wrong field and crashes
> GRUB.

Yeah, I'm not surprised due to these recent changes. For the last
decade GRUB hasn't had such major changes to the core. So maybe this is
a good time to add a patch like this.

> > There are two use cases that I understand this patch to potentially
> > break, which I think should continue to be supported. The first use
> > case is using isos which support the semi-standard loopback.cfg[1].
> > The vast majority, if not all, of the uses of the loopback.cfg for
> > loopback mounted isos (eg. autoiso.cfg and isodetect.cfg) will
> > create menu entries where $root is set to the loopbacked iso file
> > and then the loopback.cfg is run. This patch could cause these
> > created menu entries to be broken if the modules needed for the
> > commands in the loopback.cfg have not been loaded ahead of time.
> > This is because the $root is now pointing to the iso device and,
> > assuming $prefix has no device component (usually the case), then
> > modules will be attempted to be loaded from the iso, not from the
> > running grub install.
> > 
> > The other use case, which I use to boot my system, is having one
> > GRUB install load the grub.cfg of another GRUB install. Similar to
> > the first use case, the issue is that $root changed from the device
> > of the running GRUB to the device where the grub.cfg to be loaded
> > is located. So module loading, again, will attempted for modules
> > that are not for the running GRUB.
> > 
> 
> There seems no API/ABI compatibility guarantees in GRUB. So I don't 
> think it is a good idea to mix different versions of main program and 
> modules, and it should be consider unsupported (although it works in 
> most situations). It is possible to avoid this by preloading modules 
> before changing $root and/or $prefix (insmod will be no-op if given 
> module name is already exists).

Yep, I agree about the support. I do think behavior that has been
working, even unofficially, for many years, should be have weight to it
when considering changes that will break that behavior.

> > I'm not opposed in principle to adding a module version check, if
> > these issues can be resolved/mitigated. At a minimum it should be
> > easy to disable at build time (ie. a configure option to disable),
> > and perhaps even having it disabled by default for a release cycle.
> > Another, more flexible solution is to allow it to be enabled at
> > runtime through a special variable check (eg. grub_module_check=1).
> > 
> 
> Since this check is mainly designed for secure boot environments, I 
> think the best way to revise this patch is to only enforce this check
> if grub is in lockdown mode (e.g. secure boot enabled). If not in
> lockdown mode, emit a warning but still load such mismatched module.
> Would you mind this solution (a ugly warning will be displayed if
> mismatched module is loaded) ?

This is mostly okay by me. I'll respond more on the v2 patch.

Glenn

> 
> Best Regards,
> Zhang Boyang
> 
> > Glenn
> > 
> > [1] https://www.supergrubdisk.org/wiki/Loopback.cfg

Re: [RFC PATCH] kern/dl: Add module version check

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

Zhang Boyang <zhangboyang.id@gmail.com> writes:

> On 2022/12/21 06:58, Robbie Harwood wrote:
>> Zhang Boyang <zhangboyang.id@gmail.com> writes:
>> 
>>> This patch add version information to GRUB modules. Specifically,
>>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>>> section. This string is checked at module loading time. That module will
>>> be rejected if mismatch is found. This will prevent loading modules from
>>> different versions of GRUB.
>>>
>>> It should be pointed out that the version string is only consisted of
>>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
>>> noticed by version check (e.g. configure options).
>> 
>> Right now, this only affects non-secureboot scenarios (because we don't
>> have external signed module support).  I would want to see a resolution
>> to the external module signing question first so that we don't paint
>> ourselves into a corner with something like this.
>
> This feature is a prerequisite for external signed module support (and 
> it is mainly designed for this purpose). SBAT itself is not sufficient 
> to protect version mismatch attacks.

To be clear, I was not intending it as such.  That said, I dislike some
of your terminology: version mismatch is not generally an attack, merely
a self-own.  If you can write to and subsequently execute bootloader
files, you already have root to the box - there's no attack.  Secureboot
works by limiting what can be executed, and you simply can't have that
on Legacy BIOS.  So I think the order you're designing here is backward.

>> The combination of those two things leads me to suspect this is not the
>> right approach.  It seems likely that if we want to down the road of
>> versionlocking, something like the kernel's ephemeral key approach would
>> be better suited - and if we want external modules (which I don't think
>> we do), full SBAT support.
>
> Sorry but I'm not familiar with the kernel's ephemeral key approach. 
> Could you please give me some links to this?

https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/module-signing.rst

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]