[RFC PATCH] gdb: Add more support for debugging on EFI platforms

[RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Glenn Washburn]

If the configure option --enable-efi-debug is given, then enable the
printing early in EFI startup of 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. This is not printed when secure boot is enabled.

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.

Also a command named "gdbinfo" is enabled which allows the user to print
the gdb command string on-demand, which can be valuable as the printing
early in EFI startup is quickly replaced by other text. So if using a
physical screen it may appear too briefly to be registered.

Co-developed-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
This is patch 9 from the v6 "GDB script fixes and improvements" series, with
one modification. Now the gdbinfo command will print the gdb load command
even when the configure option is not enabled (though still not when lockdown
is enabled).

Robbie had 2 concerns with the last patch.

 1. Does this need to be configurable?
   * I responded that this was requested by Daniel because of concerns about
     it breaking silent boot and it seemed reasonable to me, but that I don't
     have a strong opinion. I've left it configurable until Dnaiel weighs in.
 2. Why should the load command not be printed when secure boot is enabled?
   * This was also requested by Daniel, I assume because of infomation leakage
     that may be a security concern.

Glenn
---
 config.h.in                 |  3 +++
 configure.ac                | 11 ++++++++++
 grub-core/Makefile.core.def |  1 +
 grub-core/kern/efi/debug.c  | 38 ++++++++++++++++++++++++++++++++
 grub-core/kern/efi/efi.c    |  4 ++--
 grub-core/kern/efi/init.c   |  7 +++++-
 include/grub/efi/debug.h    | 43 +++++++++++++++++++++++++++++++++++++
 include/grub/efi/efi.h      |  2 +-
 8 files changed, 105 insertions(+), 4 deletions(-)
 create mode 100644 grub-core/kern/efi/debug.c
 create mode 100644 include/grub/efi/debug.h

diff --git a/config.h.in b/config.h.in
index 4d1e50eba79c..9f88be159367 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 @EFI_DEBUG@
+
 /* 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/configure.ac b/configure.ac
index 93626b7982d4..4bf477b0729a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1521,6 +1521,17 @@ else
 fi
 AC_SUBST([MM_DEBUG])
 
+# EFI debugging.
+AC_ARG_ENABLE([efi-debug],
+	      AS_HELP_STRING([--enable-efi-debug],
+                             [include aides for debugging the EFI binary]))
+if test x$enable_efi_debug = xyes; then
+  EFI_DEBUG=1
+else
+  EFI_DEBUG=0
+fi
+AC_SUBST([EFI_DEBUG])
+
 AC_ARG_ENABLE([cache-stats],
 	      AS_HELP_STRING([--enable-cache-stats],
                              [enable disk cache statistics collection]))
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index d2b1c3e9e44d..b7401c4b49e8 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -206,6 +206,7 @@ kernel = {
 
   efi = disk/efi/efidisk.c;
   efi = kern/efi/efi.c;
+  efi = kern/efi/debug.c;
   efi = kern/efi/init.c;
   efi = kern/efi/mm.c;
   efi = term/efi/console.c;
diff --git a/grub-core/kern/efi/debug.c b/grub-core/kern/efi/debug.c
new file mode 100644
index 000000000000..506ad90dca06
--- /dev/null
+++ b/grub-core/kern/efi/debug.c
@@ -0,0 +1,38 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+/* debug.c - aides for debugging the EFI application */
+
+#include <grub/efi/debug.h>
+#include <grub/command.h>
+#include <grub/i18n.h>
+
+static grub_err_t
+grub_cmd_gdbinfo (struct grub_command *cmd __attribute__ ((unused)),
+		  int argc __attribute__ ((unused)),
+		  char **args __attribute__ ((unused)))
+{
+  grub_efi_print_gdb_info ();
+  return 0;
+}
+
+void
+grub_efi_register_debug_commands (void)
+{
+  grub_register_command_lockdown ("gdbinfo", grub_cmd_gdbinfo, 0,
+				  N_("Print infomation useful for GDB debugging"));
+}
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index cf49d6357e00..17bd06f7e63a 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 b67bc73a1b01..edacaddf064b 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -19,6 +19,7 @@
 
 #include <grub/efi/efi.h>
 #include <grub/efi/console.h>
+#include <grub/efi/debug.h>
 #include <grub/efi/disk.h>
 #include <grub/efi/sb.h>
 #include <grub/lockdown.h>
