[PATCHv5] grub-install: Add backup and restore

* [PATCHv5] grub-install: Add backup and restore
@ 2021-05-24  9:59 Dimitri John Ledkov
  2021-05-25 15:20 ` Daniel Kiper
  2021-05-28 14:51 ` Daniel Kiper
  0 siblings, 2 replies; 3+ messages in thread
From: Dimitri John Ledkov @ 2021-05-24  9:59 UTC (permalink / raw)
  To: grub-devel; +Cc: Dimitri John Ledkov

From: Dimitri John Ledkov <xnox@ubuntu.com>

Refactor clean_grub_dir to create a backup of all the files, instead
of just irrevocably removing them as the first action. If available,
register atexit handle to restore the backup if errors occur before
point of no return, or remove the backup if everything was
successful. If atexit is not available, the backup remains on disk for
manual recovery.

Some platforms defined a point of no return, i.e. after modules & core
images were updated. Failures from any commands after that stage are
ignored, and backup is cleanedup. For example, on EFI platforms update
is not reverted when efibootmgr fails.

Extra care is taken to ensure atexit handler is only invoked by the
parent process and not any children forks. Some older grub codebases
can invoke parent atexit hooks from forks, which can mess up the
backup.

This allows safer upgrades of MBR & modules, such that
modules/images/fonts/translations are consistent with MBR in case of
errors. For example accidental grub-install /dev/non-existent-disk
currently clobbers and upgrades modules in /boot/grub, despite not
actually updating any MBR.

This patch only handles backup and restore of files copied to
/boot/grub. This patch does not perform backup (or restoration) of MBR
itself or blocklists. Thus when installing i386-pc platform,
corruption may still occur with MBR and blocklists which will not be
attempted to be automatically recovered.

Also add modinfo.sh and *.efi to the cleanup/backup/restore codepath,
to ensure it is also cleaned / backed up / restored.

Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
---

changes since v4:
 + change ponr from size_t to int (there is no bool in grub yet)
 + declare loop variable at the start of the function
 + format one more comment correctly
 + add comment about significance to set ponr

 configure.ac                |   2 +-
 include/grub/util/install.h |  13 ++++
 util/grub-install-common.c  | 143 ++++++++++++++++++++++++++++++++----
 util/grub-install.c         |  40 ++++++++--
 util/grub-mknetdir.c        |   3 +
 util/grub-mkrescue.c        |   3 +
 util/grub-mkstandalone.c    |   2 +
 7 files changed, 182 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index 74719416c4..a5e94f360e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -419,7 +419,7 @@ else
 fi
 
 # Check for functions and headers.
-AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
+AC_CHECK_FUNCS(posix_memalign memalign getextmntent atexit)
 AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
 
 # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits deprecation
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index b93c73caac..b2a0175374 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -275,4 +275,17 @@ extern char *grub_install_copy_buffer;
 int
 grub_install_is_short_mbrgap_supported (void);
 
+/*
+ * grub-install-common tries to make backups of modules & auxilary
+ * files, and restore the backup upon failure to install core.img. There
+ * are platforms with additional actions after modules & core got
+ * installed in place. It is a point of no return, as core.img cannot be
+ * reverted from this point onwards, and new modules should be kept
+ * installed. Before performing these additional actions raise
+ * grub_install_backup_ponr flag, this way failure to perform additional
+ * actions will not result in reverting new modules to the old
+ * ones. (i.e. in case efivars updates fails)
+ */
+extern int grub_install_backup_ponr;
+
 #endif
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index b4f28840f1..d493e4a119 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -185,38 +185,151 @@ grub_install_mkdir_p (const char *dst)
   free (t);
 }
 
+static int
+strcmp_ext (const char *a, const char *b, const char *ext)
+{
+  char *bsuffix = grub_util_path_concat_ext (1, b, ext);
+  int r = strcmp (a, bsuffix);
+
+  free (bsuffix);
+  return r;
+}
+
+enum clean_grub_dir_mode
+{
+  CLEAN_NEW,
+  CLEAN_BACKUP,
+  CREATE_BACKUP,
+  RESTORE_BACKUP
+};
+
+static size_t backup_dirs_size = 0;
+static char **backup_dirs = NULL;
+static pid_t backup_process = 0;
+int grub_install_backup_ponr = 0;
+
 static void
