Merge ~arraybolt3/grub:ubuntu into ~ubuntu-uefi-team/grub/+git/ubuntu:ubuntu

[Aaron Rainbolt]

This patch enables the GRUB environment block to be used even if the /boot partition's filesystem cannot be written to by GRUB.

When /boot is a filesystem that GRUB cannot write to (such as BTRFS) on an EFI system, the current behavior is to disable recordfail support entirely and always show the boot menu. This is a non-ideal situation for some users. This patch works around the issue by detecting when /boot cannot be written to by GRUB and the system is EFI-based, and if both are true it saves and loads the environment block from the EFI System Partition if this is the case.

The way this is done is by detecting unwritable /boot and the use of EFI at grub.cfg generation time. If the conditions are met, grub.cfg is adjusted to search for the FS that is mounted at /boot/efi at boot time. It then saves and loads environment variables to and from (efi filesystem)/EFI/ubuntu/grubenv. Additionally, the recordfail handling in grub-common.service is adjusted to do the same check and adjust the correct grubenv file. Finally, update-grub has been adjusted to always wipe and recreate the grubenv file (in the correct location) so as to cope with changing bootloaders (an edge case described below).

How this code handles some edge cases:

* Multiple EFI System Partitions with RAID - Only the filesystem used by /boot/efi is ever used for grubenv storage. GRUB and grub-common.service should always find and handle the correct file.
* Multiple Ubuntu versions and/or flavors on the same system - This will result in some versions of Ubuntu occasionally "taking control" of the EFI system partition during GRUB bootloader upgrades. In the event the format of grubenv changes between versions of GRUB, this could theoretically lead to boot failure after a GRUB update. To work around this, on every update-grub invocation, the grubenv file is deleted and a blank one generated. Assuming update-grub is called when GRUB is updated (**which I did not verify was the case!!!**), this will ensure that grubenv is always compatible with the GRUB installed at EFI/ubuntu.
* User wants to have their own grubenv and/or grub.cfg - The user can modify EFI/ubuntu/grub.cfg to point to their own custom grub.cfg, which can point at their desired custom grubenv. This code should not override that as it only changes the generation of /boot/grub/grub.cfg, not EFI/ubuntu/grub.cfg.
* GRUB is installed to EFI/BOOT, not EFI/ubuntu - The EFI/ubuntu directory is automatically created by update-grub and grub-common.serivce if needed. It does not require GRUB to be installed there to work.
* Multiple operating systems other than Ubuntu are installed - grubenv is saved to EFI/ubuntu/grubenv, and should not affect any non-Ubuntu flavors except for Ubuntu derivatives (which presumably will also be using this change).
* EFI installation is converted to a legacy BIOS installation - the EFI system partition is only modified to include grubenv if the system if EFI-based. Even if /boot is unwritable by GRUB, grubenv will still be located at /boot/grub/grubenv on a BIOS system, rather than /boot/efi/EFI/ubuntu/grubenv.
* Legacy BIOS installation is converted to an EFI installation - See above - the logic will start reading and writing grubenv to/from /boot/efi/EFI/ubuntu/grubenv if /boot is unwritable and the system becomes EFI-based.

[Aaron Rainbolt (arraybolt3)]

Shoot, I proposed this against the wrong branch.

[Aaron Rainbolt (arraybolt3)]

There, fixed.

[Mate Kukri (mkukri)]

Would it hurt if we always put grubenv on ESP when running on EFI? I think the less magic shell scripts and the less special cases, the better off we are.

I am not sure if I am comfortable with doing this post-FF in Noble. I think we need at least an FF-exception? Or maybe should we do this early in the 24.10 cycle instead?

[Aaron Rainbolt (arraybolt3)]

My reasoning for wanting it in 24.04 isn't really a great excuse, but I work for an OEM that would really like for this to work in 24.04 for implementing a system restore feature. We have a fallback for if this isn't accepted, but it's not really an ideal one. For that reason I'd be happy to file the FFe. I totally understand if this is just too late for this cycle though.

