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

[Glenn Washburn]

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