Re: [PATCH] Warn if MBR gap is small and user uses advanced modules

[Michael Chang]

On Wed, Mar 11, 2020 at 01:43:14PM +0100, Daniel Kiper wrote:
> Adding Michael, Mihai, Javier and Peter...
> 
> Below you can find what more or less Vladimir and I agreed WRT small MBR
> gap. In general Vladimir convinced me to phase out small MBR gaps
> support gradually. This is first step in this journey. We think that we
> have to build some warnings into the code and extend documentation.
> Please chime in what you think about that...

Usually I am also not in favor of making radical changes, so I could
share the opinions to take gradual change here. Even though I just
composed the patch to stop small mbr gap support, the diversity of
viewing the problem was still expected. In the end if that helps to
foster a better approach to improve the overall situation that's still a
worthwhile effort ...

> 
> On Tue, Mar 10, 2020 at 01:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > Daniel, do you want to adjust the whitelist?
> >
> > We don't want to support small MBR gap in pair with anything but
> > the simplest config of biosdisk+part_msdos+simple filesystem. In this
> > path "simple filesystems" are all current filesystems except zfs and
> > btrfs

What about lvm, md and cryto disks here ? They could have simple file
system formatted on top, but the size of the core image can go much
bigger.

In addition, a parallel problem I want to address here is that grub
modules get copied to the platform directory way too early regardless of
the image setup result being successful or not. We'd better fix that
problem as well or the "warning" would actually be "critical" in case of
running into 'undefined symbol error' that renders system unbootable. 

Thanks,
Michael

