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

Grub EFI fallback - patches for review



Control: severity 767037 serious
Control: tag 767037 +patch

[ Raising severity to serious as I've heard more and more reports of
  the problems here recently. ]

Hi folks,

i have two patches attached here, one for grub and one for
grub-installer. They implement the logic I described below and are
reasonably self-contained and non-intrusive. Please check them over
and give feedback, I'd like to get these in ASAP.

I saw almost zero response to my previous mail... :-/

For this to work sensibly, we will need both patches applying.

On Thu, Nov 20, 2014 at 01:49:55AM +0000, Steve McIntyre wrote:
>
>Hi folks,
>
>All of these bugs look to be the same issue: dealing with broken UEFI
>implementations that don't find/boot a *correctly* installed grub-efi
>loader in \EFI\debian\grubx64.efi for some reason. There's multiple
>parts to fixing this for Debian, I think, and I'll spell them out
>here. Please comment and tell me I'm wrong if you think I am!
>
> 1. Add extra support in the grub-efi*(?) packages. When we
>    install/update a grub-efi boot image (grub*.efi), add the option
>    of *also* installing that image to the removable media path:
>    \EFI\boot\boot$ARCH.efi. This code should *not* be enabled by
>    default (as correctly-functioning UEFI implementations will not
>    need it), but we should add a debconf variable to enable it. Thus,
>    this can be configured elsewhere or even pre-seeded at
>    installation time if desired. As newer, future versions of grub
>    are installed, we should ensure that the copy of grub in the
>    removable media path is updated in sync.
>
>    (Why do we need to update the removable media path copy? To ensure
>    that the loader portion there and the rest of the grub modules,
>    config etc. remain in sync. Otherwise, badness...)
>
> 2. Add support (question, template, etc.) in grub-installer to set
>    the new debconf variable. This should be at low priority so most
>    people won't see it, but it's available in expert mode or (again)
>    for pre-seed use.
>
> 3. Add an extra path through the grub-installer code for *rescue*
>    mode: "Did you install a UEFI system but your Debian system did
>    not boot?" which can set the new debconf variable and then call
>    the normal "reinstall grub" code. We'll need to make sure we warn
>    people that this will over-write any existing UEFI bootloaders on
>    their system, so if they still want to use their old Windows
>    install dual-boot etc. they need to make sure it's configured for
>    booting via Grub.
>
>    Ideally, it would be lovely if we can somehow guess/determine
>    automatically that there is a Debian UEFI installation on the
>    system and offer this new rescue option automatically only where
>    it makes sense. Not sure how hard/easy that would be right now,
>    before investigating the code further.
>
> 4. Add documentation to the installation guide to take people through
>    this process: "If you have a broken UEFI implementation on your
>    computer, then here's how to recognise it and here's what to do to
>    work around it......"
>
>Go on, what have I missed / misunderstood / got wrong?
>
>-- 
>Steve McIntyre, Cambridge, UK.                                steve@einval.com
>  Armed with "Valor": "Centurion" represents quality of Discipline,
>  Honor, Integrity and Loyalty. Now you don't have to be a Caesar to
>  concord the digital world while feeling safe and proud.
>
>
>-- 
>To UNSUBSCRIBE, email to debian-boot-REQUEST@lists.debian.org
>with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
>Archive: https://lists.debian.org/20141120014955.GA28831@einval.com
>
>
-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"...In the UNIX world, people tend to interpret `non-technical user'
 as meaning someone who's only ever written one device driver." -- Daniel Pead
>From e384e597914b6e1b1dcbf96ef6782cf9bcc2313b Mon Sep 17 00:00:00 2001
From: Steve McIntyre <steve@einval.com>
Date: Mon, 1 Dec 2014 11:37:06 +0000
Subject: [PATCH] Add support for forcing EFI installation to the removable
 media path

Add an extra option to grub-install "--force-extra-removable". On EFI
platforms, this will cause an extra copy of the grub-efi image to be
written to the appropriate removable media patch
/boot/efi/EFI/BOOT/BOOT$ARCH.EFI as well. This will help with broken
UEFI implementations where the firmware does not work when configured
with new boot paths.