-clean_grub_dir (const char *di)
+clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode)
 {
   grub_util_fd_dir_t d;
   grub_util_fd_dirent_t de;
+  const char *suffix = "";
+
+  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
+    suffix = "~";
 
   d = grub_util_fd_opendir (di);
   if (!d)
-    grub_util_error (_("cannot open directory `%s': %s"),
-		     di, grub_util_fd_strerror ());
+    {
+      if (mode == CLEAN_BACKUP)
+	return;
+      grub_util_error (_("cannot open directory `%s': %s"),
+		       di, grub_util_fd_strerror ());
+    }
 
   while ((de = grub_util_fd_readdir (d)))
     {
       const char *ext = strrchr (de->d_name, '.');
-      if ((ext && (strcmp (ext, ".mod") == 0
-		   || strcmp (ext, ".lst") == 0
-		   || strcmp (ext, ".img") == 0
-		   || strcmp (ext, ".mo") == 0)
-	   && strcmp (de->d_name, "menu.lst") != 0)
-	  || strcmp (de->d_name, "efiemu32.o") == 0
-	  || strcmp (de->d_name, "efiemu64.o") == 0)
+
+      if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0
+		   || strcmp_ext (ext, ".lst", suffix) == 0
+		   || strcmp_ext (ext, ".img", suffix) == 0
+		   || strcmp_ext (ext, ".efi", suffix) == 0
+		   || strcmp_ext (ext, ".mo", suffix) == 0)
+	   && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
+	  || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
+	  || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
+	  || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0)
 	{
-	  char *x = grub_util_path_concat (2, di, de->d_name);
-	  if (grub_util_unlink (x) < 0)
-	    grub_util_error (_("cannot delete `%s': %s"), x,
-			     grub_util_fd_strerror ());
-	  free (x);
+	  char *srcf = grub_util_path_concat (2, di, de->d_name);
+
+	  if (mode == CREATE_BACKUP)
+	    {
+	      char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "~");
+
+	      if (grub_util_rename (srcf, dstf) < 0)
+		grub_util_error (_("cannot backup `%s': %s"), srcf,
+				 grub_util_fd_strerror ());
+	      free (dstf);
+	    }
+	  else if (mode == RESTORE_BACKUP)
+	    {
+	      char *dstf = grub_util_path_concat (2, di, de->d_name);
+
+	      dstf[strlen (dstf) - 1] = '\0';
+	      if (grub_util_rename (srcf, dstf) < 0)
+		grub_util_error (_("cannot restore `%s': %s"), dstf,
+				 grub_util_fd_strerror ());
+	      free (dstf);
+	    }
+	  else
+	    {
+	      if (grub_util_unlink (srcf) < 0)
+		grub_util_error (_("cannot delete `%s': %s"), srcf,
+				 grub_util_fd_strerror ());
+	    }
+	  free (srcf);
 	}
     }
   grub_util_fd_closedir (d);
 }
 
+static void
+restore_backup_atexit (void)
+{
+  size_t i;
+  /*
+   * some child inherited atexit handler, did not clear it, and called
+   * it; thus skip clean or restore logic.
+   */
+  if (backup_process != getpid ())
+    return;
+
+  for (i = 0; i < backup_dirs_size; i++)
+    {
+      /*
+       * if past point of no return simply clean the backups.
+       * Otherwise cleanup newly installed files, and restore the
+       * backups
+       */
+      if (grub_install_backup_ponr)
+	clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP);
+      else
+	{
+	  clean_grub_dir_real (backup_dirs[i], CLEAN_NEW);
+	  clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP);
+	}
+      free (backup_dirs[i]);
+    }
+
+  backup_dirs_size = 0;
+
+  free (backup_dirs);
+}
+
+static void
+append_to_backup_dirs (const char *dir)
+{
+  backup_dirs = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_size + 1));
+  backup_dirs[backup_dirs_size] = strdup (dir);
+  backup_dirs_size++;
+  if (!backup_process)
+    {
+      atexit (restore_backup_atexit);
+      backup_process = getpid ();
+    }
+}
+
+static void
+clean_grub_dir (const char *di)
+{
+  clean_grub_dir_real (di, CLEAN_BACKUP);
+  clean_grub_dir_real (di, CREATE_BACKUP);
+#if defined(HAVE_ATEXIT)
+  append_to_backup_dirs (di);
+#endif
+}
+
 struct install_list
 {
   int is_default;
diff --git a/util/grub-install.c b/util/grub-install.c
index a0babe3eff..e3c06c3b73 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1719,10 +1719,14 @@ main (int argc, char *argv[])
 			
 	/*  Now perform the installation.  */
 	if (install_bootsector)
-	  grub_util_bios_setup (platdir, "boot.img", "core.img",
-				install_drive, force,
-				fs_probe, allow_floppy, add_rs_codes,
-				!grub_install_is_short_mbrgap_supported ());
+	  {
+	    grub_util_bios_setup (platdir, "boot.img", "core.img",
+				  install_drive, force,
+				  fs_probe, allow_floppy, add_rs_codes,
+				  !grub_install_is_short_mbrgap_supported ());
+
+	    grub_install_backup_ponr = 1;
+	  }
 	break;
       }
     case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