@@ -104,7 +105,7 @@ grub_addr_t grub_modbase;
 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 ();
@@ -123,11 +124,15 @@ grub_efi_init (void)
       grub_lockdown ();
       grub_shim_lock_verifier_setup ();
     }
+  else
+    grub_efi_print_gdb_info ();
 
   efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
 	      0, 0, 0, NULL);
 
   grub_efidisk_init ();
+
+  grub_efi_register_debug_commands ();
 }
 
 void (*grub_efi_net_config) (grub_efi_handle_t hnd,
diff --git a/include/grub/efi/debug.h b/include/grub/efi/debug.h
new file mode 100644
index 000000000000..25414b78d9c6
--- /dev/null
+++ b/include/grub/efi/debug.h
@@ -0,0 +1,43 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+/* debug.h - declare variables and functions for EFI debugging support */
+
+#ifndef GRUB_EFI_DEBUG_HEADER
+#define GRUB_EFI_DEBUG_HEADER	1
+
+#include <grub/efi/efi.h>
+#include <grub/misc.h>
+
+
+void grub_efi_register_debug_commands (void);
+
+static inline void
+grub_efi_print_gdb_info (void)
+{
+#if PRINT_GDB_SYM_LOAD_CMD
+  grub_addr_t text;
+
+  text = grub_efi_section_addr (".text");
+  if (!text)
+    return;
+
+  grub_printf ("dynamic_load_symbols %p\n", (void *)text);
+#endif
+}
+
+#endif /* ! GRUB_EFI_DEBUG_HEADER */
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e61272de5330..586ac856b5e1 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);
-- 
2.34.1

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

Glenn Washburn <development@efficientek.com> writes:

> If the configure option --enable-efi-debug is given, then enable the
> printing early in EFI startup of 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. This is not printed when secure boot is enabled.
>
> 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.
>
> Also a command named "gdbinfo" is enabled which allows the user to print
> the gdb command string on-demand, which can be valuable as the printing
> early in EFI startup is quickly replaced by other text. So if using a
> physical screen it may appear too briefly to be registered.
>
> Co-developed-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> This is patch 9 from the v6 "GDB script fixes and improvements" series, with
> one modification. Now the gdbinfo command will print the gdb load command
> even when the configure option is not enabled (though still not when lockdown
> is enabled).
>
> Robbie had 2 concerns with the last patch.
>
>  1. Does this need to be configurable?
>    * I responded that this was requested by Daniel because of concerns about
>      it breaking silent boot and it seemed reasonable to me, but that I don't
>      have a strong opinion. I've left it configurable until Dnaiel weighs in.

Yeah, I think these concerns are valid.  The version in the rhboot tree
gates printing on an env var.  Right now, it seems to me that:

- we want it to be default-off because silent boot
- we want to have the ability to reenable without rebuilding because
  secureboot, convenience, etc.

>  2. Why should the load command not be printed when secure boot is enabled?
>    * This was also requested by Daniel, I assume because of infomation leakage
>      that may be a security concern.

I seem to have also missed Daniel's reply about this earlier, which was:

>> I think leaking info about the GRUB image addresses on the Secure
>> Boot enabled systems is not the best idea. Or do you think having
>> this feature enabled by default overweight potential dangers coming
>> from its misuse?

I don't know how these could help an attacker.  They'd need access to
console out to retrieve the values, and some way to send input... and
that's basically physical presence: at the very least, if they have
those, I imagine they'd just edit the menu entries, or drop to the grub
shell.

Do you see a danger here?

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Peter Jones]

On Thu, Mar 09, 2023 at 06:00:04PM -0500, Robbie Harwood wrote:
> Glenn Washburn <development@efficientek.com> writes:
> >  2. Why should the load command not be printed when secure boot is enabled?
> >    * This was also requested by Daniel, I assume because of infomation leakage
> >      that may be a security concern.
> 
> I seem to have also missed Daniel's reply about this earlier, which was:
> 
> >> I think leaking info about the GRUB image addresses on the Secure
> >> Boot enabled systems is not the best idea. Or do you think having
> >> this feature enabled by default overweight potential dangers coming
> >> from its misuse?
> 
> I don't know how these could help an attacker.  They'd need access to
> console out to retrieve the values, and some way to send input... and
> that's basically physical presence: at the very least, if they have
> those, I imagine they'd just edit the menu entries, or drop to the grub
> shell.

