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);