grub-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Pete Batard
Subject: Re: [RFC PATCH v2 1/1] kern/dl: Add module version check
Date: Wed, 21 Dec 2022 15:45:07 +0000
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 
Hi again Zhang,
I hadn't had a chance to properly look at your v2 before replying earlier, and it looks like it addresses the elements I had an issue with. 

On 2022.12.21 12:11, Zhang Boyang 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.
This alteration to your original patch looks like a good compromise to both our issues. Thanks for this!
I had a worry that lockdown was something that could be enabled for both BIOS and UEFI, but looking at the current lockdown.h, grub_is_lockdown() always returns GRUB_LOCKDOWN_DISABLED for BIOS, so this looks like a good solution to me. 


A warning message will be emited in that situation.


For BIOS boot, please don't.
If someone explicitly could not enable lockdown, they are unlikely to want lockdown-related warnings to be produced, especially for BIOS where lockdown cannot apply anyway.
So I think we want to add a '#ifdef GRUB_MACHINE_EFI' for that warning (see below). Is that something that you agree with?
If you submit a v3 with the changes I propose below, then I think I will have no objection with this proposal. 


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


#ifdef GRUB_MACHINE_EFI


+  else
+    {
+      grub_printf_ (N_("warning: module %.63s has incorrect version: %.63s != 
%s\n"),
+                   mod->name, modver, PACKAGE_VERSION);
+      return GRUB_ERR_NONE;
+    }


#endif


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


Regards,

/Pete
reply via email to
[Prev in Thread] Current Thread [Next in Thread]