I'd like to also point out that we currently don't have anything like
ASLR in the pre-boot space, nor much hope of meaningfully getting it,
and typically have an identity memory mapped.  So, a serious attacker
can take a workflow like:

1) make a rough guess what kind of system they're attacking - vendor,
   product line, minimum memory size, etc.
2) get two or three similar machines on ebay
3) build a certificate chain that's got the exact same topology and cert
   sizes as the real one
4) enroll that the normal way
5) rebuild the grub binary with the "should we print this line" test
   inverted, so it's literally 1 bit different (i.e. jz -> jnz)
6) sign it
7) watch the load patterns on each of those machines and build a pretty
   simple model for candidate addresses
8) try them all when trying to exploit whatever bug they've found,
   either on individual attempts or possibly multiple at once if
   they're clever

If they're also using the shotgun method of identifying targets - i.e.
they've already used some other technique to get root on a bunch of
machines and now they're trying to escalate however many they can to
have Secure Boot breakouts - they're going to have a fair degree of
success with this method, and hiding the addresses will be an incredibly
minor speed bump for them.

I think worrying about exposing load addresses on the console with open
source firmware and no ASLR is not worth the trouble.

-- 
        Peter

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Daniel Kiper]

On Fri, Mar 10, 2023 at 03:27:13PM -0500, Peter Jones wrote:
> On Thu, Mar 09, 2023 at 06:00:04PM -0500, Robbie Harwood wrote:
> > Glenn Washburn <development@efficientek.com> writes:
> > >  2. Why should the load command not be printed when secure boot is enabled?
> > >    * This was also requested by Daniel, I assume because of infomation leakage
> > >      that may be a security concern.
> >
> > I seem to have also missed Daniel's reply about this earlier, which was:
> >
> > >> I think leaking info about the GRUB image addresses on the Secure
> > >> Boot enabled systems is not the best idea. Or do you think having
> > >> this feature enabled by default overweight potential dangers coming
> > >> from its misuse?
> >
> > I don't know how these could help an attacker.  They'd need access to
> > console out to retrieve the values, and some way to send input... and
> > that's basically physical presence: at the very least, if they have
> > those, I imagine they'd just edit the menu entries, or drop to the grub
> > shell.
>
> I'd like to also point out that we currently don't have anything like
> ASLR in the pre-boot space, nor much hope of meaningfully getting it,
> and typically have an identity memory mapped.  So, a serious attacker
> can take a workflow like:
>
> 1) make a rough guess what kind of system they're attacking - vendor,
>    product line, minimum memory size, etc.
> 2) get two or three similar machines on ebay
> 3) build a certificate chain that's got the exact same topology and cert
>    sizes as the real one
> 4) enroll that the normal way
> 5) rebuild the grub binary with the "should we print this line" test
>    inverted, so it's literally 1 bit different (i.e. jz -> jnz)
> 6) sign it
> 7) watch the load patterns on each of those machines and build a pretty
>    simple model for candidate addresses
> 8) try them all when trying to exploit whatever bug they've found,
>    either on individual attempts or possibly multiple at once if
>    they're clever
>
> If they're also using the shotgun method of identifying targets - i.e.
> they've already used some other technique to get root on a bunch of
> machines and now they're trying to escalate however many they can to
> have Secure Boot breakouts - they're going to have a fair degree of
> success with this method, and hiding the addresses will be an incredibly
> minor speed bump for them.
>
> I think worrying about exposing load addresses on the console with open
> source firmware and no ASLR is not worth the trouble.

Good points! I was thinking about using lsefimmap to leak info about
memory usage. Of course this does not directly shows you GRUB memory
allocations but it is enough to infer them by comparison of lists of
memory regions with and without the GRUB. So, taking into account above
it looks printing the GRUB load address does not give potential attacker
much advantage and we can print the load address unconditionally. Until
I do not hear otherwise...

Daniel

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Glenn Washburn]

