On Wed, 21 Dec 2022 16:07:27 +0800 Zhang Boyang <zhangboyang...@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...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