I suppose always putting grubenv on ESP when running on EFI would work too. I intentionally didn't do that as I was trying to trip as little "gotchas" that may exist as possible by tweaking the code and the behavior as little as I could manage. I'm happy to rework this to simply say "if on an EFI system, put grubenv there instead". That would be significantly less complicated code-wise, though it would affect more scenarios and therefore potentially come with a greater regression risk.

[Steve Langasek (vorlon)]

> Would it hurt if we always put grubenv on ESP when running on EFI?

One argument against this is that ESPs can't be RAIDed; if you have a system with multiple disks in a redundant configuration, putting the grubenv on the ESP means you then have two grubenvs, and I think if you are booting from the fallback disk there are various subtle cases where you'll regress (e.g. the 'initrdfail' handling in 00_header).

If it were possible instead for grub to provide a variable corresponding to "the ESP I booted from", this would not be an issue.

review: Needs Fixing

[Aaron Rainbolt (arraybolt3)]

vorlon: Implemented all suggested changes.

[Steve Langasek (vorlon)]

Thanks. fwiw my review was a bit of a drive-by, my real purpose was to comment on whether we should turn this on for all UEFI systems. So I'll abstain from here and let Julian/Máté take it the rest of the way.

review: Abstain

[Julian Andres Klode (juliank)]

I left some comments. I'm not sure we can still land this, but if we can get the patch size reduced to alternative lookup path in the loadenv command, this probably is fine.

review: Needs Fixing

[Aaron Rainbolt (arraybolt3)]

I should be up front - if this involves patching the C code component of the bootloader, I do not think I can do this. My C skills stink, and with a piece of software as security-critical as the bootloader, I don't trust myself to be able to modify GRUB's "kernel" without introducing the next Secure Boot-bypassing CVE on accident. That's part of the reason I did this in the shell scripts - GRUB has all the features to do this, so I tried to just leverage them.

If nothing else, hopefully this was at least an interesting proof-of-concept and we can try to get the actual implementation into 24.10. Still, I'm willing to try to make things work in an ideal way, just... get someone from the security team to grill my code if I do try to mess with the C stuff. :)

[Aaron Rainbolt (arraybolt3)]

OK, I think I will go ahead and at least try this, since the proposed solution sounds like theoretically it won't be too difficult to implement even in C, I can be careful, and there's some stuff I can tweak in Calamares to help make this workable.

[Mate Kukri (mkukri)]

@arraybolt3 I'll look at this in more detail next week (hopefully Monday), I didn't have much time for it yet.

d76bcc4... by Aaron Rainbolt on 2024-03-15

Fix missing build dep, enable saving grubenv on ESP

[Aaron Rainbolt (arraybolt3)]

I have reworked the patch to fix all issues pointed out by juliank, including patching the loadenv.c file to work with fallbacks.

CRITICAL NOTE:
*******************************************
**I HAVE NOT TESTED THIS CODE!!! !!! !!!***
*******************************************
I'm not quite sure how to get grub-efi-amd64 to build unsigned, and have been very busy, so this is sort of a "here's where I'm at, look it over and tell me if I'm on the right path" sort of thing. I'll try and test it later today most likely.

Unmerged commits

d76bcc4... by Aaron Rainbolt on 2024-03-15

Fix missing build dep, enable saving grubenv on ESP

6dfb508... by Mate Kukri on 2024-03-05

debian/: Add tests for grub-sort-version

a748151... by Mate Kukri on 2024-03-05