On 3/9/23 23:00, Robbie Harwood wrote:
> Glenn Washburn <development@efficientek.com> writes:
> 
>> If the configure option --enable-efi-debug is given, then enable the
>> printing early in EFI startup of 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. This is not printed when secure boot is enabled.
>>
>> 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.
>>
>> Also a command named "gdbinfo" is enabled which allows the user to print
>> the gdb command string on-demand, which can be valuable as the printing
>> early in EFI startup is quickly replaced by other text. So if using a
>> physical screen it may appear too briefly to be registered.
>>
>> Co-developed-by: Peter Jones <pjones@redhat.com>
>> Signed-off-by: Glenn Washburn <development@efficientek.com>
>> ---
>> This is patch 9 from the v6 "GDB script fixes and improvements" series, with
>> one modification. Now the gdbinfo command will print the gdb load command
>> even when the configure option is not enabled (though still not when lockdown
>> is enabled).
>>
>> Robbie had 2 concerns with the last patch.
>>
>>   1. Does this need to be configurable?
>>     * I responded that this was requested by Daniel because of concerns about
>>       it breaking silent boot and it seemed reasonable to me, but that I don't
>>       have a strong opinion. I've left it configurable until Dnaiel weighs in.
> 
> Yeah, I think these concerns are valid.  The version in the rhboot tree
> gates printing on an env var.  Right now, it seems to me that:
> 
> - we want it to be default-off because silent boot

I understand you to be talking about a default-off at runtime, not built 
time. Right now there is a configure option which defaults to off, is 
this acceptable?

> - we want to have the ability to reenable without rebuilding because
>    secureboot, convenience, etc.

This would be great, but how do you propose that this would work? This 
patch will print very early in EFI init. We can't use GRUB variables. We 
probably could use EFI variables, but this needs to be well defined (and 
not by me, since I don't have this requirement). I'm not sure if the 
GRUB env block is available at this point, but that might be an option.

Can you point me to RH's patch you've referred to? Does it meet this 
requirement, and if so how?

Glenn

> 
>>   2. Why should the load command not be printed when secure boot is enabled?
>>     * This was also requested by Daniel, I assume because of infomation leakage
>>       that may be a security concern.
> 
> I seem to have also missed Daniel's reply about this earlier, which was:
> 
>>> I think leaking info about the GRUB image addresses on the Secure
>>> Boot enabled systems is not the best idea. Or do you think having
>>> this feature enabled by default overweight potential dangers coming
>>> from its misuse?
> 
> I don't know how these could help an attacker.  They'd need access to
> console out to retrieve the values, and some way to send input... and
> that's basically physical presence: at the very least, if they have
> those, I imagine they'd just edit the menu entries, or drop to the grub
> shell.
> 
> Do you see a danger here?
> 
> Be well,
> --Robbie

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Glenn Washburn]

On 3/13/23 12:27, Daniel Kiper wrote:
> On Fri, Mar 10, 2023 at 03:27:13PM -0500, Peter Jones wrote:
>> On Thu, Mar 09, 2023 at 06:00:04PM -0500, Robbie Harwood wrote:
>>> Glenn Washburn <development@efficientek.com> writes:
>>>>   2. Why should the load command not be printed when secure boot is enabled?
>>>>     * This was also requested by Daniel, I assume because of infomation leakage
>>>>       that may be a security concern.
>>>
>>> I seem to have also missed Daniel's reply about this earlier, which was:
>>>
>>>>> I think leaking info about the GRUB image addresses on the Secure
>>>>> Boot enabled systems is not the best idea. Or do you think having
>>>>> this feature enabled by default overweight potential dangers coming
>>>>> from its misuse?
>>>
>>> I don't know how these could help an attacker.  They'd need access to
>>> console out to retrieve the values, and some way to send input... and
>>> that's basically physical presence: at the very least, if they have
>>> those, I imagine they'd just edit the menu entries, or drop to the grub
>>> shell.
>>
>> I'd like to also point out that we currently don't have anything like
>> ASLR in the pre-boot space, nor much hope of meaningfully getting it,
>> and typically have an identity memory mapped.  So, a serious attacker
>> can take a workflow like:
>>
>> 1) make a rough guess what kind of system they're attacking - vendor,
>>     product line, minimum memory size, etc.
>> 2) get two or three similar machines on ebay
>> 3) build a certificate chain that's got the exact same topology and cert
>>     sizes as the real one
>> 4) enroll that the normal way
>> 5) rebuild the grub binary with the "should we print this line" test
>>     inverted, so it's literally 1 bit different (i.e. jz -> jnz)
>> 6) sign it
>> 7) watch the load patterns on each of those machines and build a pretty
>>     simple model for candidate addresses
>> 8) try them all when trying to exploit whatever bug they've found,
>>     either on individual attempts or possibly multiple at once if
>>     they're clever
>>
>> If they're also using the shotgun method of identifying targets - i.e.
>> they've already used some other technique to get root on a bunch of
>> machines and now they're trying to escalate however many they can to
>> have Secure Boot breakouts - they're going to have a fair degree of
>> success with this method, and hiding the addresses will be an incredibly
>> minor speed bump for them.
>>
>> I think worrying about exposing load addresses on the console with open
>> source firmware and no ASLR is not worth the trouble.
> 
> Good points! I was thinking about using lsefimmap to leak info about
> memory usage. Of course this does not directly shows you GRUB memory
> allocations but it is enough to infer them by comparison of lists of
> memory regions with and without the GRUB. So, taking into account above
> it looks printing the GRUB load address does not give potential attacker
> much advantage and we can print the load address unconditionally. Until
> I do not hear otherwise...

