Grub EFI fallback - patches for review

Cyril Brulebois kibi at debian.org
Mon Dec 1 14:52:51 UTC 2014


Steve McIntyre <steve at einval.com> (2014-12-01):
> 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.

Some comments inline + there seems to be little to no consistency as far
as “{U,}EFI removable path” is concerned. Maybe make that uniform across
the patches to reduce user bewilderment?

Did you send the templates for review through debian-l10n-english?

> 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 at 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 at lists.debian.org
> >with a subject of "unsubscribe". Trouble? Contact listmaster at lists.debian.org
> >Archive: https://lists.debian.org/20141120014955.GA28831@einval.com
> >
> >
> -- 
> Steve McIntyre, Cambridge, UK.                                steve at 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 at 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 at 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);
                        ^^^^^^^

Needs a matching free()?

> ++
> +       /* 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
                                     ^^^

you

> + 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 at 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 at 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

in-target?

> +	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 )
> +

Mraw,
KiBi.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-grub-devel/attachments/20141201/7895d3db/attachment-0003.sig>


More information about the Pkg-grub-devel mailing list