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

[Glenn Washburn]

On Wed, 21 Dec 2022 20:11:57 +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.
> 
> If GRUB is locked down, modules with mismatched version will be
> rejected. This is necessary for implementing external signed modules
> securely (in future).
> 
> For compatibility reasons, if GRUB isn't locked down, modules can
> still be loaded even if they have mismatched version. A warning
> message will be emited in that situation.
> 
> 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).
> 
> Link:
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
> Link:
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html
> 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           | 33
> +++++++++++++++++++++++++++++++++ util/grub-module-verifierXX.c | 10
> ++++++++++ 4 files changed, 61 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..0292b162c 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -32,6 +32,7 @@
>  #include <grub/env.h>
>  #include <grub/cache.h>
>  #include <grub/i18n.h>
> +#include <grub/lockdown.h>
>  
>  /* Platforms where modules are in a readonly area of memory.  */
>  #if defined(GRUB_MACHINE_QEMU)
> @@ -475,6 +476,37 @@ 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");
> +  const char *modver;
> +  
> +  modver = _("(unknown)");
> +  if (s == NULL)
> +    goto fail;
> +
> +  modver = (char *) e + s->sh_offset;
> +  if (grub_strcmp (modver, PACKAGE_VERSION) != 0)
> +    goto fail;
> +
> +  return GRUB_ERR_NONE;
> +
> +fail:
> +  if (grub_is_lockdown () == GRUB_LOCKDOWN_ENABLED)
> +    {
> +      return grub_error (GRUB_ERR_BAD_MODULE,
> +                      N_("refuse to load module %.63s of
> incorrect version: %.63s != %s"),
> +                      mod->name, modver, PACKAGE_VERSION);
> +    }
> +  else
> +    {
> +      grub_printf_ (N_("warning: module %.63s has incorrect version:
> %.63s != %s\n"),
> +                 mod->name, modver, PACKAGE_VERSION);

I don't quite like this, but I could live with it. I mostly don't like
it because I don't want to get spammed with a lot of these warning
messages. But maybe this is the best of the bad set of options.

One thing to note is I believe that this will take GRUB out
of graphical mode and put it into text mode. This doesn't affect me
(right now) though.

Another thing is that, for the use case of loading a grub.cfg from a
different GRUB installation, this message from version mismatched
modules loaded in the sourcing of the script will be displayed for a
fraction of a second until the menu from the sourced grub.cfg is
displayed. So it may not be helpful for the user.

This isn't likely an issue for loopback.cfg because usually the module
loading is done implicitly and all the commands are generally inside
the menu entries. In this case, printing to the screen seems like a
good idea because directly after the module loading the commands will
run, which might crash GRUB. So if the crash hangs the machine the user
will see this message and have a clue.

Glenn

> +      return GRUB_ERR_NONE;
> +    }
> +}
> +
>  static grub_err_t
>  grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
>  {
> @@ -650,6 +682,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);