Ok, so I'm understanding that I can move the printing of the gdb command 
out of the !secureboot condition and have it always print. I'm also 
understanding that I should not disable the gdbinfo command in lockdown 
mode, correct?

Glenn

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Daniel Kiper]

On Wed, Mar 15, 2023 at 03:11:05AM +0000, Glenn Washburn wrote:
> On 3/13/23 12:27, Daniel Kiper wrote:
> > On Fri, Mar 10, 2023 at 03:27:13PM -0500, Peter Jones wrote:
> > > On Thu, Mar 09, 2023 at 06:00:04PM -0500, Robbie Harwood wrote:
> > > > Glenn Washburn <development@efficientek.com> writes:
> > > > >   2. Why should the load command not be printed when secure boot is enabled?
> > > > >     * This was also requested by Daniel, I assume because of infomation leakage
> > > > >       that may be a security concern.
> > > >
> > > > I seem to have also missed Daniel's reply about this earlier, which was:
> > > >
> > > > > > I think leaking info about the GRUB image addresses on the Secure
> > > > > > Boot enabled systems is not the best idea. Or do you think having
> > > > > > this feature enabled by default overweight potential dangers coming
> > > > > > from its misuse?
> > > >
> > > > I don't know how these could help an attacker.  They'd need access to
> > > > console out to retrieve the values, and some way to send input... and
> > > > that's basically physical presence: at the very least, if they have
> > > > those, I imagine they'd just edit the menu entries, or drop to the grub
> > > > shell.
> > >
> > > I'd like to also point out that we currently don't have anything like
> > > ASLR in the pre-boot space, nor much hope of meaningfully getting it,
> > > and typically have an identity memory mapped.  So, a serious attacker
> > > can take a workflow like:
> > >
> > > 1) make a rough guess what kind of system they're attacking - vendor,
> > >     product line, minimum memory size, etc.
> > > 2) get two or three similar machines on ebay
> > > 3) build a certificate chain that's got the exact same topology and cert
> > >     sizes as the real one
> > > 4) enroll that the normal way
> > > 5) rebuild the grub binary with the "should we print this line" test
> > >     inverted, so it's literally 1 bit different (i.e. jz -> jnz)
> > > 6) sign it
> > > 7) watch the load patterns on each of those machines and build a pretty
> > >     simple model for candidate addresses
> > > 8) try them all when trying to exploit whatever bug they've found,
> > >     either on individual attempts or possibly multiple at once if
> > >     they're clever
> > >
> > > If they're also using the shotgun method of identifying targets - i.e.
> > > they've already used some other technique to get root on a bunch of
> > > machines and now they're trying to escalate however many they can to
> > > have Secure Boot breakouts - they're going to have a fair degree of
> > > success with this method, and hiding the addresses will be an incredibly
> > > minor speed bump for them.
> > >
> > > I think worrying about exposing load addresses on the console with open
> > > source firmware and no ASLR is not worth the trouble.
> >
> > Good points! I was thinking about using lsefimmap to leak info about
> > memory usage. Of course this does not directly shows you GRUB memory
> > allocations but it is enough to infer them by comparison of lists of
> > memory regions with and without the GRUB. So, taking into account above
> > it looks printing the GRUB load address does not give potential attacker
> > much advantage and we can print the load address unconditionally. Until
> > I do not hear otherwise...
>
> Ok, so I'm understanding that I can move the printing of the gdb command out
> of the !secureboot condition and have it always print. I'm also

Modulo silent boot... I hope Robbie help us to define how exactly it
should work.

> understanding that I should not disable the gdbinfo command in lockdown
> mode, correct?

