Re: [PATCH v4 08/15] gdb: If enabled, print line used to load EFI kernel symbols when using gdb_grub script

[Glenn Washburn]

On Wed, 21 Dec 2022 16:20:17 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> Adding Robbie...
> 
> Please CC him next time when you post these patches. I would want to
> hear his opinion too. Or at least he is aware what is happening
> here...

Sure, I CC'd him and Peter on the first couple of ones. But there was
never had a response in the 4 months since then, so I figured they
didn't care.

> 
> On Thu, Dec 15, 2022 at 11:29:31PM -0600, Glenn Washburn wrote:
> > If the macro PRINT_GDB_SYM_LOAD_CMD is non-zero, compile code which
> > will print the command needed to load symbols for the GRUB EFI
> > kernel. This is needed because EFI firmware determines where to
> > load the GRUB EFI at runtime, and so the relevant addresses are not
> > known ahead of time.
> >
> > The command is a custom command defined in the gdb_grub GDB script.
> > So GDB should be started with the script as an argument to the -x
> > option or sourced into an active GDB session before running the
> > outputted command.
> 
> I think this functionality should be disabled when lockdown is
> enforced, e.g. on UEFI platforms with Secure Boot enabled.

Since this is off by default and must be enabled at build time, then if
the builder enabled it, they really did want it, regardless of
lockdown. What you're worried about seems highly improbable to me (but
then I don't know the inner workings of the distros). The concern as I
understand it, is that someone doing an official release of a distro
which will be secure boot ready will accidentally have this build time
macro enabled. That's almost inconceivable to me, but I'm curious what
the others have to say (especially since Robbie posted a similar patch
that always printed this info as a debug message[1]). Or is it more
about a regular user signing with their own keys accidentally shooting
themselves in the foot by forgetting to disable this (after having
already enabled it) and then some physical attacker getting extra info
to do an evil maid attack?

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2021-11/msg00008.html

> 
> > Co-developed-by: Peter Jones <pjones@redhat.com>
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  config.h.in               |  3 +++
> >  grub-core/kern/efi/efi.c  |  4 ++--
> >  grub-core/kern/efi/init.c | 19 ++++++++++++++++++-
> >  include/grub/efi/efi.h    |  2 +-
> >  4 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/config.h.in b/config.h.in
> > index 4d1e50eba7..c31eee1bca 100644
> > --- a/config.h.in
> > +++ b/config.h.in
> > @@ -13,6 +13,9 @@
> >  #define MM_DEBUG @MM_DEBUG@
> >  #endif
> >
> > +/* Define to 1 to enable printing of gdb command to load module
> > symbols.  */ +#define PRINT_GDB_SYM_LOAD_CMD 0
> > +
> >  /* Define to 1 to enable disk cache statistics.  */
> >  #define DISK_CACHE_STATS @DISK_CACHE_STATS@
> >  #define BOOT_TIME_STATS @BOOT_TIME_STATS@
> > diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> > index cf49d6357e..17bd06f7e6 100644
> > --- a/grub-core/kern/efi/efi.c
> > +++ b/grub-core/kern/efi/efi.c
> > @@ -299,7 +299,7 @@ grub_efi_get_variable (const char *var, const
> > grub_efi_guid_t *guid, /* Search the mods section from the
> > PE32/PE32+ image. This code uses a PE32 header, but should work
> > with PE32+ as well.  */ grub_addr_t
> > -grub_efi_modules_addr (void)
> > +grub_efi_section_addr (const char *section_name)
> >  {
> >    grub_efi_loaded_image_t *image;
> >    struct grub_msdos_image_header *header;
> > @@ -328,7 +328,7 @@ grub_efi_modules_addr (void)
> >         i < coff_header->num_sections;
> >         i++, section++)
> >      {
> > -      if (grub_strcmp (section->name, "mods") == 0)
> > +      if (grub_strcmp (section->name, section_name) == 0)
> >     break;
> >      }
> >
> > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> > index b67bc73a1b..ed3d463c8f 100644
> > --- a/grub-core/kern/efi/init.c
> > +++ b/grub-core/kern/efi/init.c
> > @@ -101,10 +101,24 @@ stack_protector_init (void)
> >
> >  grub_addr_t grub_modbase;
> >
> > +#if PRINT_GDB_SYM_LOAD_CMD
> > +static void
> > +grub_efi_print_gdb_info (void)
> > +{
> > +  grub_addr_t text;
> > +
> > +  text = grub_efi_section_addr (".text");
> > +  if (!text)
> > +    return;
> > +
> > +  grub_printf ("dynamic_load_symbols %p\n", (void *)text);
> > +}
> > +#endif
> > +
> >  void
> >  grub_efi_init (void)
> >  {
> > -  grub_modbase = grub_efi_modules_addr ();
> > +  grub_modbase = grub_efi_section_addr ("mods");
> >    /* First of all, initialize the console so that GRUB can display
> >       messages.  */
> >    grub_console_init ();
> > @@ -127,6 +141,9 @@ grub_efi_init (void)
> >    efi_call_4
> > (grub_efi_system_table->boot_services->set_watchdog_timer, 0, 0, 0,
> > NULL);
> >
> > +#if PRINT_GDB_SYM_LOAD_CMD
> > +  grub_efi_print_gdb_info ();
> > +#endif
> >    grub_efidisk_init ();
> >  }
> >
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index e61272de53..586ac856b5 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -109,7 +109,7 @@ grub_err_t
> > grub_arch_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
> > char *args); #endif
> >
> > -grub_addr_t grub_efi_modules_addr (void);
> > +grub_addr_t grub_efi_section_addr (const char *section);
> >
> >  void grub_efi_mm_init (void);
> >  void grub_efi_mm_fini (void);
> 
> Daniel