[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Glenn Washburn
Subject: Re: [RFC PATCH] kern/dl: Add module version check
Date: Wed, 21 Dec 2022 11:27:39 -0600

On Wed, 21 Dec 2022 16:07:27 +0800
Zhang Boyang <> wrote:

> Hi,
> On 2022/12/21 03:04, Glenn Washburn wrote:
> > On Mon, 19 Dec 2022 23:33:29 +0800
> > 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. 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, 
> ). 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
> 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
> 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

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.


> Best Regards,
> Zhang Boyang
> > Glenn
> > 
> > [1]

reply via email to

[Prev in Thread] Current Thread [Next in Thread]