d/grub-sort-version: Append `-0` to abi strings before passing to python-apt (Fixes LP: #2041827)

c699230... by Mate Kukri on 2024-03-05

d/grub-sort-version: Update regex to correctly match kernel flavour

GRUB_FLAVOUR_ORDER didn't work because the regex f"[0-9]*-{flavour}$"
never matched against the entire ABI string.

39483fe... by Mate Kukri on 2024-03-04

d/p/grub-sort-version.patch: Also patch grub-mkconfig to export GRUB_FLAVOUR_ORDER

GRUB_FLAVOUR_ORDER specified in /etc/default/grub was ignored before
this.

diff --git a/debian/control b/debian/control
index f0a896b..3cc18db 100644
--- a/debian/control
+++ b/debian/control
@@ -7,6 +7,7 @@ Uploaders: Felix Zielcke <[e-mail]>, Jordi Mallach <[e-mail]>, S
 Build-Depends: debhelper-compat (= 13),
  patchutils,
  python3,
+ python3-apt,
  python3-pytest,
  flex,
  bison,
diff --git a/debian/grub-common-run b/debian/grub-common-run
new file mode 100755
index 0000000..c7631d2
--- /dev/null
+++ b/debian/grub-common-run
@@ -0,0 +1,33 @@
+#! /bin/sh
+set -e
+
+if test -f /etc/default/grub; then
+  . /etc/default/grub
+fi
+
+for x in /etc/default/grub.d/*.cfg; do
+  if [ -e "${x}" ]; then
+    . "${x}"
+  fi
+done
+
+while getopts 'ri' opt_str; do
+  case "${opt_str}" in
+    r) reset_grubenv='y';;
+    i) check_initrdless_boot_fallback='y';;
+  esac
+done
+
+grubenv_dir="echo "/boot/efi/EFI/$(echo ${GRUB_DISTRIBUTOR} | tr 'A-Z' 'a-z' | cut -d' ' -f1|LC_ALL=C sed 's,[^[:alnum:]_],_,g')")"
+grubenv_location="$grubenv_dir/grubenv"
+
+[ -s "$grubenv_location" ] || rm -f "$grubenv_location"; mkdir -p "$grubenv_dir"
+[ "$reset_grubenv" = 'y' ] && rm -f "$grubenv_location"; mkdir -p "$grubenv_dir"
+
+grub-editenv "$grubenv_location" unset recordfail
+
+if [ "$check_initrdless_boot_fallback" = 'y' ]; then
+  if grub-editenv "$grubenv_location" list | grep -q initrdless_boot_fallback_triggered=1; then
+    echo "grub: GRUB_FORCE_PARTUUID set, initrdless boot paniced, fallback triggered."
+  fi
+fi
diff --git a/debian/grub-common.install.in b/debian/grub-common.install.in
index d43de70..fdf6fa2 100644
--- a/debian/grub-common.install.in
+++ b/debian/grub-common.install.in
@@ -4,6 +4,7 @@
 ../../debian/grub-multi-install usr/lib/grub/
 ../../debian/grub-sort-version	usr/lib/grub
 ../../debian/canonical-uefi-ca.crt usr/share/grub/
+../../debian/grub-common-run usr/lib/grub/
 
 etc/grub.d
 usr/bin/grub-editenv
diff --git a/debian/grub-common.service b/debian/grub-common.service
index e7bdf4b..85011ba 100644
--- a/debian/grub-common.service
+++ b/debian/grub-common.service
@@ -6,9 +6,7 @@ ConditionPathExists=/boot/grub/grub.cfg
 [Service]
 Type=oneshot
 Restart=no
-ExecStartPre=/bin/sh -c '[ -s /boot/grub/grubenv ] || rm -f /boot/grub/grubenv; mkdir -p /boot/grub'
-ExecStart=grub-editenv /boot/grub/grubenv unset recordfail
-ExecStartPost=/bin/sh -c 'if grub-editenv /boot/grub/grubenv list | grep -q initrdless_boot_fallback_triggered=1; then echo "grub: GRUB_FORCE_PARTUUID set, initrdless boot paniced, fallback triggered."; fi'
+ExecStart=/usr/lib/grub/grub-common-run -i
 StandardOutput=kmsg
 
 [Install]
diff --git a/debian/patches/enable-grubenv-on-esp.patch b/debian/patches/enable-grubenv-on-esp.patch
new file mode 100644
index 0000000..1354729
--- /dev/null
+++ b/debian/patches/enable-grubenv-on-esp.patch
@@ -0,0 +1,214 @@
+--- a/grub-core/commands/loadenv.c
++++ b/grub-core/commands/loadenv.c
+@@ -161,17 +161,62 @@ grub_cmd_load_env (grub_extcmd_context_t
+   grub_file_t file;
+   grub_envblk_t envblk;
+   grub_env_whitelist_t whitelist;
++  enum grub_file_type sigskip = state[1].set
++			     ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE;
+ 
+   whitelist.len = argc;
+   whitelist.list = args;
+ 
+   /* state[0] is the -f flag; state[1] is the --skip-sig flag */
+-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
+-			   GRUB_FILE_TYPE_LOADENV
+-			   | (state[1].set
+-			      ? GRUB_FILE_TYPE_SKIP_SIGNATURE : GRUB_FILE_TYPE_NONE));
+-  if (! file)
+-    return grub_errno;
++  if (state[0].set)
++    {
++      file = open_envblk_file (state[0].arg,
++			       GRUB_FILE_TYPE_SAVEENV | sigskip);
++      if (! file)
++        {
++          file = open_envblk_file (state[0].arg,
++				   GRUB_FILE_TYPE_LOADENV | sigskip);
++          if (! file)
++            return grub_errno;
++          grub_env_set ("grubenv_readonly", "y");
++        }
++    }
++  else
++    {
++      const char *envpath = 0;
++      char *filename = 0;
++
++      envpath = grub_env_get ("envpath");
++      if (envpath)
++        {
++          filename = grub_strdup (envpath);
++          file = open_envblk_file (filename,
++				   GRUB_FILE_TYPE_SAVEENV | sigskip);
++          if (! file)
++            {
++              file = open_envblk_file (0, GRUB_FILE_TYPE_SAVEENV | sigskip);
++              if (! file)
++                {
++                  file = open_envblk_file (0, GRUB_FILE_TYPE_LOADENV | sigskip);
++                  if (! file)
++                    return grub_errno;
++                  grub_env_set ("grubenv_readonly", "y");
++                }
++            }
++          grub_free (filename);
++        }
++      else
++        {
++          file = open_envblk_file (0, GRUB_FILE_TYPE_SAVEENV | sigskip);
++          if (! file)
++            {
++              file = open_envblk_file (0, GRUB_FILE_TYPE_LOADENV | sigskip);
++              if (! file)
++                return grub_errno;
++              grub_env_set ("grubenv_readonly", "y");
++            }
++        }
++    }
+ 
+   envblk = read_envblk_file (file);
+   if (! envblk)
+@@ -392,11 +437,45 @@ grub_cmd_save_env (grub_extcmd_context_t
+   if (! argc)
+     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
+ 
+-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
+-			   GRUB_FILE_TYPE_SAVEENV
+-			   | GRUB_FILE_TYPE_SKIP_SIGNATURE);
+-  if (! file)
+-    return grub_errno;
++  if (state[0].set)
++    {
++      file = open_envblk_file (state[0].arg,
++			       GRUB_FILE_TYPE_SAVEENV
++			       | GRUB_FILE_TYPE_SKIP_SIGNATURE);
++      if (! file)
++        return grub_errno;
++    }
++  else
++    {
++      const char *envpath = 0;
++      char *filename = 0;
++
++      envpath = grub_env_get ("envpath");
++      if (envpath)
++        {
++          filename = grub_strdup (envpath);
++          file = open_envblk_file (filename,
++				   GRUB_FILE_TYPE_SAVEENV
++				   | GRUB_FILE_TYPE_SKIP_SIGNATURE);
++          if (! file)
++            {
++              file = open_envblk_file (0,
++				       GRUB_FILE_TYPE_SAVEENV
++				       | GRUB_FILE_TYPE_SKIP_SIGNATURE);
++              if (! file)
++                return grub_errno;
++            }
++          grub_free (filename);
++        }
++      else
++        {
++          file = open_envblk_file (0,
++				   GRUB_FILE_TYPE_SAVEENV
++				   | GRUB_FILE_TYPE_SKIP_SIGNATURE);
++          if (! file)
++            return grub_errno;
++        }
++    }
+ 
+   if (! file->device->disk)
+     {
+--- a/util/grub.d/00_header.in
++++ b/util/grub.d/00_header.in
+@@ -23,6 +23,9 @@ datarootdir="@datarootdir@"
+ grub_lang=`echo $LANG | cut -d . -f 1`
+ grubdir="`echo "/@bootdirname@/@grubdirname@" | sed 's,//*,/,g'`"
+ quick_boot="@QUICK_BOOT@"
++efi_system_partition_disk="$(grub-probe --target=device /boot/efi)"
++efi_system_partition_uuid="$(grub-probe --device $efi_system_partition_disk --target=fs_uuid 2> /dev/null)"
++grub_distributor_normalized="$(echo ${GRUB_DISTRIBUTOR} | tr 'A-Z' 'a-z' | cut -d' ' -f1|LC_ALL=C sed 's,[^[:alnum:]_],_,g')";
+ 
+ export TEXTDOMAIN=@PACKAGE@
+ export TEXTDOMAINDIR="@localedir@"
+@@ -45,7 +48,12 @@ if [ "x${GRUB_DEFAULT_BUTTON}" = "xsaved
+ if [ "x${GRUB_TIMEOUT_BUTTON}" = "x" ] ; then GRUB_TIMEOUT_BUTTON="$GRUB_TIMEOUT" ; fi
+ 
+ cat << EOF
+-if [ -s \$prefix/grubenv ]; then
++search.fs_uuid $efi_system_partition_uuid efisys
++set envpath="(\${efisys})/EFI/$grub_distributor_normalized/grubenv"
++if [ -s \$envpath ]; then
++  set have_grubenv=true
++  load_env
++elif [ -s \$prefix/grubenv ]; then
+   set have_grubenv=true
+   load_env
+ fi
+@@ -132,41 +140,7 @@ if [ "$quick_boot" = 1 ]; then
+     cat <<EOF
+ function recordfail {
+   set recordfail=1
+-EOF
+-
+-  check_writable () {
+-    abstractions="$(grub-probe --target=abstraction "${grubdir}")"
+-    for abstraction in $abstractions; do
+-      case "$abstraction" in
+-        diskfilter | lvm)
+-          cat <<EOF
+-  # GRUB lacks write support for $abstraction, so recordfail support is disabled.
+-EOF
+-          return 1
+-          ;;
+-      esac
+-    done
+-
+-    FS="$(grub-probe --target=fs "${grubdir}")"
+-    case "$FS" in
+-      btrfs | cpiofs | newc | odc | romfs | squash4 | tarfs | zfs)
+-	cat <<EOF
+-  # GRUB lacks write support for $FS, so recordfail support is disabled.
+-EOF
+-	return 1
+-	;;
+-    esac
+-
+-    cat <<EOF
+   if [ -n "\${have_grubenv}" ]; then if [ -z "\${boot_once}" ]; then save_env recordfail; fi; fi
+-EOF
+-  }
+-
+-  if ! check_writable; then
+-    recordfail_broken=1
+-  fi
+-
+-  cat <<EOF
+ }
+ EOF
+ fi
+@@ -407,17 +381,15 @@ EOF
+     cat << EOF
+   fi
+ fi
+-EOF
+-if [ "$recordfail_broken" = 1 ]; then
+-  cat << EOF
+-if [ \$grub_platform = efi ]; then
+-  set timeout=${GRUB_RECORDFAIL_TIMEOUT:-30}
+-  if [ x\$feature_timeout_style = xy ] ; then
+-    set timeout_style=menu
++if [ x\$grubenv_readonly = xy ] ; then
++  if [ \$grub_platform = efi ]; then
++    set timeout=${GRUB_RECORDFAIL_TIMEOUT:-30}
++    if [ x\$feature_timeout_style = xy ] ; then
++      set timeout_style=menu
++    fi
+   fi
+ fi
+ EOF
+-fi
+ }
+ 
+ if [ "x$GRUB_BUTTON_CMOS_ADDRESS" != "x" ]; then
diff --git a/debian/patches/series b/debian/patches/series
index 05de63a..7b73833 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -111,3 +111,4 @@ Revert-kern-ieee1275-init-ppc64-Return-allocated-address-.patch
 Revert-kern-ieee1275-init-ppc64-Decide-by-request-whether.patch
 Revert-kern-ieee1275-init-ppc64-Introduce-a-request-for-r.patch
 grub-install-efi-title.patch
+enable-grubenv-on-esp.patch
diff --git a/debian/update-grub b/debian/update-grub
index 0c43327..fa87c35 100644
--- a/debian/update-grub
+++ b/debian/update-grub
@@ -1,3 +1,6 @@
 #!/bin/sh
 set -e
+
+/usr/lib/grub/grub-common-run -r
+
 exec grub-mkconfig -o /boot/grub/grub.cfg "$@"