@@ -1746,10 +1750,14 @@ main (int argc, char *argv[])
 			
 	/*  Now perform the installation.  */
 	if (install_bootsector)
-	  grub_util_sparc_setup (platdir, "boot.img", "core.img",
-				 install_drive, force,
-				 fs_probe, allow_floppy,
-				 0 /* unused */, 0 /* unused */ );
+	  {
+	    grub_util_sparc_setup (platdir, "boot.img", "core.img",
+				   install_drive, force,
+				   fs_probe, allow_floppy,
+				   0 /* unused */, 0 /* unused */ );
+
+	    grub_install_backup_ponr = 1;
+	  }
 	break;
       }
 
@@ -1776,6 +1784,8 @@ main (int argc, char *argv[])
 	  grub_elf = grub_util_path_concat (2, core_services, "grub.elf");
 	  grub_install_copy_file (imgfile, grub_elf, 1);
 
+	  grub_install_backup_ponr = 1;
+
 	  f = grub_util_fopen (mach_kernel, "a+");
 	  if (!f)
 	    grub_util_error (_("Can't create file: %s"), strerror (errno));
@@ -1878,6 +1888,8 @@ main (int argc, char *argv[])
 	  boot_efi = grub_util_path_concat (2, core_services, "boot.efi");
 	  grub_install_copy_file (imgfile, boot_efi, 1);
 
+	  grub_install_backup_ponr = 1;
+
 	  f = grub_util_fopen (mach_kernel, "r+");
 	  if (!f)
 	    grub_util_error (_("Can't create file: %s"), strerror (errno));
@@ -1916,6 +1928,9 @@ main (int argc, char *argv[])
       {
 	char *dst = grub_util_path_concat (2, efidir, efi_file);
 	grub_install_copy_file (imgfile, dst, 1);
+
+	grub_install_backup_ponr = 1;
+
 	free (dst);
       }
       if (!removable && update_nvram)
@@ -1967,6 +1982,15 @@ main (int argc, char *argv[])
       break;
     }
 
+  /*
+   * Either there are no platform specific code, or it didn't raise
+   * ponr. Raise it here, because usually this is already past point
+   * of no return. If we leave this flag false, atexit all the modules
+   * will be removed from the prefix which would be very confusing.
+   *
+   */
+  grub_install_backup_ponr = 1;
+
   fprintf (stderr, "%s\n", _("Installation finished. No error reported."));
 
   /* Free resources.  */
diff --git a/util/grub-mknetdir.c b/util/grub-mknetdir.c
index 602574d52e..c9ea345b37 100644
--- a/util/grub-mknetdir.c
+++ b/util/grub-mknetdir.c
@@ -159,6 +159,9 @@ process_input_dir (const char *input_dir, enum grub_install_plat platform)
   grub_install_make_image_wrap (input_dir, prefix, output,
 				0, load_cfg,
 				targets[platform].mkimage_target, 0);
+
+  grub_install_backup_ponr = 1;
+
   grub_install_pop_module ();
 
   /* TRANSLATORS: First %s is replaced by platform name. Second one by filename.  */
diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
index 51831027f7..cc87fde38e 100644
--- a/util/grub-mkrescue.c
+++ b/util/grub-mkrescue.c
@@ -530,6 +530,9 @@ main (int argc, char *argv[])
 			       boot_grub, plat);
       source_dirs[plat] = xstrdup (grub_install_source_directory);
     }
+
+  grub_install_backup_ponr = 1;
+
   if (system_area == SYS_AREA_AUTO || grub_install_source_directory)
     {
       if (source_dirs[GRUB_INSTALL_PLATFORM_I386_PC]
diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
index edf309717c..3d23ee71e5 100644
--- a/util/grub-mkstandalone.c
+++ b/util/grub-mkstandalone.c
@@ -318,6 +318,8 @@ main (int argc, char *argv[])
   grub_install_copy_files (grub_install_source_directory,
 			   boot_grub, plat);
 
+  grub_install_backup_ponr = 1;
+
   char *memdisk_img = grub_util_make_temporary_file ();
 
   memdisk = grub_util_fopen (memdisk_img, "wb");
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread