* [RFC PATCH] kern/dl: Add module version check @ 2022-12-19 15:33 Zhang Boyang 2022-12-20 19:04 ` Glenn Washburn 2022-12-20 22:58 ` Robbie Harwood 0 siblings, 2 replies; 13+ messages in thread From: Zhang Boyang @ 2022-12-19 15:33 UTC (permalink / raw) To: grub-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-19 15:33 [RFC PATCH] kern/dl: Add module version check Zhang Boyang @ 2022-12-20 19:04 ` Glenn Washburn 2022-12-21 8:07 ` Zhang Boyang 2022-12-20 22:58 ` Robbie Harwood 1 sibling, 1 reply; 13+ messages in thread From: Glenn Washburn @ 2022-12-20 19:04 UTC (permalink / raw) To: Zhang Boyang; +Cc: The development of GNU GRUB, Daniel Kiper 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); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-20 19:04 ` Glenn Washburn @ 2022-12-21 8:07 ` Zhang Boyang 2022-12-21 17:27 ` Glenn Washburn 0 siblings, 1 reply; 13+ messages in thread From: Zhang Boyang @ 2022-12-21 8:07 UTC (permalink / raw) To: Glenn Washburn Cc: The development of GNU GRUB, Daniel Kiper, Pete Batard, Robbie Harwood 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 8:07 ` Zhang Boyang @ 2022-12-21 17:27 ` Glenn Washburn 0 siblings, 0 replies; 13+ messages in thread From: Glenn Washburn @ 2022-12-21 17:27 UTC (permalink / raw) To: Zhang Boyang Cc: The development of GNU GRUB, Daniel Kiper, Pete Batard, Robbie Harwood 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-19 15:33 [RFC PATCH] kern/dl: Add module version check Zhang Boyang 2022-12-20 19:04 ` Glenn Washburn @ 2022-12-20 22:58 ` Robbie Harwood 2022-12-21 2:43 ` Pete Batard 2022-12-21 7:57 ` Zhang Boyang 1 sibling, 2 replies; 13+ messages in thread From: Robbie Harwood @ 2022-12-20 22:58 UTC (permalink / raw) To: Zhang Boyang, grub-devel; +Cc: Zhang Boyang [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-20 22:58 ` Robbie Harwood @ 2022-12-21 2:43 ` Pete Batard 2022-12-21 8:21 ` Zhang Boyang 2022-12-21 7:57 ` Zhang Boyang 1 sibling, 1 reply; 13+ messages in thread From: Pete Batard @ 2022-12-21 2:43 UTC (permalink / raw) To: grub-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 2:43 ` Pete Batard @ 2022-12-21 8:21 ` Zhang Boyang 2022-12-21 9:43 ` Thomas Schmitt 0 siblings, 1 reply; 13+ messages in thread From: Zhang Boyang @ 2022-12-21 8:21 UTC (permalink / raw) To: Pete Batard Cc: The development of GNU GRUB, Robbie Harwood, Glenn Washburn, Daniel Kiper 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 8:21 ` Zhang Boyang @ 2022-12-21 9:43 ` Thomas Schmitt 2022-12-21 11:34 ` Didier Spaier 2022-12-21 11:38 ` Zhang Boyang 0 siblings, 2 replies; 13+ messages in thread From: Thomas Schmitt @ 2022-12-21 9:43 UTC (permalink / raw) To: grub-devel; +Cc: pete, zhangboyang.id, rharwood, development, dkiper 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 9:43 ` Thomas Schmitt @ 2022-12-21 11:34 ` Didier Spaier 2022-12-21 11:38 ` Zhang Boyang 1 sibling, 0 replies; 13+ messages in thread From: Didier Spaier @ 2022-12-21 11:34 UTC (permalink / raw) To: grub-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 9:43 ` Thomas Schmitt 2022-12-21 11:34 ` Didier Spaier @ 2022-12-21 11:38 ` Zhang Boyang 2022-12-21 14:39 ` Pete Batard 1 sibling, 1 reply; 13+ messages in thread From: Zhang Boyang @ 2022-12-21 11:38 UTC (permalink / raw) To: Thomas Schmitt, grub-devel; +Cc: pete, rharwood, development, dkiper 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 11:38 ` Zhang Boyang @ 2022-12-21 14:39 ` Pete Batard 0 siblings, 0 replies; 13+ messages in thread From: Pete Batard @ 2022-12-21 14:39 UTC (permalink / raw) To: Zhang Boyang, Thomas Schmitt, grub-devel; +Cc: rharwood, development, dkiper 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 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-20 22:58 ` Robbie Harwood 2022-12-21 2:43 ` Pete Batard @ 2022-12-21 7:57 ` Zhang Boyang 2022-12-21 18:01 ` Robbie Harwood 1 sibling, 1 reply; 13+ messages in thread From: Zhang Boyang @ 2022-12-21 7:57 UTC (permalink / raw) To: Robbie Harwood Cc: The development of GNU GRUB, Glenn Washburn, Daniel Kiper, Pete Batard 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH] kern/dl: Add module version check 2022-12-21 7:57 ` Zhang Boyang @ 2022-12-21 18:01 ` Robbie Harwood 0 siblings, 0 replies; 13+ messages in thread From: Robbie Harwood @ 2022-12-21 18:01 UTC (permalink / raw) To: Zhang Boyang Cc: The development of GNU GRUB, Glenn Washburn, Daniel Kiper, Pete Batard [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-21 18:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-19 15:33 [RFC PATCH] kern/dl: Add module version check Zhang Boyang 2022-12-20 19:04 ` Glenn Washburn 2022-12-21 8:07 ` Zhang Boyang 2022-12-21 17:27 ` Glenn Washburn 2022-12-20 22:58 ` Robbie Harwood 2022-12-21 2:43 ` Pete Batard 2022-12-21 8:21 ` Zhang Boyang 2022-12-21 9:43 ` Thomas Schmitt 2022-12-21 11:34 ` Didier Spaier 2022-12-21 11:38 ` Zhang Boyang 2022-12-21 14:39 ` Pete Batard 2022-12-21 7:57 ` Zhang Boyang 2022-12-21 18:01 ` Robbie Harwood
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.