Yep!

Daniel

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]

Glenn Washburn <development@efficientek.com> writes:

> On 3/9/23 23:00, Robbie Harwood wrote:
>> Glenn Washburn <development@efficientek.com> writes:
>> 
>>> If the configure option --enable-efi-debug is given, then enable the
>>> printing early in EFI startup of 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. This is not printed when secure boot is enabled.
>>>
>>> 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.
>>>
>>> Also a command named "gdbinfo" is enabled which allows the user to print
>>> the gdb command string on-demand, which can be valuable as the printing
>>> early in EFI startup is quickly replaced by other text. So if using a
>>> physical screen it may appear too briefly to be registered.
>>>
>>> Co-developed-by: Peter Jones <pjones@redhat.com>
>>> Signed-off-by: Glenn Washburn <development@efficientek.com>
>>> ---
>>> This is patch 9 from the v6 "GDB script fixes and improvements" series, with
>>> one modification. Now the gdbinfo command will print the gdb load command
>>> even when the configure option is not enabled (though still not when lockdown
>>> is enabled).
>>>
>>> Robbie had 2 concerns with the last patch.
>>>
>>>   1. Does this need to be configurable?
>>>     * I responded that this was requested by Daniel because of concerns about
>>>       it breaking silent boot and it seemed reasonable to me, but that I don't
>>>       have a strong opinion. I've left it configurable until Dnaiel weighs in.
>> 
>> Yeah, I think these concerns are valid.  The version in the rhboot
>> tree gates printing on an env var.  Right now, it seems to me that:
>> 
>> - we want it to be default-off because silent boot
>
> I understand you to be talking about a default-off at runtime, not
> built time. Right now there is a configure option which defaults to
> off, is this acceptable?

Indeed, I'm talking about runtime configurability.  Build-time
configurability means it's either always on (bad) or we have to rebuild
in order to debug (annoying, interacts poorly with scureboot).

>> - we want to have the ability to reenable without rebuilding because
>>    secureboot, convenience, etc.
>
> This would be great, but how do you propose that this would work? This 
> patch will print very early in EFI init. We can't use GRUB variables. We 
> probably could use EFI variables, but this needs to be well defined (and 
> not by me, since I don't have this requirement). I'm not sure if the 
> GRUB env block is available at this point, but that might be an option.
>
> Can you point me to RH's patch you've referred to? Does it meet this 
> requirement, and if so how?

I thought you were basing your work off it, given the "Co-developed-by"
and your review on v1:
https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00076.html

(v2 was
https://lists.gnu.org/archive/html/grub-devel/2021-11/msg00006.html and
the versions we're shipping are
https://github.com/rhboot/grub2/commit/93803e83135e074ed5a3e7f67af22538896dbefe
and
https://github.com/rhboot/grub2/commit/c314270222b4adffa16840e3ca764d793e93a0e8
)

As you say, the grub env block isn't available (module loading happens
before filesystems).  We have an additional patch that allows the use of
an EFI variable for setting grubenv:
https://github.com/rhboot/grub2/commit/22a11bfa59ff8f239cfe8698274281bc919673dc
which lets one do e.g.

    grub2-editenv ./grubenv set debug=gdb,modules,dl
    { printf '\x07\x00\x00\x00' ; cat ./grubenv ; } > GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8
    cp GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8 /sys/firmware/efi/efivars/

and then have configuration for the next boot.  Javier informally
proposed this to the list a few years ago for ZFS-related things:
https://lists.gnu.org/archive/html/grub-devel/2020-02/msg00045.html but
I don't think it was ever followed up on, possibly because ZFS is not
one of our use cases :)

I'm happy to formally propose it, or feel free to include/adapt it if
it's helpful to you, etc..

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [RFC PATCH] gdb: Add more support for debugging on EFI platforms

[Glenn Washburn]

