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

[Zhang Boyang]

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