> 
> Missing SOB...
> 
> > ---
> >  grub-core/partmap/gpt.c     |  9 ++++++++-
> >  grub-core/partmap/msdos.c   |  7 ++++++-
> >  include/grub/partition.h    |  3 ++-
> >  include/grub/util/install.h |  7 +++++--
> >  util/grub-install-common.c  | 24 ++++++++++++++++++++++++
> >  util/grub-install.c         | 10 +++++++---
> >  util/grub-setup.c           |  2 +-
> >  util/setup.c                |  5 +++--
> >  8 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> > index 103f6796f..25a5a1934 100644
> > --- a/grub-core/partmap/gpt.c
> > +++ b/grub-core/partmap/gpt.c
> > @@ -25,6 +25,9 @@
> >  #include <grub/msdos_partition.h>
> >  #include <grub/gpt_partition.h>
> >  #include <grub/i18n.h>
> > +#ifdef GRUB_UTIL
> > +#include <grub/emu/misc.h>
> > +#endif
> >
> >  GRUB_MOD_LICENSE ("GPLv3+");
> >
> > @@ -169,7 +172,8 @@ static grub_err_t
> >  gpt_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    struct gpt_partition_map_embed_ctx ctx = {
> >      .start = 0,
> > @@ -191,6 +195,9 @@ gpt_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >          N_("this GPT partition label contains no BIOS Boot Partition;"
> >     " embedding won't be possible"));
> >
> > +  if (ctx.len < 450) {
> 
> Could you define constant somewhere?
> 
> Is it in sectors? Why 450? Should not it be 2048 if 1 MiB below?
> 
> ...and missing "&& warn_short" check...
> 
> > +    grub_util_warn("Your BIOS Boot Partition is under 1 MiB, please
> > increase its size.");
> > +  }
> >    if (ctx.len < *nsectors)
> 
> Could not we use this check somehow instead of adding new one?
> 
> >      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >          N_("your BIOS Boot Partition is too small;"
> > diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> > index 7b8e45076..402bce050 100644
> > --- a/grub-core/partmap/msdos.c
> > +++ b/grub-core/partmap/msdos.c
> > @@ -236,7 +236,8 @@ static grub_err_t
> >  pc_partition_map_embed (struct grub_disk *disk, unsigned int *nsectors,
> >   unsigned int max_nsectors,
> >   grub_embed_type_t embed_type,
> > - grub_disk_addr_t **sectors)
> > + grub_disk_addr_t **sectors,
> > + int warn_short)
> >  {
> >    grub_disk_addr_t end = ~0ULL;
> >    struct grub_msdos_partition_mbr mbr;
> > @@ -390,6 +391,10 @@ pc_partition_map_embed (struct grub_disk *disk,
> > unsigned int *nsectors,
> >        return GRUB_ERR_NONE;
> >      }
> >
> > +  if (end < 450 && warn_short) {
> > +    grub_util_warn("You have a short MBR gap and use advanced config.
> > Please increase post-MBR gap");
> 
> Ditto?
> 
> > +  }
> > +
> >    if (end <= 1)
> >      return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> >          N_("this msdos-style partition label has no "
> > diff --git a/include/grub/partition.h b/include/grub/partition.h
> > index 7adb7ec6e..5697e28d5 100644
> > --- a/include/grub/partition.h
> > +++ b/include/grub/partition.h
> > @@ -55,7 +55,8 @@ struct grub_partition_map
> >    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> >          unsigned int max_nsectors,
> >          grub_embed_type_t embed_type,
> > -        grub_disk_addr_t **sectors);
> > +        grub_disk_addr_t **sectors,
> > +        int warn_short);
> >  #endif
> >  };
> >  typedef struct grub_partition_map *grub_partition_map_t;
> > diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> > index 2631b1074..982115f57 100644
> > --- a/include/grub/util/install.h
> > +++ b/include/grub/util/install.h
> > @@ -193,13 +193,13 @@ grub_util_bios_setup (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes);
> > +       int add_rs_codes, int warn_short_mbr_gap);
> >  void
> >  grub_util_sparc_setup (const char *dir,
> >          const char *boot_file, const char *core_file,
> >          const char *dest, int force,
> >          int fs_probe, int allow_floppy,
> > -        int add_rs_codes);
> > +        int add_rs_codes, int warn_short_mbr_gap);
> >
> >  char *
> >  grub_install_get_image_targets_string (void);
> > @@ -265,4 +265,7 @@ grub_util_get_target_name (const struct
> > grub_install_image_target_desc *t);
> >  extern char *grub_install_copy_buffer;
> >  #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
> >
> > +int
> 
> extern int
> 
> > +grub_install_is_short_mgrgap_supported(void);
> > +
> >  #endif
> > diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> > index ca0ac612a..e4cff2871 100644
> > --- a/util/grub-install-common.c
> > +++ b/util/grub-install-common.c
> > @@ -234,6 +234,30 @@ char *grub_install_source_directory = NULL;
> >  char *grub_install_locale_directory = NULL;
> >  char *grub_install_themes_directory = NULL;
> >
> > +int
> > +grub_install_is_short_mgrgap_supported()
> > +{
> > +  int i, j;
> > +  static const char *whitelist[] =
> > +    {
> > +     "part_msdos", "biosdisk", "affs", "afs", "bfs", "archelp",
> > +     "cpio", "cpio_be", "newc", "odc", "ext2", "fat", "exfat",
> > +     "f2fs", "fshelp", "hfs", "hfsplus", "hfspluscomp",
> > +     "iso9660", "jfs", "minix", "minix2", "minix3", "minix_be",
> > +     "minix2_be", "minix2_be", "nilfs2", "ntfs", "ntfscomp",
> > +     "reiserfs", "romfs", "sfs", "squash4", "tar", "udf",
> > +     "ufs1", "ufs1_be", "ufs2", "xfs"
> > +    };
> 
> The list LGTM...
> 
> > +  for (i = 0; i < modules.n_entries; i++) {
> > +    for (j = 0; j < ARRAY_SIZE (whitelist); j++)
> > +      if (strcmp(modules.entries[i], whitelist[j]) == 0)
> > + break;
> > +    if (j == ARRAY_SIZE (whitelist))
> > +      return 0;
> > +  }
> > +  return 1;
> > +}
> > +
> >  void
> >  grub_install_push_module (const char *val)
> >  {
> > diff --git a/util/grub-install.c b/util/grub-install.c
> > index 8970b73aa..ba27657d9 100644
> > --- a/util/grub-install.c
> > +++ b/util/grub-install.c
> > @@ -1718,10 +1718,14 @@ main (int argc, char *argv[])
> >   install_device);
> >
> >   /*  Now perform the installation.  */
> > - if (install_bootsector)
> > + if (install_bootsector) {
> > +   int warn_short_mbr_gap = grub_install_is_short_mgrgap_supported();
> > +
> >     grub_util_bios_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> > - fs_probe, allow_floppy, add_rs_codes);
> > + fs_probe, allow_floppy, add_rs_codes,
> > + warn_short_mbr_gap);
> > + }
> >   break;
> >        }
> >      case GRUB_INSTALL_PLATFORM_SPARC64_IEEE1275:
> > @@ -1748,7 +1752,7 @@ main (int argc, char *argv[])
> >     grub_util_sparc_setup (platdir, "boot.img", "core.img",
> >   install_drive, force,
> >   fs_probe, allow_floppy,
> > - 0 /* unused */ );
> > + 0 /* unused */, 0 /* unused */ );
> >   break;
> >        }
> >
> > diff --git a/util/grub-setup.c b/util/grub-setup.c
> > index 42b98ad3c..1783224dd 100644
> > --- a/util/grub-setup.c
> > +++ b/util/grub-setup.c
> > @@ -315,7 +315,7 @@ main (int argc, char *argv[])
> >      arguments.core_file ? : DEFAULT_CORE_FILE,
> >      dest_dev, arguments.force,
> >      arguments.fs_probe, arguments.allow_floppy,
> > -    arguments.add_rs_codes);
> > +    arguments.add_rs_codes, 0);
> >
> >    /* Free resources.  */
> >    grub_fini_all ();
> > diff --git a/util/setup.c b/util/setup.c
> > index 3be88aae1..da5f2c07f 100644
> > --- a/util/setup.c
> > +++ b/util/setup.c
> > @@ -254,7 +254,8 @@ SETUP (const char *dir,
> >         const char *boot_file, const char *core_file,
> >         const char *dest, int force,
> >         int fs_probe, int allow_floppy,
> > -       int add_rs_codes __attribute__ ((unused))) /* unused on sparc64 */
> > +       int add_rs_codes __attribute__ ((unused)), /* unused on sparc64 */
> > +       int warn_small)
> >  {
> >    char *core_path;
> >    char *boot_img, *core_img, *boot_path;
> > @@ -530,7 +531,7 @@ SETUP (const char *dir,
> >   GRUB_EMBED_PCBIOS, &sectors);
> >      else if (ctx.dest_partmap)
> >        err = ctx.dest_partmap->embed (dest_dev->disk, &nsec, maxsec,
> > -      GRUB_EMBED_PCBIOS, &sectors);
> > +      GRUB_EMBED_PCBIOS, &sectors, warn_small);
> >      else
> >        err = fs->fs_embed (dest_dev, &nsec, maxsec,
> >     GRUB_EMBED_PCBIOS, &sectors);
> 
> ...plus some missing spaces in function names and other places...
> 
> Additionally, please extend relevant doc section with explanation why we
> are doing that. And maybe with an schedule for phase out...
> 
> Daniel