On 3/16/23 21:05, Robbie Harwood wrote:
> Glenn Washburn <development@efficientek.com> writes:
> 
>> On 3/9/23 23:00, Robbie Harwood wrote:
>>> Glenn Washburn <development@efficientek.com> writes:
>>>
>>>> If the configure option --enable-efi-debug is given, then enable the
>>>> printing early in EFI startup of 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. This is not printed when secure boot is enabled.
>>>>
>>>> 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.
>>>>
>>>> Also a command named "gdbinfo" is enabled which allows the user to print
>>>> the gdb command string on-demand, which can be valuable as the printing
>>>> early in EFI startup is quickly replaced by other text. So if using a
>>>> physical screen it may appear too briefly to be registered.
>>>>
>>>> Co-developed-by: Peter Jones <pjones@redhat.com>
>>>> Signed-off-by: Glenn Washburn <development@efficientek.com>
>>>> ---
>>>> This is patch 9 from the v6 "GDB script fixes and improvements" series, with
>>>> one modification. Now the gdbinfo command will print the gdb load command
>>>> even when the configure option is not enabled (though still not when lockdown
>>>> is enabled).
>>>>
>>>> Robbie had 2 concerns with the last patch.
>>>>
>>>>    1. Does this need to be configurable?
>>>>      * I responded that this was requested by Daniel because of concerns about
>>>>        it breaking silent boot and it seemed reasonable to me, but that I don't
>>>>        have a strong opinion. I've left it configurable until Dnaiel weighs in.
>>>
>>> Yeah, I think these concerns are valid.  The version in the rhboot
>>> tree gates printing on an env var.  Right now, it seems to me that:
>>>
>>> - we want it to be default-off because silent boot
>>
>> I understand you to be talking about a default-off at runtime, not
>> built time. Right now there is a configure option which defaults to
>> off, is this acceptable?
> 
> Indeed, I'm talking about runtime configurability.  Build-time
> configurability means it's either always on (bad) or we have to rebuild
> in order to debug (annoying, interacts poorly with scureboot).
> 
>>> - we want to have the ability to reenable without rebuilding because
>>>     secureboot, convenience, etc.
>>
>> This would be great, but how do you propose that this would work? This
>> patch will print very early in EFI init. We can't use GRUB variables. We
>> probably could use EFI variables, but this needs to be well defined (and
>> not by me, since I don't have this requirement). I'm not sure if the
>> GRUB env block is available at this point, but that might be an option.
>>
>> Can you point me to RH's patch you've referred to? Does it meet this
>> requirement, and if so how?
> 
> I thought you were basing your work off it, given the "Co-developed-by"
> and your review on v1:
> https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00076.html

Yes, I am off that patch. I was thinking things had changed or there 
were other patches to get the requirement met. Thanks for providing the 
info anyway. Looks like nothing has changed and the missing piece is the 
envblk from an EFI variable patch.

> 
> (v2 was
> https://lists.gnu.org/archive/html/grub-devel/2021-11/msg00006.html and
> the versions we're shipping are
> https://github.com/rhboot/grub2/commit/93803e83135e074ed5a3e7f67af22538896dbefe
> and
> https://github.com/rhboot/grub2/commit/c314270222b4adffa16840e3ca764d793e93a0e8
> )
> 
> As you say, the grub env block isn't available (module loading happens
> before filesystems).  We have an additional patch that allows the use of
> an EFI variable for setting grubenv:
> https://github.com/rhboot/grub2/commit/22a11bfa59ff8f239cfe8698274281bc919673dc

Looks like the above commit relies on this commit:

https://github.com/rhboot/grub2/commit/636612cec23868d9165d79f2c3e2a9ea6c278c38

> which lets one do e.g.
> 
>      grub2-editenv ./grubenv set debug=gdb,modules,dl
>      { printf '\x07\x00\x00\x00' ; cat ./grubenv ; } > GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8
>      cp GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8 /sys/firmware/efi/efivars/

A bit off topic, but perhaps this could be wrapped in an option to 
grub2-editenv and doing something like:

   grub2-editenv -E set debug=gdb,modules,dl

> and then have configuration for the next boot.  Javier informally
> proposed this to the list a few years ago for ZFS-related things:
> https://lists.gnu.org/archive/html/grub-devel/2020-02/msg00045.html but
> I don't think it was ever followed up on, possibly because ZFS is not
> one of our use cases :)
> 
> I'm happy to formally propose it, or feel free to include/adapt it if
> it's helpful to you, etc..

The envblk EFI variable seems to me to be a nice feature to have, so I'm 
supportive in general. One concern I have is that this efi-envblk 
feature is a non-trivial amount of code for this particular patch to be 
dependent on. So if the efi-envblk is proposed and doesn't get included 
for this release cycle, neither will this patch. I would say yes to 
submitting the efi-envblk patches, regardless of what we do with this 
patch. And I'd like to hear Daniel's thoughts on this.

Glenn