Also added new debconf logic to add this extra option to grub-install
calls when grub2/force_efi_extra_removable is set true. This allows
other programs like d-i / grub-installer to configure this for general
use.
---
 debian/changelog                                  |   4 +
 debian/config.in                                  |   1 +
 debian/patches/grub-install-extra-removable.patch | 115 ++++++++++++++++++++++
 debian/patches/series                             |   1 +
 debian/postinst.in                                |   6 +-
 debian/templates.in                               |  11 +++
 6 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 debian/patches/grub-install-extra-removable.patch

diff --git a/debian/changelog b/debian/changelog
index 09034d9..0ee7e89 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,10 @@ grub2 (2.02~beta2-17) UNRELEASED; urgency=medium
   [ Ian Campbell ]
   * Correct syntax error in grub-xen-host bootstrap configuration file.
 
+  [ Steve McIntyre ]
+  * Add support for forcing an extra copy of grub-efi to the removable
+    media path /boot/efi/EFI/BOOT/BOOT$ARCH.EFI (#767037)
+
  -- Colin Watson <cjwatson@debian.org>  Tue, 18 Nov 2014 14:55:27 +0000
 
 grub2 (2.02~beta2-16) unstable; urgency=medium
diff --git a/debian/config.in b/debian/config.in
index fcc0dd4..d2afbcb 100644
--- a/debian/config.in
+++ b/debian/config.in
@@ -73,4 +73,5 @@ esac
 
 db_input ${priority} grub2/linux_cmdline || true
 db_input medium grub2/linux_cmdline_default || true
+db_input low grub2/force_efi_extra_removable || true
 db_go
diff --git a/debian/patches/grub-install-extra-removable.patch b/debian/patches/grub-install-extra-removable.patch
new file mode 100644
index 0000000..c32634a
--- /dev/null
+++ b/debian/patches/grub-install-extra-removable.patch
@@ -0,0 +1,115 @@
+Index: grub2-2.02~beta2/util/grub-install.c
+===================================================================
+--- grub2-2.02~beta2.orig/util/grub-install.c
++++ grub2-2.02~beta2/util/grub-install.c
+@@ -56,6 +56,7 @@
+ 
+ static char *target;
+ static int removable = 0;
++static int force_extra_removable = 0;
+ static int recheck = 0;
+ static int update_nvram = 1;
+ static char *install_device = NULL;
+@@ -114,7 +115,8 @@ enum
+     OPTION_LABEL_BGCOLOR,
+     OPTION_PRODUCT_VERSION,
+     OPTION_UEFI_SECURE_BOOT,
+-    OPTION_NO_UEFI_SECURE_BOOT
++    OPTION_NO_UEFI_SECURE_BOOT,
++    OPTION_FORCE_EXTRA_REMOVABLE
+   };
+ 
+ static int fs_probe = 1;
+@@ -217,6 +219,10 @@ argp_parser (int key, char *arg, struct
+       removable = 1;
+       return 0;
+ 
++    case OPTION_FORCE_EXTRA_REMOVABLE:
++      force_extra_removable = 1;
++      return 0;
++
+     case OPTION_ALLOW_FLOPPY:
+       allow_floppy = 1;
+       return 0;
+@@ -323,6 +329,9 @@ static struct argp_option options[] = {
+    N_("do not install an image usable with UEFI Secure Boot, even if the "
+       "system was currently started using it. "
+       "This option is only available on EFI."), 2},
++  {"force-extra-removable", OPTION_FORCE_EXTRA_REMOVABLE, 0, 0,
++   N_("force installation to the removable media path also. "
++      "This option is only available on EFI."), 2},
+   {0, 0, 0, 0, 0, 0}
+ };
+ 
+@@ -829,6 +838,27 @@ fill_core_services (const char *core_ser
+   free (sysv_plist);
+ }
+ 
++static void
++also_install_removable(const char *src, const char *base_efidir, const char *efi_suffix_upper)
++{
++  char *efi_file = NULL;
++  char *dst = NULL;
++  char *dir = NULL;
++		
++  if (!efi_suffix_upper)
++    grub_util_error ("%s", _("You've found a bug"));
++  efi_file = xasprintf ("BOOT%s.EFI", efi_suffix_upper);
++		
++  dir = grub_util_path_concat (3, base_efidir, "EFI", "BOOT");
++  grub_install_mkdir_p (dir);
++		
++  dst = grub_util_path_concat (2, dir, efi_file);
++  grub_install_copy_file (src, dst, 1);
++  free (dst);
++  free (efi_file);
++  free (dir);
++}
++
+ int
+ main (int argc, char *argv[])
+ {
+@@ -846,6 +876,7 @@ main (int argc, char *argv[])
+   char *relative_grubdir;
+   char **efidir_device_names = NULL;
+   grub_device_t efidir_grub_dev = NULL;
++  char *base_efidir = NULL;
+   char *efidir_grub_devname;
+   int efidir_is_mac = 0;
+   int is_prep = 0;
+@@ -878,6 +909,9 @@ main (int argc, char *argv[])
+       bootloader_id = xstrdup ("grub");
+     }
+ 
++  if (removable && force_extra_removable)
++    grub_util_error (_("Invalid to use both --removable and --force_extra_removable"));
++
+   if (!grub_install_source_directory)
+     {
+       if (!target)
+@@ -1087,6 +1121,8 @@ main (int argc, char *argv[])
+       if (!efidir_is_mac && grub_strcmp (fs->name, "fat") != 0)
+ 	grub_util_error (_("%s doesn't look like an EFI partition.\n"), efidir);
+ 
++      base_efidir = xstrdup(efidir);
++
+       /* The EFI specification requires that an EFI System Partition must
+ 	 contain an "EFI" subdirectory, and that OS loaders are stored in
+ 	 subdirectories below EFI.  Vendors are expected to pick names that do
+@@ -1949,9 +1985,15 @@ main (int argc, char *argv[])
+ 	    fprintf (config_dst_f, "configfile $prefix/grub.cfg\n");
+ 	    fclose (config_dst_f);
+ 	    free (config_dst);
++	    if (force_extra_removable)
++	      also_install_removable(efi_signed, base_efidir, efi_suffix_upper);
+ 	  }
+ 	else
+-	  grub_install_copy_file (imgfile, dst, 1);
++	  {
++	    grub_install_copy_file (imgfile, dst, 1);
++	    if (force_extra_removable)
++	      also_install_removable(imgfile, base_efidir, efi_suffix_upper);
++	  }
+ 	free (dst);
+       }
+       if (!removable && update_nvram)
diff --git a/debian/patches/series b/debian/patches/series
index 2fdba0c..0107ec5 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -61,3 +61,4 @@ ieee1275-clear-reset.patch
 ppc64el-disable-vsx.patch
 grub-install-pvxen-paths.patch
 gettext-print-typo.patch
+grub-install-extra-removable.patch
diff --git a/debian/postinst.in b/debian/postinst.in
index 30e6947..cf99e2a 100644
--- a/debian/postinst.in
+++ b/debian/postinst.in
@@ -696,7 +696,11 @@ case "$1" in
             grub-efi-arm)   target=arm-efi ;;
             grub-efi-arm64) target=arm64-efi ;;
           esac
-          grub-install --target="$target"
+          db_get grub2/force_efi_extra_removable
+          if [ "$RET" = true ]; then
+	    FORCE_EXTRA_REMOVABLE="--force-extra-removable"
+	  fi
+          grub-install --target="$target" "$FORCE_EXTRA_REMOVABLE"
         fi
 
         # /boot/grub/ has more chances of being accessible by GRUB
diff --git a/debian/templates.in b/debian/templates.in
index c8326f6..7068602 100644
--- a/debian/templates.in
+++ b/debian/templates.in
@@ -12,6 +12,17 @@ _Description: Linux default command line:
  The following string will be used as Linux parameters for the default menu
  entry but not for the recovery mode.
 
+Template: grub2/force_efi_extra_removable
+Type: boolean
+Default: false
+_Description: Force extra installation to the EFI removable path?
+ Some UEFI-based systems are buggy and do not handle new bootloaders correctly.
+ If you force extra installation of GRUB to the EFI removable path, it should
+ make sure that this system will boot Debian correctly despite such a problem.
+ However, this may remove the ability to boot any other operating systems that
+ also depend on this path. If so, uou will need to ensure that GRUB is
+ configured successfully to be able boot any other OS installations correctly.
+
 # still unused
 Template: grub2/kfreebsd_cmdline
 Type: string
-- 
2.1.3

>From 75c67230ec3464dfa9ef9e15cf05101cf814afd9 Mon Sep 17 00:00:00 2001
From: Steve McIntyre <steve@einval.com>
Date: Mon, 1 Dec 2014 02:53:14 +0000
Subject: [PATCH] Add support for using the UEFI removable media path

Either during installation (low priority or preseeding), or as an
extra rescue-mode option to help people fix their systems post-install
once they realise they need to. (#767037)
---
 debian/changelog                        |   10 ++++
 debian/grub-installer.templates         |   43 +++++++++++++++
 grub-installer                          |   14 +++++
 rescue.d/81grub-efi-force-removable     |   91 +++++++++++++++++++++++++++++++
 rescue.d/81grub-efi-force-removable.tst |    3 +
 5 files changed, 161 insertions(+)
 create mode 100644 rescue.d/81grub-efi-force-removable
 create mode 100644 rescue.d/81grub-efi-force-removable.tst

diff --git a/debian/changelog b/debian/changelog
index 6d94005..20e48cc 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+grub-installer (1.102) unstable; urgency=medium
+
+  [ Steve McIntyre ]
+  * Add extra support for forcing installation to the UEFI
+    removable media path, either during installation (low priority or
+    preseeding), or as an extra rescue-mode option to help people fix
+    their systems post-install once they realise they need to. (#767037)
+
+ -- Steve McIntyre <93sam@debian.org>  Mon, 01 Dec 2014 02:49:36 +0000
+
 grub-installer (1.101) unstable; urgency=medium
 
   [ Steve McIntyre ]
diff --git a/debian/grub-installer.templates b/debian/grub-installer.templates
index e439ad0..a6af2ec 100644
--- a/debian/grub-installer.templates
+++ b/debian/grub-installer.templates
@@ -209,6 +209,21 @@ Type: text
 # :sl1:
 _Description: Updating /etc/kernel-img.conf...
 
+Template: grub-installer/progress/step_force_efi
+Type: text
+# :sl1:
+_Description: Checking whether to force usage of the removable media path
+
+Template: grub-installer/progress/step_mount_filesystems
+Type: text
+# :sl1:
+_Description: Mounting filesystems
+
+Template: grub-installer/progress/step_update_debconf_efi_removable
+Type: text
+# :sl1:
+_Description: Configuring grub-efi for future usage of the removable media path
+
 Template: debian-installer/grub-installer/title
 Type: text
 #  Main menu item
@@ -242,3 +257,31 @@ _Description: Failed to mount /target/proc
  Check /var/log/syslog or see virtual console 4 for the details.
  .
  Warning: Your system may be unbootable!
+
+Template: rescue/menu/grub-efi-force-removable
+Type: text
+# Rescue menu item
+# :sl2:
+_Description: Force GRUB installation to the UEFI removable media path
+
+Template: grub-installer/force-efi-extra-removable
+Type: boolean
+Default: false
+# :sl1:
+_Description: Force GRUB installation to the UEFI removable media path?
+ It seems that this computer is configured to boot via UEFI, but maybe
+ that configuration will not work for booting from the hard
+ drive. Some UEFI firmware implementations do not meet the UEFI
+ specification (i.e. they are buggy!) and do not support proper
+ configuration of boot options from system hard drives.
+ .
+ A workaround for this problem is to install an extra copy of the UEFI
+ version of the GRUB boot loader to a fallback location, the
+ "removable media path". Almost all UEFI systems, no matter how buggy,
+ will boot GRUB that way.
+ .
+ Warning: If the installer failed to detect another operating system
+ that is present on your computer that also depends on this fallback,
+ installing GRUB there will make that operating system temporarily
+ unbootable. GRUB can be manually configured later to boot it if
+ necessary.
diff --git a/grub-installer b/grub-installer
index 4c12998..ef81dbf 100755
--- a/grub-installer
+++ b/grub-installer
@@ -785,6 +785,20 @@ if [ -z "$frdisk" ]; then
 			fi
 		fi
 
+		# Should we force a copy of grub-efi to be installed
+		# to the removable media path too? Ask at low
+		# priority, or can also be pre-seeded of course
+		db_input low grub-installer/force-efi-extra-removable || [ $? -eq 30 ]
+		db_go || exit 10
+		db_get grub-installer/force-efi-extra-removable
+		if [ "$RET" = true ]; then
+			grub_install_params="$grub_install_params --force-extra-removable"
+			# Make sure this happens on upgrades too
+			$chroot $ROOT 'debconf-set-selections' <<EOF
+grub2/force_efi_extra_removable boolean true
+EOF
+		fi
+
 		if [ "$ARCH" = "powerpc/chrp_pegasos" ] ; then
 			# nvram is broken here
 			grub_install_params="$grub_install_params --no-nvram"
diff --git a/rescue.d/81grub-efi-force-removable b/rescue.d/81grub-efi-force-removable
new file mode 100644
index 0000000..b35b724
--- /dev/null
+++ b/rescue.d/81grub-efi-force-removable
@@ -0,0 +1,91 @@
+#! /bin/sh -e
+
+. /usr/share/debconf/confmodule
+
+. /usr/share/grub-installer/functions.sh
+
+log () {
+	logger -t grub-installer "grub-efi-force-removable $@"
+}
+
+error () {
+	log "error: $@"
+}
+
+die () {
+	local template="$1"
+	shift
+
+	error "$@"
+	db_input critical "$template" || [ $? -eq 30 ]
+	db_go || true
+	exit 1
+}
+
+mountvirtfs () {
+	fstype="$1"
+	path="$2"
+	if grep -q "[[:space:]]$fstype\$" /proc/filesystems && \
+	   ! grep -q "^[^ ]\+ \+$path " /proc/mounts; then
+		mkdir -p "$path" || \
+			die grub-installer/mounterr "Error creating $path"
+		mount -t "$fstype" "$fstype" "$path" || \
+			die grub-installer/mounterr "Error mounting $path"
+		trap "umount $path" HUP INT QUIT KILL PIPE TERM EXIT
+	fi
+}
+
+db_progress START 0 3 grub-installer/progress/title
+db_progress INFO grub-installer/progress/step_force_efi
+
+# Should we also install grub-efi to the removable media path?
+# Ask the user
+log "Prompting user about removable media path"
+db_input high grub-installer/force-efi-extra-removable
+if ! db_go; then
+	# back up to menu
+	db_progress STOP
+	exit 10
+fi
+db_get grub-installer/force-efi-extra-removable
+if [ "$RET" != true ]; then
+	db_progress STOP
+	exit 0
+fi
+
+db_progress STEP 1
+db_progress INFO grub-installer/progress/step_mount_filesystems
+
+log "Mounting filesystems"
+# If we're installing grub-efi, it wants /sys mounted in the
+# target. Maybe /proc too?
+mountvirtfs proc /target/proc
+mountvirtfs sysfs /target/sys
+chroot /target mount /boot/efi || true
+
+db_progress STEP 1
+db_progress INFO grub-installer/progress/step_install_loader
+# Do the installation now
+log "Running grub-install"
+if ! chroot /target grub-install --force-extra-removable; then
+	db_input critical grub-installer/grub-install-failed || true
+	db_go || true
+	db_progress STOP
+	exit 1
+fi
+
+db_progress STEP 1
+db_progress INFO grub-installer/progress/step_update_debconf_efi_removable
+# And add the debconf flag so the installed system will also do this in future
+log "Running debconf-set-selections in the chroot"
+chroot /target 'debconf-set-selections' <<EOF
+grub2/force_efi_extra_removable boolean true
+EOF
+
+db_progress STEP 1
+db_progress STOP
+
+# And do some cleanup
+umount /target/boot/efi
+umount /target/sys
+umount /target/proc
diff --git a/rescue.d/81grub-efi-force-removable.tst b/rescue.d/81grub-efi-force-removable.tst
new file mode 100644
index 0000000..9b78191
--- /dev/null
+++ b/rescue.d/81grub-efi-force-removable.tst
@@ -0,0 +1,3 @@
+#! /bin/sh -e
+[ -f /target/boot/grub/grub.cfg ] && ( grep -q /boot/efi /target/etc/fstab )
+
-- 
1.7.10.4


Reply to: