Re: [RFC PATCH v2 1/1] kern/dl: Add module version check

[Glenn Washburn]

On Wed, 21 Dec 2022 20:11:57 +0800
Zhang Boyang  wrote:

> This patch add version information to GRUB modules. Specifically,
> PACKAGE_VERSION is embedded as null-terminated string in .modver
> section. This string is checked at module loading time.
> 
> If GRUB is locked down, modules with mismatched version will be
> rejected. This is necessary for implementing external signed modules
> securely (in future).
> 
> For compatibility reasons, if GRUB isn't locked down, modules can
> still be loaded even if they have mismatched version. A warning
> message will be emited in that situation.
> 
> It should be pointed out that the version string is only consisted of
> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is
> not noticed by version check (e.g. configure options).
> 
> Link:
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
> Link:
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html
> Signed-off-by: Zhang Boyang  ---
> grub-core/Makefile.am |  2 +- grub-core/genmod.sh.in|
> 28 +--- grub-core/kern/dl.c   | 33
> + util/grub-module-verifierXX.c | 10
> ++ 4 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index 80e7a83ed..d76aa80f4 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
>  CLEANFILES += gentrigtables$(BUILD_EXEEXT)
>  
>  build-grub-module-verifier$(BUILD_EXEEXT):
> $(top_srcdir)/util/grub-module-verifier.c
> $(top_srcdir)/util/grub-module-verifier32.c
> $(top_srcdir)/util/grub-module-verifier64.c
> $(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
> - $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
> $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
> -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
> + $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
> $(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
> -DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\"
> -DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^ CLEANFILES +=
> build-grub-module-verifier$(BUILD_EXEEXT) # trigtables.c diff --git
> a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in index
> e57c4d920..58a4cdcbe 100644 --- a/grub-core/genmod.sh.in
> +++ b/grub-core/genmod.sh.in
> @@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
>  rm -f $tmpfile $outfile
>  
>  if test x@TARGET_APPLE_LINKER@ != x1; then
> -# stripout .modname and .moddeps sections from input module
> -@TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
> +# stripout .modname and .moddeps and .modver sections from input
> module
> +@TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile
> $tmpfile 
> -# Attach .modname and .moddeps sections
> +# Attach .modname and .moddeps and .modver sections
>  t1=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
>  printf "$modname\0" >$t1
>  
>  t2=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
>  for dep in $deps; do printf "$dep\0" >> $t2; done
>  
> +t3=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
> +printf "@PACKAGE_VERSION@\0" >$t3
> +
>  if test -n "$deps"; then
> - @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .moddeps=$t2 $tmpfile
> + @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .moddeps=$t2 --add-section .modver=$t3 $tmpfile else
> - @TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
> + @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section
> .modver=$t3 $tmpfile fi
> -rm -f $t1 $t2
> +rm -f $t1 $t2 $t3
>  
>   if test x@platform@ != xemu; then
>   @TARGET_STRIP@ --strip-unneeded \
> @@ -71,23 +74,26 @@ else
>  tmpfile2=${outfile}.tmp2
>  t1=${outfile}.t1.c
>  t2=${outfile}.t2.c
> +t3=${outfile}.t3.c
>  
>  # remove old files if any
> -rm -f $t1 $t2
> +rm -f $t1 $t2 $t3
>  
>  cp $infile $tmpfile
>  
> -# Attach .modname and .moddeps sections
> +# Attach .modname and .moddeps and .modver sections
>  echo "char modname[]  __attribute__ ((section(\"_modname,
> _modname\"))) = \"$modname\";" >$t1 
>  for dep in $deps; do echo "char moddep_$dep[] __attribute__
> ((section(\"_moddeps, _moddeps\"))) = \"$dep\";" >>$t2; done 
> +echo "char modver[]  __attribute__ ((section(\"_modver,
> _modver\"))) = \"@PACKAGE_VERSION@\";" >$t3 +
>  if test -n "$deps"; then
> - @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t2 $tmpfile -Wl,-r
> + @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t2 $t3 $tmpfile -Wl,-r else
> - @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $tmpfile -Wl,-r
> + @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o
> $tmpfile2 $t1 $t3 

[PATCH v2 5/8] ns8250: Add configuration parameter when adding ports

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt 

This will allow ports to be added with a pre-set configuration

Signed-off-by: Benjamin Herrenschmidt 
---
 grub-core/term/ns8250.c | 27 ---
 grub-core/term/serial.c |  2 +-
 include/grub/serial.h   |  4 ++--
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 9e0ebff5a..d9d93fcf8 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -304,7 +304,7 @@ grub_ns8250_hw_get_port (const unsigned int unit)
 }
 
 char *
-grub_serial_ns8250_add_port (grub_port_t port)
+grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config 
*config)
 {
   struct grub_serial_port *p;
   unsigned i;
@@ -314,6 +314,9 @@ grub_serial_ns8250_add_port (grub_port_t port)
   {
if (dead_ports & (1 << i))
  return NULL;
+   /* give the opportunity for SPCR to configure a default com port */
+   if (config != NULL)
+ grub_serial_port_configure (_ports[i], config);
return com_names[i];
   }
 
@@ -326,32 +329,39 @@ grub_serial_ns8250_add_port (grub_port_t port)
 return NULL;
 
   p = grub_malloc (sizeof (*p));
-  if (!p)
+  if (p == NULL)
 return NULL;
   p->name = grub_xasprintf ("port%lx", (unsigned long) port);
-  if (!p->name)
+  if (p->name == NULL)
 {
   grub_free (p);
   return NULL;
 }
   p->driver = _ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = false;
   p->port = port;
+  if (config != NULL)
+grub_serial_port_configure (p, config);
+  else
+grub_serial_config_defaults (p);
   grub_serial_register (p);
 
   return p->name;
 }
 
 char *
-grub_serial_ns8250_add_mmio (grub_addr_t addr)
+grub_serial_ns8250_add_mmio (grub_addr_t addr, struct grub_serial_config 
*config)
 {
   struct grub_serial_port *p;
   unsigned i;
 
   for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
 if (com_ports[i].mmio && com_ports[i].mmio_base == addr)
-   return com_names[i];
+  {
+if (config != NULL)
+  grub_serial_port_configure (_ports[i], config);
+return com_names[i];
+  }
 
   p = grub_malloc (sizeof (*p));
   if (p == NULL)
@@ -363,9 +373,12 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr)
   return NULL;
 }
   p->driver = _ns8250_driver;
-  grub_serial_config_defaults (p);
   p->mmio = true;
   p->mmio_base = addr;
+  if (config != NULL)
+grub_serial_port_configure (p, config);
+  else
+grub_serial_config_defaults (p);
   grub_serial_register (p);
 
   return p->name;
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index c8f5f02db..a1707d2f7 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -156,7 +156,7 @@ grub_serial_find (const char *name)
   && grub_isxdigit (name [sizeof ("port") - 1]))
 {
   name = grub_serial_ns8250_add_port (grub_strtoul ([sizeof ("port") 
- 1],
-   0, 16));
+   0, 16), NULL);
   if (!name)
return NULL;
 
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 5e008f0f5..a94327140 100644
--- a/include/grub/serial.h
+++ b/include/grub/serial.h
@@ -185,8 +185,8 @@ grub_serial_config_defaults (struct grub_serial_port *port)
 
 #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
 void grub_ns8250_init (void);
-char *grub_serial_ns8250_add_port (grub_port_t port);
-char *grub_serial_ns8250_add_mmio (grub_addr_t addr);
+char *grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config 
*config);
+char *grub_serial_ns8250_add_mmio (grub_addr_t addr, struct grub_serial_config 
*config);
 #endif
 #ifdef GRUB_MACHINE_IEEE1275
 void grub_ofserial_init (void);
-- 
2.34.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 4/8] ns8250: Move base clock definition to a header

[Benjamin Herrenschmidt]

And while at it, unify it as clock frequency in HZ, to match the
value in "struct grub_serial_config" and do the division by
16 in one common place.

This will simplify adding SPCR support.

Signed-off-by: Benjamin Herrenschmidt 
---
 grub-core/term/ns8250.c | 15 ---
 include/grub/ns8250.h   | 13 +
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 43b9b3719..9e0ebff5a 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -37,12 +37,6 @@ static const grub_port_t serial_hw_io_addr[] = 
GRUB_MACHINE_SERIAL_PORTS;
 
 static int dead_ports = 0;
 
-#ifdef GRUB_MACHINE_MIPS_LOONGSON
-#define DEFAULT_BASE_CLOCK (2 * 115200)
-#else
-#define DEFAULT_BASE_CLOCK 115200
-#endif
-
 static grub_uint8_t
 ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
 {
@@ -71,7 +65,14 @@ serial_get_divisor (const struct grub_serial_port *port 
__attribute__ ((unused))
   grub_uint32_t divisor;
   grub_uint32_t actual_speed, error;
 
-  base_clock = config->base_clock ? (config->base_clock >> 4) : 
DEFAULT_BASE_CLOCK;
+  /* Get the UART input clock frequency */
+  base_clock = config->base_clock ? config->base_clock : 
UART_DEFAULT_BASE_CLOCK;
+
+  /*
+   * The UART uses 16 times oversampling for the BRG, so adjust the value
+   * accordingly to calculate the divisor.
+   */
+  base_clock >>= 4;
 
   divisor = (base_clock + (config->speed / 2)) / config->speed;
   if (config->speed == 0)
diff --git a/include/grub/ns8250.h b/include/grub/ns8250.h
index 7947ba9c9..697637912 100644
--- a/include/grub/ns8250.h
+++ b/include/grub/ns8250.h
@@ -70,6 +70,19 @@
 /* Turn on DTR, RTS, and OUT2.  */
 #define UART_ENABLE_OUT2   0x08
 
+/*
+ * Default clock input of the UART (feeds the baud rate generator)
+ *
+ * The standard value here is 1.8432 Mhz, which corresponds to
+ * 115200 bauds * 16 (16 times oversampling).
+ *
+ */
+#ifdef GRUB_MACHINE_MIPS_LOONGSON
+#define UART_DEFAULT_BASE_CLOCK ((2 * 115200) << 4)
+#else
+#define UART_DEFAULT_BASE_CLOCK (115200 << 4)
+#endif
+
 #ifndef ASM_FILE
 #include 
 
-- 
2.34.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 8/8] serial: Add ability to specify MMIO ports via 'serial'

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt 

This adds the ability to explicitely add an MMIO based serial port
via the 'serial' command. The syntax is:

serial --port=mmio,{.b,.w,.l,.q}

Signed-off-by: Benjamin Herrenschmidt 
---
 docs/grub.texi  | 26 ++--
 grub-core/term/serial.c | 53 +
 2 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index fd22a69fa..502ca2ef7 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the 
command-line.
 @deffn Command serial [@option{--unit=unit}] [@option{--port=port}] 
[@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] 
[@option{--stop=stop}]
 Initialize a serial device. @var{unit} is a number in the range 0-3
 specifying which serial port to use; default is 0, which corresponds to
-the port often called COM1. @var{port} is the I/O port where the UART
-is to be found; if specified it takes precedence over @var{unit}.
+the port often called COM1.
+
+@var{port} is the I/O port where the UART is to be found or, if prefixed
+with @samp{mmio,}, the MMIO address of the UART. If specified it takes
+precedence over @var{unit}.
+
+Additionally, an MMIO address can be suffixed with:
+@itemize @bullet
+@item
+@samp{.b} for bytes access (default)
+@item
+@samp{.w} for 16-bit word access
+@item
+@samp{.l} for 32-bit long word access or
+@item
+@samp{.q} for 64-bit long long word access
+@end itemize
+
 @var{speed} is the transmission speed; default is 9600. @var{word} and
 @var{stop} are the number of data bits and stop bits. Data bits must
 be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
@@ -4185,6 +4201,12 @@ The serial port is not used as a communication channel 
unless the
 @command{terminal_input} or @command{terminal_output} command is used
 (@pxref{terminal_input}, @pxref{terminal_output}).
 
+Examples:
+@example
+serial --port=3f8 --speed=9600
+serial --port=mmio,fefb.l --speed=115200
+@end example
+
 See also @ref{Serial terminal}.
 @end deffn
 
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index ed7b398b7..d374495cb 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -152,8 +152,8 @@ grub_serial_find (const char *name)
   break;
 
 #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) && 
!defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
-  if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
-  && grub_isxdigit (name [sizeof ("port") - 1]))
+  if (!port && grub_strncmp (name, "port", grub_strlen ("port")) == 0
+  && grub_isxdigit (name [grub_strlen ("port")]))
 {
   name = grub_serial_ns8250_add_port (grub_strtoul ([sizeof ("port") 
- 1],
0, 16), NULL);
@@ -164,6 +164,49 @@ grub_serial_find (const char *name)
 if (grub_strcmp (port->name, name) == 0)
break;
 }
+  if (!port && grub_strncmp (name, "mmio,", grub_strlen ("mmio,")) == 0
+  && grub_isxdigit (name [grub_strlen ("mmio,")]))
+{
+  const char *p1, *p = [grub_strlen ("mmio,")];
+  grub_addr_t addr = grub_strtoul (p, , 16);
+  unsigned int acc_size = 1;
+  unsigned int nlen = p1 - p;
+
+  /* Check address validity and general syntax */
+  if (addr == 0 || (p1[0] != '\0' && p1[0] != '.'))
+return NULL;
+  if (p1[0] == '.')
+switch(p1[1])
+  {
+  case 'w':
+acc_size = 2;
+break;
+  case 'l':
+acc_size = 3;
+break;
+  case 'q':
+acc_size = 4;
+break;
+  case 'b':
+acc_size = 1;
+   /* fallthrough */
+ default:
+   /*
+* Should we abort for an unknown size ? Let's just default
+* to 1 byte, it would increase the chance that the user who
+* did a typo can actually see the console.
+*/
+   acc_size = 1;
+  }
+  name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
+  if (!name)
+return NULL;
+
+  FOR_SERIAL_PORTS (port)
+if (grub_strncmp (port->name, name, nlen) == 0) {
+  break;
+}
+}
   if (!port && grub_strcmp (name, "auto") == 0)
 {
   /* Look for an SPCR if any. If not, default to com0 */
@@ -178,9 +221,9 @@ grub_serial_find (const char *name)
 #endif
 
 #ifdef GRUB_MACHINE_IEEE1275
-  if (!port && grub_memcmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
+  if (!port && grub_strncmp (name, "ieee1275/", grub_strlen ("ieee1275/")) == 
0)
 {
-  name = grub_ofserial_add_port ([sizeof ("ieee1275/") - 1]);
+  name = grub_ofserial_add_port ([grub_strlen ("ieee1275/")]);
   if (!name)
return NULL;
 
@@ -212,7 +255,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char 
**args)
 
   if 

[PATCH v2 3/8] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt 

This adds the ability for the driver to access UARTs via MMIO instead
of PIO selectively at runtime, and exposes a new function to add an
MMIO port.

In an ideal world, MMIO accessors would be generic and have architecture
specific memory barriers. However, existing drivers don't have them and
most of those "bare metal" drivers tend to be for x86 which doesn't need
them. If necessary, those can be added later.

Signed-off-by: Benjamin Herrenschmidt 
---
 grub-core/term/ns8250.c | 80 -
 grub-core/term/serial.c |  8 +++--
 include/grub/serial.h   | 11 +-
 3 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index 83b25990c..43b9b3719 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -43,6 +43,24 @@ static int dead_ports = 0;
 #define DEFAULT_BASE_CLOCK 115200
 #endif
 
+static grub_uint8_t
+ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+  return grub_inb (port->port + reg);
+}
+
+static void
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+{
+  asm volatile("" : : : "memory");
+  if (port->mmio)
+*((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+  else
+grub_outb (value, port->port + reg);
+}
 
 /* Convert speed to divisor.  */
 static unsigned short
@@ -93,43 +111,42 @@ do_real_config (struct grub_serial_port *port)
   divisor = serial_get_divisor (port, >config);
 
   /* Turn off the interrupt.  */
-  grub_outb (0, port->port + UART_IER);
+  ns8250_reg_write (port, 0, UART_IER);
 
   /* Set DLAB.  */
-  grub_outb (UART_DLAB, port->port + UART_LCR);
+  ns8250_reg_write (port, UART_DLAB, UART_LCR);
 
-  /* Set the baud rate.  */
-  grub_outb (divisor & 0xFF, port->port + UART_DLL);
-  grub_outb (divisor >> 8, port->port + UART_DLH);
+  ns8250_reg_write (port, divisor & 0xFF, UART_DLL);
+  ns8250_reg_write (port, divisor >> 8, UART_DLH);
 
   /* Set the line status.  */
   status |= (parities[port->config.parity]
 | (port->config.word_len - 5)
 | stop_bits[port->config.stop_bits]);
-  grub_outb (status, port->port + UART_LCR);
+  ns8250_reg_write (port, status, UART_LCR);
 
   if (port->config.rtscts)
 {
   /* Enable the FIFO.  */
-  grub_outb (UART_ENABLE_FIFO_TRIGGER1, port->port + UART_FCR);
+  ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER1, UART_FCR);
 
   /* Turn on DTR and RTS.  */
-  grub_outb (UART_ENABLE_DTRRTS, port->port + UART_MCR);
+  ns8250_reg_write (port, UART_ENABLE_DTRRTS, UART_MCR);
 }
   else
 {
   /* Enable the FIFO.  */
-  grub_outb (UART_ENABLE_FIFO_TRIGGER14, port->port + UART_FCR);
+  ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER14, UART_FCR);
 
   /* Turn on DTR, RTS, and OUT2.  */
-  grub_outb (UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, port->port + UART_MCR);
+  ns8250_reg_write (port, UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, UART_MCR);
 }
 
   /* Drain the input buffer.  */
   endtime = grub_get_time_ms () + 1000;
-  while (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
+  while (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
 {
-  grub_inb (port->port + UART_RX);
+  ns8250_reg_read (port, UART_RX);
   if (grub_get_time_ms () > endtime)
{
  port->broken = 1;
@@ -145,8 +162,8 @@ static int
 serial_hw_fetch (struct grub_serial_port *port)
 {
   do_real_config (port);
-  if (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
-return grub_inb (port->port + UART_RX);
+  if (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
+return ns8250_reg_read (port, UART_RX);
 
   return -1;
 }
@@ -166,7 +183,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
   else
 endtime = grub_get_time_ms () + 200;
   /* Wait until the transmitter holding register is empty.  */
-  while ((grub_inb (port->port + UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
+  while ((ns8250_reg_read (port, UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
 {
   if (grub_get_time_ms () > endtime)
{
@@ -179,7 +196,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
   if (port->broken)
 port->broken--;
 
-  grub_outb (c, port->port + UART_TX);
+  ns8250_reg_write (port, c, UART_TX);
 }
 
 /* Initialize a serial device. PORT is the port number for a serial device.
@@ -262,6 +279,7 @@ grub_ns8250_init (void)
com_ports[i].name = com_names[i];
com_ports[i].driver = _ns8250_driver;
com_ports[i].port = serial_hw_io_addr[i];
+   com_ports[i].mmio = false;
err = grub_serial_config_defaults (_ports[i]);
if (err)
  grub_print_error ();
@@ -289,6 +307,7 @@ grub_serial_ns8250_add_port (grub_port_t port)
 {
   struct grub_serial_port *p;
   unsigned i;
+
   for (i = 

[PATCH v2 1/8] acpi: Export a generic grub_acpi_find_table

[Benjamin Herrenschmidt]

And convert grub_acpi_find_fadt to use it

Signed-off-by: Benjamin Herrenschmidt 
---
 grub-core/kern/acpi.c | 43 +--
 include/grub/acpi.h   |  3 +++
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/grub-core/kern/acpi.c b/grub-core/kern/acpi.c
index 70898ddcd..5d5b54b72 100644
--- a/grub-core/kern/acpi.c
+++ b/grub-core/kern/acpi.c
@@ -85,35 +85,42 @@ grub_acpi_xsdt_find_table (struct grub_acpi_table_header 
*xsdt, const char *sig)
   return 0;
 }
 
-struct grub_acpi_fadt *
-grub_acpi_find_fadt (void)
+void *
+grub_acpi_find_table (const char *sig)
 {
-  struct grub_acpi_fadt *fadt = 0;
+  struct grub_acpi_fadt *r = 0;
   struct grub_acpi_rsdp_v10 *rsdpv1;
   struct grub_acpi_rsdp_v20 *rsdpv2;
+
   rsdpv1 = grub_machine_acpi_get_rsdpv1 ();
   if (rsdpv1)
-fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv1->rsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-return fadt;
+r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+  (grub_addr_t) rsdpv1->rsdt_addr,
+  sig);
+  if (r)
+return r;
   rsdpv2 = grub_machine_acpi_get_rsdpv2 ();
   if (rsdpv2)
-fadt = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-return fadt;
+r = grub_acpi_rsdt_find_table ((struct grub_acpi_table_header *)
+  (grub_addr_t) rsdpv2->rsdpv1.rsdt_addr,
+  sig);
+  if (r)
+return r;
   if (rsdpv2
 #if GRUB_CPU_SIZEOF_VOID_P != 8
   && !(rsdpv2->xsdt_addr >> 32)
 #endif
   )
-fadt = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
- (grub_addr_t) rsdpv2->xsdt_addr,
- GRUB_ACPI_FADT_SIGNATURE);
-  if (fadt)
-return fadt;
+r = grub_acpi_xsdt_find_table ((struct grub_acpi_table_header *)
+  (grub_addr_t) rsdpv2->xsdt_addr,
+  sig);
+  if (r)
+return r;
   return 0;
 }
+
+struct grub_acpi_fadt *
+grub_acpi_find_fadt (void)
+{
+  return grub_acpi_find_table (GRUB_ACPI_FADT_SIGNATURE);
+}
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 84f49487d..8c126b2b9 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -244,4 +244,7 @@ enum
 struct grub_acpi_fadt *
 EXPORT_FUNC(grub_acpi_find_fadt) (void);
 
+void *
+EXPORT_FUNC(grub_acpi_find_table) (const char *sig);
+
 #endif /* ! GRUB_ACPI_HEADER */
-- 
2.34.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 7/8] ns8250: Support more MMIO access sizes

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt 

It is common for PCI based UARTs to use larger than one byte access
sizes. This adds support for this and uses the information present
in SPCR accordingly.

Signed-off-by: Benjamin Herrenschmidt 
---
 grub-core/term/ns8250-spcr.c |  3 +-
 grub-core/term/ns8250.c  | 67 ++--
 include/grub/serial.h| 10 --
 3 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 575622a4e..c8de90246 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -74,7 +74,8 @@ grub_ns8250_spcr_init (void)
   switch (spcr->base_addr.space_id)
 {
   case GRUB_ACPI_GENADDR_MEM_SPACE:
-return grub_serial_ns8250_add_mmio (spcr->base_addr.addr, );
+return grub_serial_ns8250_add_mmio (spcr->base_addr.addr,
+spcr->base_addr.access_size, 
);
   case GRUB_ACPI_GENADDR_IO_SPACE:
 return grub_serial_ns8250_add_port (spcr->base_addr.addr, );
   default:
diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
index d9d93fcf8..37a923b2d 100644
--- a/grub-core/term/ns8250.c
+++ b/grub-core/term/ns8250.c
@@ -42,16 +42,59 @@ ns8250_reg_read (struct grub_serial_port *port, grub_addr_t 
reg)
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+{
+  /*
+   * Note: we assume MMIO UARTs are little endian. This is not true of all
+   * embedded platforms but will do for now
+   */
+  switch(port->access_size)
+{
+default:
+  /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1" 
(byte) */
+case 1:
+  return *((volatile grub_uint8_t *) (port->mmio_base + reg));
+case 2:
+  return grub_le_to_cpu16(*((volatile grub_uint16_t *) 
(port->mmio_base + (reg << 1;
+case 3:
+  return grub_le_to_cpu32(*((volatile grub_uint32_t *) 
(port->mmio_base + (reg << 2;
+case 4:
+  /*
+   * This will only work properly on 64-bit systems since 64-bit
+   * accessors aren't atomic on 32-bit hardware. Thankfully the
+   * case of a UART with a 64-bit register spacing on 32-bit
+   * also probably doesn't exist.
+   */
+  return grub_le_to_cpu64(*((volatile grub_uint64_t *) 
(port->mmio_base + (reg << 3;
+}
+}
   return grub_inb (port->port + reg);
 }
 
 static void
-ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t, grub_addr_t reg)
+ns8250_reg_write (struct grub_serial_port *port, grub_uint8_t value, 
grub_addr_t reg)
 {
   asm volatile("" : : : "memory");
   if (port->mmio)
-*((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+{
+  switch(port->access_size)
+{
+default:
+  /* ACPI tables occasionally uses "0" (legacy) as equivalent to "1" 
(byte) */
+case 1:
+  *((volatile grub_uint8_t *) (port->mmio_base + reg)) = value;
+  break;
+case 2:
+  *((volatile grub_uint16_t *) (port->mmio_base + (reg << 1))) = 
grub_cpu_to_le16(value);
+  break;
+case 3:
+  *((volatile grub_uint32_t *) (port->mmio_base + (reg << 2))) = 
grub_cpu_to_le32(value);
+  break;
+case 4:
+  /* See commment in ns8250_reg_read() */
+  *((volatile grub_uint64_t *) (port->mmio_base + (reg << 3))) = 
grub_cpu_to_le64(value);
+  break;
+}
+}
   else
 grub_outb (value, port->port + reg);
 }
@@ -286,6 +329,7 @@ grub_ns8250_init (void)
  grub_print_error ();
 
grub_serial_register (_ports[i]);
+   com_ports[i].access_size = 1;
   }
 }
 
@@ -312,12 +356,12 @@ grub_serial_ns8250_add_port (grub_port_t port, struct 
grub_serial_config *config
   for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
 if (com_ports[i].port == port)
   {
-   if (dead_ports & (1 << i))
- return NULL;
-   /* give the opportunity for SPCR to configure a default com port */
-   if (config != NULL)
- grub_serial_port_configure (_ports[i], config);
-   return com_names[i];
+if (dead_ports & (1 << i))
+  return NULL;
+/* give the opportunity for SPCR to configure a default com port */
+if (config != NULL)
+  grub_serial_port_configure (_ports[i], config);
+return com_names[i];
   }
 
   grub_outb (0x5a, port + UART_SR);
@@ -340,6 +384,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct 
grub_serial_config *config
   p->driver = _ns8250_driver;
   p->mmio = false;
   p->port = port;
+  p->access_size = 1;
   if (config != NULL)
 grub_serial_port_configure (p, config);
   else
@@ -350,7 +395,8 @@ grub_serial_ns8250_add_port (grub_port_t port, struct 
grub_serial_config *config
 }
 
 char *
-grub_serial_ns8250_add_mmio (grub_addr_t 

[PATCH v2 2/8] acpi: Add SPCR and generic address definitions

[Benjamin Herrenschmidt]

This adds the definition of the two ACPI tables according to
the spec.

Signed-off-by: Benjamin Herrenschmidt 
---
 include/grub/acpi.h | 51 +
 1 file changed, 51 insertions(+)

diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 8c126b2b9..17aadb802 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -179,6 +179,57 @@ enum
 GRUB_ACPI_MADT_ENTRY_SAPIC_FLAGS_ENABLED = 1
   };
 
+struct grub_acpi_genaddr {
+  grub_uint8_t space_id;
+#define GRUB_ACPI_GENADDR_MEM_SPACE 0x00
+#define GRUB_ACPI_GENADDR_IO_SPACE  0x01
+  grub_uint8_t bit_width;
+  grub_uint8_t bit_offset;
+  grub_uint8_t access_size;
+#define GRUB_ACPI_GENADDR_SIZE_LGCY  0x00
+#define GRUB_ACPI_GENADDR_SIZE_BYTE  0x01
+#define GRUB_ACPI_GENADDR_SIZE_WORD  0x02
+#define GRUB_ACPI_GENADDR_SIZE_DWORD 0x03
+#define GRUB_ACPI_GENADDR_SIZE_QWORD 0x04
+  grub_uint64_t addr;
+} GRUB_PACKED;
+
+#define GRUB_ACPI_SPCR_SIGNATURE "SPCR"
+
+struct grub_acpi_spcr {
+  struct grub_acpi_table_header hdr;
+  grub_uint8_t intf_type;
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550  0x00
+#define GRUB_ACPI_SPCR_INTF_TYPE_16550X 0x01
+  grub_uint8_t reserved_0[3];
+  struct grub_acpi_genaddr base_addr;
+  grub_uint8_t interrupt_type;
+  grub_uint8_t irq;
+  grub_uint32_t gsi;
+  grub_uint8_t baud_rate;
+#define GRUB_ACPI_SPCR_BAUD_CURRENT  0x00
+#define GRUB_ACPI_SPCR_BAUD_9600 0x03
+#define GRUB_ACPI_SPCR_BAUD_192000x04
+#define GRUB_ACPI_SPCR_BAUD_576000x06
+#define GRUB_ACPI_SPCR_BAUD_115200   0x07
+  grub_uint8_t parity;
+  grub_uint8_t stop_bits;
+  grub_uint8_t flow_control;
+#define GRUB_ACPI_SPCR_FC_DCD_TX (1 << 0)
+#define GRUB_ACPI_SPCR_FC_RTSCTS (1 << 1)
+#define GRUB_ACPI_SPCR_FC_XONXOFF(1 << 2)
+  grub_uint8_t terminal_type;
+  grub_uint8_t language;
+  grub_uint16_tpci_device_id;
+  grub_uint16_t pci_vendor_id;
+  grub_uint8_t pci_bus;
+  grub_uint8_t pci_device;
+  grub_uint8_t pci_function;
+  grub_uint32_t pci_flags;
+  grub_uint8_t pci_segment;
+  grub_uint32_t reserved_1;
+} GRUB_PACKED;
+
 #ifndef GRUB_DSDT_TEST
 struct grub_acpi_rsdp_v10 *grub_acpi_get_rsdpv1 (void);
 struct grub_acpi_rsdp_v20 *grub_acpi_get_rsdpv2 (void);
-- 
2.34.1


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 6/8] ns8250: Use ACPI SPCR table when available to configure serial

[Benjamin Herrenschmidt]

From: Benjamin Herrenschmidt 

"serial auto" is now equivalent to just "serial" and will use the
SPCR to discover the port if present, otherwise defaults to "com0"
as before.

This allows to support MMIO ports specified by ACPI which is needed
on AWS EC2 "metal" instances, and will enable GRUB to pickup the
port configuration specified by ACPI in other cases.

Signed-off-by: Benjamin Herrenschmidt 
---
 docs/grub.texi   |  9 
 grub-core/Makefile.core.def  |  1 +
 grub-core/term/ns8250-spcr.c | 83 
 grub-core/term/serial.c  | 21 ++---
 include/grub/serial.h|  1 +
 5 files changed, 110 insertions(+), 5 deletions(-)
 create mode 100644 grub-core/term/ns8250-spcr.c

diff --git a/docs/grub.texi b/docs/grub.texi
index 50c811a88..fd22a69fa 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -2719,6 +2719,10 @@ you want to use COM2, you must specify @samp{--unit=1} 
instead. This
 command accepts many other options, so please refer to @ref{serial},
 for more details.
 
+Without argument or with @samp{--port=auto}, GRUB will attempt to use
+ACPI when available to auto-detect the default serial port and its
+configuration.
+
 The commands @command{terminal_input} (@pxref{terminal_input}) and
 @command{terminal_output} (@pxref{terminal_output}) choose which type of
 terminal you want to use. In the case above, the terminal will be a
@@ -4172,6 +4176,11 @@ be in the range 5-8 and stop bits must be 1 or 2. 
Default is 8 data
 bits and one stop bit. @var{parity} is one of @samp{no}, @samp{odd},
 @samp{even} and defaults to @samp{no}.
 
+If passed no @var{unit} nor @var{port}, or if @var{port} is set to
+@samp{auto} then GRUB will attempt to use ACPI to automatically detect
+the system default serial port and its configuration. If this information
+is not available, it will default to @var{unit} 0.
+
 The serial port is not used as a communication channel unless the
 @command{terminal_input} or @command{terminal_output} command is used
 (@pxref{terminal_input}, @pxref{terminal_output}).
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 95942fc8c..b656bdb44 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2048,6 +2048,7 @@ module = {
   name = serial;
   common = term/serial.c;
   x86 = term/ns8250.c;
+  x86 = term/ns8250-spcr.c;
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
new file mode 100644
index 0..575622a4e
--- /dev/null
+++ b/grub-core/term/ns8250-spcr.c
@@ -0,0 +1,83 @@
+/*
+ *  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 .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+char *
+grub_ns8250_spcr_init (void)
+{
+  struct grub_acpi_spcr *spcr;
+  struct grub_serial_config config;
+
+  spcr = grub_acpi_find_table (GRUB_ACPI_SPCR_SIGNATURE);
+  if (spcr == NULL)
+return NULL;
+  if (spcr->hdr.revision < 2)
+return NULL;
+  if (spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550 &&
+  spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550X)
+return NULL;
+  /* For now, we only support byte accesses */
+  if (spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_BYTE &&
+  spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_LGCY)
+return NULL;
+  config.word_len = 8;
+  config.parity = GRUB_SERIAL_PARITY_NONE;
+  config.stop_bits = GRUB_SERIAL_STOP_BITS_1;
+  config.base_clock = UART_DEFAULT_BASE_CLOCK;
+  if (spcr->flow_control & GRUB_ACPI_SPCR_FC_RTSCTS)
+config.rtscts = 1;
+  else
+config.rtscts = 0;
+  switch (spcr->baud_rate)
+{
+  case GRUB_ACPI_SPCR_BAUD_9600:
+config.speed = 9600;
+break;
+  case GRUB_ACPI_SPCR_BAUD_19200:
+config.speed = 19200;
+break;
+  case GRUB_ACPI_SPCR_BAUD_57600:
+config.speed = 57600;
+break;
+  case GRUB_ACPI_SPCR_BAUD_115200:
+config.speed = 115200;
+break;
+  case GRUB_ACPI_SPCR_BAUD_CURRENT:
+  default:
+   /*
+* We don't (yet) have a way to read the currently
+* configured speed in HW, so let's use a sane default
+*/
+config.speed = 115200;
+break;
+};
+  switch 

[PATCH v2 0/8] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

This series adds support for MMIO serial ports and auto-configuration
via ACPI SPCR.

This is necessary for the serial port to work on AWS EC2 "metal" x86
systems.

An MMIO serial port can be specified explicitely using the
"mmio," prefix for the --port argument to the 'serial' command,
the register access size can also be specified via a suffix,
and when 'serial' is used without arguments, it will try to use
an ACPI SPCR entry (if any) to add the default port and configure
it appropriately (speed, parity etc...)

This was tested using SPCR on an actual c5.metal instance, and using
explicit instanciation via serial -p mmio on a modified qemu
hacked to create MMIO PCI serial ports and to create a SPCR ACPI table.

The insipiration was an original implementation by
Matthias Lange ! matthias.lange at kernkonzept.com
However, the code was rewritten pretty much from scratch.

v2: This version (hopefully) addresses all the review commments and
has been rebased on the latest master branch.



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 15/15] docs: Add debugging chapter to development documentation

[Glenn Washburn]

On Wed, 21 Dec 2022 11:07:38 +0800
Jeremy Szu  wrote:

> On Fri, Dec 16, 2022 at 1:33 PM Glenn Washburn
>  wrote:
> >
> > Signed-off-by: Glenn Washburn 
> > ---
> >  docs/grub-dev.texi | 191
> > + 1 file changed, 191
> > insertions(+)
> >
> > diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> > index f76fc658bf..8171e91c33 100644
> > --- a/docs/grub-dev.texi
> > +++ b/docs/grub-dev.texi
> > @@ -79,6 +79,7 @@ This edition documents version @value{VERSION}.
> >  * Contributing Changes::
> >  * Setting up and running test suite::
> >  * Updating External Code::
> > +* Debugging::
> >  * Porting::
> >  * Error Handling::
> >  * Stack and heap size::
> > @@ -595,6 +596,196 @@ cp minilzo-2.10/*.[hc] grub-core/lib/minilzo
> >  rm -r minilzo-2.10*
> >  @end example
> >
> > +@node Debugging
> > +@chapter Debugging
> > +
> > +GRUB2 can be difficult to debug because it runs on the bare-metal
> > and thus +does not have the debugging facilities normally provided
> > by an operating +system. This chapter aims to provide useful
> > information on some ways to +debug GRUB2 for some architectures. It
> > by no means intends to be exhaustive. +The focus will be one X86_64
> > and i386 architectures. Luckily for some issues +virtual machines
> > have made the ability to debug GRUB2 much easier, and this +chapter
> > will focus debugging via the QEMU virtual machine. We will not be
> > +going over debugging of the userland tools (eg. grub-install),
> > there are +many tutorials on debugging programs in userland. +
> > +You will need GDB and the QEMU binaries for your system, on Debian
> > these +can be installed with the @samp{gdb} and
> > @samp{qemu-system-x86} packages. +Also it is assumed that you have
> > already successfully compiled GRUB2 from +source for the target
> > specified in the section below and have some +familiarity with GDB.
> > When GRUB2 is built it will create many different +binaries. The
> > ones of concern will be in the @file{grub-core} +directory of the
> > GRUB2 build dir. To aide in debugging we will want the +debugging
> > symbols generated during the build because these symbols are not
> > +kept in the binaries which get installed to the boot location. The
> > build +process outputs two sets of binaries, one without symbols
> > which gets executed +at boot, and another set of ELF images with
> > debugging symbols. The built +images with debugging symbols will
> > have a @file{.image} suffix, and the ones +without a @file{.img}
> > suffix. Similarly, loadable modules with debugging +symbols will
> > have a @file{.module} suffix, and ones without a @file{.mod}
> > +suffix. In the case of the kernel the binary with symbols is named
> > +@file{kernel.exec}. + +In the following sections, information will
> > be provided on debugging on +various targets using @command{gdb}
> > and the @samp{gdb_grub} GDB script. +
> > +@menu
> > +* i386-pc::
> > +* x86_64-efi::
> > +@end menu
> > +
> > +@node i386-pc
> > +@section i386-pc
> > +
> > +The i386-pc target is a good place to start when first debugging
> > GRUB2 +because in some respects its easier than EFI platforms. The
> > reason being +that the initial load address is always known in
> > advance. To start +debugging GRUB2 first QEMU must be started in
> > GDB stub mode. The following +command is a simple illustration:
> > +
> > +@example
> > +qemu-system-i386 -drive file=disk.img,format=raw \
> > +-device virtio-scsi-pci,id=scsi0,num_queues=4 -S -s
> > +@end example
> > +
> > +This will start a QEMU instance booting from @file{disk.img}. It
> > will pause +at start waiting for a GDB instance to attach to it.
> > You should change +@file{disk.img} to something more appropriate. A
> > block device can be used, +but you may need to run QEMU as a
> > privileged user. +
> > +To connect to this QEMU instance with GDB, the @code{target
> > remote} GDB +command must be used. We also need to load a binary
> > image, preferably with +symbols. This can be done using the GDB
> > command @code{file kernel.exec}, if +GDB is started from the
> > @file{grub-core} directory in the GRUB2 build +directory. GRUB2
> > developers have made this more simple by including a GDB +script
> > which does much of the setup. This file at
> > @file{grub-core/gdb_grub} +of the build directory and is also
> > installed via @command{make install}. +If not building GRUB, the
> > distribution may have a package which installs +this GDB script
> > along with debug symbol binaries, such as Debian's
> > +@samp{grub-pc-dbg} package. The GDB scripts is intended to by used
> > +like so, assuming: + +@example
> > +cd $(dirname /path/to/script/gdb_grub)
> > +gdb -x gdb_grub
> > +@end example
> > +
> > +Once GDB has been started with the @file{gdb_grub} script it will
> > +automatically connect to the QEMU instance. You can then do things
> > you +normally would in GDB like set a break point on
> > @var{grub_main}. +
> > +Setting breakpoints in 

Re: [PATCH v4 12/15] gdb: Allow running user-defined commands at GRUB start

[Glenn Washburn]

On Wed, 21 Dec 2022 12:19:16 -0600
Glenn Washburn  wrote:

> On Wed, 21 Dec 2022 16:27:40 +0100
> Daniel Kiper  wrote:
> 
> > On Thu, Dec 15, 2022 at 11:29:35PM -0600, Glenn Washburn wrote:
> > > A new command, run_on_start, is created which handles some
> > > complexities of the EFI platform when breaking on GRUB start. If
> > > GRUB start is hooked, run "onstart" command if it is defned.
> > >
> > > Signed-off-by: Glenn Washburn 
> > > ---
> > >  grub-core/gdb_grub.in | 38 ++
> > >  1 file changed, 38 insertions(+)
> > >
> > > diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> > > index 8ae6344edf..3b3cea1a4d 100644
> > > --- a/grub-core/gdb_grub.in
> > > +++ b/grub-core/gdb_grub.in
> > > @@ -36,6 +36,8 @@ end
> > >  define dynamic_load_symbols
> > >   dynamic_load_kernel_exec_symbols $arg0
> > >
> > > + run_on_start
> > > +
> > >   # We may have been very late to loading the kernel.exec
> > > symbols and # and modules may already be loaded. So load symbols
> > > for any already # loaded.
> > > @@ -134,6 +136,41 @@ document runtime_load_module
> > >   Load module symbols at runtime as they are loaded.
> > >  end
> > >
> > > +define run_on_start
> > > + # TODO: Add check to see if _start symbol is defined, if
> > > not, then
> > > + # the symbols have not yet been loaded and this command
> > > will not work.
> > > + watch *_start
> > > + set $break_efi_start_bpnum = $bpnum
> > > + commands
> > > + silent
> > > + delete $break_efi_start_bpnum
> > > + break _start
> > 
> > s/break/hbreak/?
> 
> A regular break works here for me. I want to avoid hbreak at all
> costs, which is why I had a previous convoluted method using break
> (which I thought worked, and then found it didn't quite). My
> understanding is that the number of hardware breakpoints are limited
> and commonly its around 4. Specifically, my understanding is that on
> x86-64 the number is exactly 4, so I would prefer the user have
> usable as many as possible.
> 
> Really, I'd like to figure out why sometimes break works and why
> sometimes not, and then figure out a way to make it work for these
> scripts. I recently had the idea that maybe the UEFI firmware sets up
> the pages where it loads the .text section of the GRUB UEFI binary to
> readonly in the page table structure. But I went through the structure
> when %eip is at _start and the R/W bit is set on the pages I checked.
> Even if the pages were set to readonly, I suspect the qemu gdb stub
> allows writing to that memory anyway.
> 
> So I'm at a loss as to what could be preventing break from working.
> I'd love to hear some ideas if anyone has some.

Okay so its been a while since I was deep in this and I forgot some
stuff I already knew (and which I should incorporate into the
documentation patch). The reason that software break doesn't always
work, is that when its not working its because the breakpoint is being
set before the GRUB EFI app is loaded into memory. So happens is that
GDB sets a breakpoint (ie a 0xcc instruction where the symbol points
to), but it then gets over-written by the UEFI firmware as its loads
the GRUB EFI app. So the app loading effectively will clear all
software breakpoints. This doesn't affect the hardware breakpoints
because they are in CPU debug registers which are not affected by the
firmware loading the EFI app.

The reason that the above worked, and was written that way in the first
place, is that when the watch commands run the memory at _start has just
been modified. This happens when the UEFI firmware is loading the GRUB
EFI app. Then we can use software break without worrying about it being
cleared.

Revisiting this has given me some ideas for improving the patch series.

Glenn

> 
> > > + commands
> > > + silent
> > > + delete $break_efi_start_bpnum
> > > + set $onstart_name = "onstart"
> > > + is_user_command $onstart_name
> > > + if $ret
> > > + onstart
> > > + end
> > > + continue
> > > + end
> > > + set $break_efi_start_bpnum = $bpnum
> > > + continue
> > > + end
> > > +end
> > > +document run_on_start
> > > + On some targets, such as x86_64-efi, even if you know
> > > where the
> > > + firmware will load the grub image, you can not simply set
> > > a break
> > 
> > Nit, s/grub/GRUB/...
> > 
> > > + point before the image is loaded because loading the
> > > image
> > > + overwrites the break point in memory. So setup a hardware
> > > watch
> > > + point, which does not have that problem, and if that gets
> > > triggered,
> > > + then reset the break point. If a user-defined command
> > > named
> > > + "onstart" exists it will be run after the start is hit.
> > > + NOTE: This assumes symbols have already been correctly
> > > loaded for
> > > + the EFI application.
> > > +end
> > > +
> > >  ###
> > >
> > >  set 

Re: [PATCH 7/7] serial: Add ability to specify MMIO ports via 'serial'

[Benjamin Herrenschmidt]

On Wed, 2022-12-21 at 15:11 +0100, Daniel Kiper wrote:
> > +  if (!port && grub_memcmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
> 
> I think grub_strncmp() in string context would be more appropriate.
> Please replace grub_memcmp() with grub_strncmp() where possible.

Same comment as patch 3, this is a pre-existing construct (look a few
lines above in the original code:

  if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
  && grub_isxdigit (name [sizeof ("port") - 1]))
{

I'll fix them all in the respun patch.

> > +  && grub_isxdigit (name [sizeof ("mmio,") - 1]))
> > +    {
> > +  const char *p1, *p = [sizeof ("mmio,") - 1];
> > +  grub_addr_t addr = grub_strtoul (p, , 16);
> 
> You blindly assume grub_strtoul() will not fail. It is not true.
> Please take a look at commit ac8a37dda (net/http: Allow use of
> non-standard TCP/IP ports) how properly detect grub_strtoul() failures.

Indeed, that's not great, I'll fix.

> > +  unsigned int acc_size = 1;
> > +  unsigned int nlen = p1 - p;
> 
> Please add empty line here.
> 
> > +  if (p1[0] == '.')
> > +    switch(p1[1])
> > + {
> > + case 'w':
> > +    acc_size = 2;
> > +    break;
> > + case 'l':
> > +    acc_size = 3;
> > +    break;
> > + case 'q':
> > +    acc_size = 4;
> > +    break;
> 
> Does not compiler complain there is no default here? I think I saw such
> warnings somewhere when it is missing.

I'm pretty sure it didn't but it might depend on a specific gcc 
version, I'll fix.

Cheers,
Ben.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] ns8250: Add base support for MMIO UARTs

[Benjamin Herrenschmidt]

Thanks for your review !

I'll  address everything. Small "nits":

On Wed, 2022-12-21 at 14:25 +0100, Daniel Kiper wrote:
> > +
> > +char *
> > +grub_serial_ns8250_add_mmio(grub_addr_t addr)
> > +{
> > +  struct grub_serial_port *p;
> > +  unsigned i;
> 
> Please add en empty line here.

Ack. I copied grub_serial_ns8250_add_port() :-) I'll fix that one as
well

> > +  for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
> > +    if (com_ports[i].mmio &&  com_ports[i].mmio_base == addr)
> > +   return com_names[i];
> > +
> > +  p = grub_malloc (sizeof (*p));
> > +  if (!p)
> 
> I prefer "p == NULL" instead of "!p". If you could fix that here and in
> the other places/patches that will be nice.

Ditto (pre-existing construct).

I'll fix them in the other code too, holler if you object.

Cheers,
Ben.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RESEND PATCH 0/7] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

On Wed, 2022-12-21 at 15:15 +0100, Daniel Kiper wrote:
> 
> First of all sorry for very late reply. I sent you my comments. There
> are some important problems plus some nitpicks which have to be
> fixed.
> Though I like the patch set in general. Than you for doing this work.

Thanks ! I'll try to address all of them asap.

Cheers,
Ben.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 13/15] gdb: Add extra early initialization symbols for i386-pc

[Glenn Washburn]

On Wed, 21 Dec 2022 16:28:35 +0100
Daniel Kiper  wrote:

> On Thu, Dec 15, 2022 at 11:29:36PM -0600, Glenn Washburn wrote:
> > Add symbols for boot.image, disk.image, and lzma_decompress.image
> > if the target is i386-pc.
> 
> Why?

Why not? I'm not clear on what your objection is. This is for debugging
the code that has those symbols, eg. to break on those symbols.

Glenn

> > Signed-off-by: Glenn Washburn 
> > ---
> >  grub-core/gdb_grub.in | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> > index 3b3cea1a4d..a9c9d00430 100644
> > --- a/grub-core/gdb_grub.in
> > +++ b/grub-core/gdb_grub.in
> > @@ -181,12 +181,18 @@ set confirm off
> >  # fail.
> >
> >  set $platform_efi = $_streq("@platform@", "efi")
> > +set $target = "@target_cpu@-@platform@"
> >
> >  if ! $runonce
> > if $platform_efi
> > # Only load the executable file, not the symbols
> > exec-file kernel.exec
> > else
> > +   if $_streq($target, "i386-pc")
> > +   add-symbol-file boot.image
> > +   add-symbol-file diskboot.image
> > +   add-symbol-file lzma_decompress.image
> > +   end
> > file kernel.exec
> > run_on_start
> > runtime_load_module
> 
> Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 12/15] gdb: Allow running user-defined commands at GRUB start

[Glenn Washburn]

On Wed, 21 Dec 2022 16:27:40 +0100
Daniel Kiper  wrote:

> On Thu, Dec 15, 2022 at 11:29:35PM -0600, Glenn Washburn wrote:
> > A new command, run_on_start, is created which handles some
> > complexities of the EFI platform when breaking on GRUB start. If
> > GRUB start is hooked, run "onstart" command if it is defned.
> >
> > Signed-off-by: Glenn Washburn 
> > ---
> >  grub-core/gdb_grub.in | 38 ++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> > index 8ae6344edf..3b3cea1a4d 100644
> > --- a/grub-core/gdb_grub.in
> > +++ b/grub-core/gdb_grub.in
> > @@ -36,6 +36,8 @@ end
> >  define dynamic_load_symbols
> > dynamic_load_kernel_exec_symbols $arg0
> >
> > +   run_on_start
> > +
> > # We may have been very late to loading the kernel.exec
> > symbols and # and modules may already be loaded. So load symbols
> > for any already # loaded.
> > @@ -134,6 +136,41 @@ document runtime_load_module
> > Load module symbols at runtime as they are loaded.
> >  end
> >
> > +define run_on_start
> > +   # TODO: Add check to see if _start symbol is defined, if
> > not, then
> > +   # the symbols have not yet been loaded and this command
> > will not work.
> > +   watch *_start
> > +   set $break_efi_start_bpnum = $bpnum
> > +   commands
> > +   silent
> > +   delete $break_efi_start_bpnum
> > +   break _start
> 
> s/break/hbreak/?

A regular break works here for me. I want to avoid hbreak at all costs,
which is why I had a previous convoluted method using break (which I
thought worked, and then found it didn't quite). My understanding is
that the number of hardware breakpoints are limited and commonly its
around 4. Specifically, my understanding is that on x86-64 the number
is exactly 4, so I would prefer the user have usable as many as
possible.

Really, I'd like to figure out why sometimes break works and why
sometimes not, and then figure out a way to make it work for these
scripts. I recently had the idea that maybe the UEFI firmware sets up
the pages where it loads the .text section of the GRUB UEFI binary to
readonly in the page table structure. But I went through the structure
when %eip is at _start and the R/W bit is set on the pages I checked.
Even if the pages were set to readonly, I suspect the qemu gdb stub
allows writing to that memory anyway.

So I'm at a loss as to what could be preventing break from working. I'd
love to hear some ideas if anyone has some.

Glenn

> > +   commands
> > +   silent
> > +   delete $break_efi_start_bpnum
> > +   set $onstart_name = "onstart"
> > +   is_user_command $onstart_name
> > +   if $ret
> > +   onstart
> > +   end
> > +   continue
> > +   end
> > +   set $break_efi_start_bpnum = $bpnum
> > +   continue
> > +   end
> > +end
> > +document run_on_start
> > +   On some targets, such as x86_64-efi, even if you know
> > where the
> > +   firmware will load the grub image, you can not simply set
> > a break
> 
> Nit, s/grub/GRUB/...
> 
> > +   point before the image is loaded because loading the image
> > +   overwrites the break point in memory. So setup a hardware
> > watch
> > +   point, which does not have that problem, and if that gets
> > triggered,
> > +   then reset the break point. If a user-defined command named
> > +   "onstart" exists it will be run after the start is hit.
> > +   NOTE: This assumes symbols have already been correctly
> > loaded for
> > +   the EFI application.
> > +end
> > +
> >  ###
> >
> >  set confirm off
> > @@ -151,6 +188,7 @@ if ! $runonce
> > exec-file kernel.exec
> > else
> > file kernel.exec
> > +   run_on_start
> > runtime_load_module
> > end
> 
> Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Robbie Harwood]

Zhang Boyang  writes:

> On 2022/12/21 06:58, Robbie Harwood wrote:
>> Zhang Boyang  writes:
>> 
>>> This patch add version information to GRUB modules. Specifically,
>>> PACKAGE_VERSION is embedded as null-terminated string in .modver
>>> section. This string is checked at module loading time. That module will
>>> be rejected if mismatch is found. This will prevent loading modules from
>>> different versions of GRUB.
>>>
>>> It should be pointed out that the version string is only consisted of
>>> PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
>>> noticed by version check (e.g. configure options).
>> 
>> Right now, this only affects non-secureboot scenarios (because we don't
>> have external signed module support).  I would want to see a resolution
>> to the external module signing question first so that we don't paint
>> ourselves into a corner with something like this.
>
> This feature is a prerequisite for external signed module support (and 
> it is mainly designed for this purpose). SBAT itself is not sufficient 
> to protect version mismatch attacks.

To be clear, I was not intending it as such.  That said, I dislike some
of your terminology: version mismatch is not generally an attack, merely
a self-own.  If you can write to and subsequently execute bootloader
files, you already have root to the box - there's no attack.  Secureboot
works by limiting what can be executed, and you simply can't have that
on Legacy BIOS.  So I think the order you're designing here is backward.

>> The combination of those two things leads me to suspect this is not the
>> right approach.  It seems likely that if we want to down the road of
>> versionlocking, something like the kernel's ephemeral key approach would
>> be better suited - and if we want external modules (which I don't think
>> we do), full SBAT support.
>
> Sorry but I'm not familiar with the kernel's ephemeral key approach. 
> Could you please give me some links to this?

https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/module-signing.rst

Be well,
--Robbie


signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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  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/msg8.html

> 
> > Co-developed-by: Peter Jones 
> > Signed-off-by: Glenn Washburn 
> > ---
> >  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
> > 

Re: [RFC PATCH] kern/dl: Add module version check

[Glenn Washburn]

On Wed, 21 Dec 2022 16:07:27 +0800
Zhang Boyang  wrote:

> Hi,
> 
> On 2022/12/21 03:04, Glenn Washburn wrote:
> > On Mon, 19 Dec 2022 23:33:29 +0800
> > Zhang Boyang  wrote:
> > 
> >> This patch add version information to GRUB modules. Specifically,
> >> PACKAGE_VERSION is embedded as null-terminated string in .modver
> >> section. This string is checked at module loading time. That module
> >> will be rejected if mismatch is found. This will prevent loading
> >> modules from different versions of GRUB.
> >>
> >> It should be pointed out that the version string is only consisted
> >> of PACKAGE_VERSION. Any difference not reflected in
> >> PACKAGE_VERSION is not noticed by version check (e.g. configure
> >> options).
> > 
> > Is this solving a real world problem? I've been loading modules from
> > other GRUB versions for years and have yet to notice an issue.
> > Though, I do see where issues could occur.
> > 
> 
> The main purpose is to implement truly loadable modules in secure
> boot environment (please see my reply to Robbie for details, 
> https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
> ). But I did encountered strange crashes with mismatched modules,
> when using stock debian grub modules with latest git grub, on both
> UEFI and BIOS. If you are interested, here are reproducible steps:
> 
> 1) Please install a fresh Debian (without GUI) into a virtual machine 
> from debian-11.6.0-amd64-netinst.iso (Secure boot need to be disabled
> if using UEFI)
> 
> 2) Build latest GRUB from git using these commands
> sudo apt update
> sudo apt install build-essential git
> sudo apt build-dep grub2
> git clone https://git.savannah.gnu.org/git/grub.git
> cd grub
> ./bootstrap
> ./configure --prefix=/usr/local --with-platform=efi  # or 
> --with-platform=pc if using BIOS
> make -j2
> sudo make install
> 
> 3) Backup stock debian GRUB modules
> sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.debian  # replace 
> x86_64-efi with i386-pc if using BIOS, the same below
> 
> 4) Install our build
> sudo /usr/local/sbin/grub-install  # add /dev/sda or similar if using
> BIOS
> 
> 5) Replace modules with stock modules
> sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.mybuild
> sudo mv /boot/grub/x86_64-efi.debian /boot/grub/x86_64-efi
> 
> 6) Reboot. GRUB should crash with messages like this:
> Loading Linux 5.10.0-20-amd64 ...
> 452: out of range pointer: 0x3ff8da10
> 
> Aborted. Press any key to exit.
> 
> 
> I ran `git bisect` and it reports this problem is introduced by 
> 052e6068be62 ("mm: When adding a region, merge with region after as
> well as before"). This commit changed the layout of `struct
> grub_mm_region`, which is both used in main program and
> "relocator.mod", so the module will read the wrong field and crashes
> GRUB.

Yeah, I'm not surprised due to these recent changes. For the last
decade GRUB hasn't had such major changes to the core. So maybe this is
a good time to add a patch like this.

> > There are two use cases that I understand this patch to potentially
> > break, which I think should continue to be supported. The first use
> > case is using isos which support the semi-standard loopback.cfg[1].
> > The vast majority, if not all, of the uses of the loopback.cfg for
> > loopback mounted isos (eg. autoiso.cfg and isodetect.cfg) will
> > create menu entries where $root is set to the loopbacked iso file
> > and then the loopback.cfg is run. This patch could cause these
> > created menu entries to be broken if the modules needed for the
> > commands in the loopback.cfg have not been loaded ahead of time.
> > This is because the $root is now pointing to the iso device and,
> > assuming $prefix has no device component (usually the case), then
> > modules will be attempted to be loaded from the iso, not from the
> > running grub install.
> > 
> > The other use case, which I use to boot my system, is having one
> > GRUB install load the grub.cfg of another GRUB install. Similar to
> > the first use case, the issue is that $root changed from the device
> > of the running GRUB to the device where the grub.cfg to be loaded
> > is located. So module loading, again, will attempted for modules
> > that are not for the running GRUB.
> > 
> 
> There seems no API/ABI compatibility guarantees in GRUB. So I don't 
> think it is a good idea to mix different versions of main program and 
> modules, and it should be consider unsupported (although it works in 
> most situations). It is possible to avoid this by preloading modules 
> before changing $root and/or $prefix (insmod will be no-op if given 
> module name is already exists).

Yep, I agree about the support. I do think behavior that has been
working, even unofficially, for many years, should be have weight to it
when considering changes that will break that behavior.

> > I'm not opposed in principle to adding a module version check, if
> > these issues can be resolved/mitigated. At a minimum it should be
> > easy to disable at 

Re: [PATCH v2 1/2] mm: Adjust new region size to take management overhead into account

[Zhang Boyang]

Hi,

On 2022/12/21 01:16, Daniel Kiper wrote:

Please do not send your new sets of patches as a reply to previous
discussions. If you do that then it is more difficult to find them
in threaded views.



OK. I will not do that in future.


On Thu, Dec 15, 2022 at 07:19:23PM +0800, Zhang Boyang wrote:

When grub_memalign() encounters out-of-memory, it will try
grub_mm_add_region_fn() to request more memory from system firmware.
However, the size passed to it doesn't take region management overhead
into account. Adding a memory area of "size" bytes will result in a heap
region of less than "size" bytes truely avaliable. Thus, the new region
may not be adequate for current allocation request, confusing
out-of-memory handling code.

This patch introduces GRUB_MM_MGMT_OVERHEAD to address the region
management overhead (e.g. metadata, padding). The value of this new
constant should make grub_malloc(size) always success after a successful
call to grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD).

The new region size is now set to "size + GRUB_MM_MGMT_OVERHEAD", thus
if grub_mm_add_region_fn() succeeded, current allocation request can
always success.

Signed-off-by: Zhang Boyang 
---
  grub-core/kern/mm.c | 37 ++---
  1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index ae2279133..20bb54dde 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -232,6 +232,7 @@ grub_mm_init_region (void *addr, grub_size_t size)
addr, size);
/* Allocate a region from the head.  */
r = (grub_mm_region_t) ALIGN_UP ((grub_addr_t) addr, GRUB_MM_ALIGN);
+#define GRUB_MM_MGMT_OVERHEAD_HEAD_PADDING (GRUB_MM_ALIGN - 1)

/* If this region is too small, ignore it.  */
if (size < GRUB_MM_ALIGN + (char *) r - (char *) addr + sizeof (*r))
@@ -239,15 +240,18 @@ grub_mm_init_region (void *addr, grub_size_t size)

size -= (char *) r - (char *) addr + sizeof (*r);

+#define GRUB_MM_MGMT_OVERHEAD_MM_HEADERsizeof (struct 
grub_mm_header)
h = (grub_mm_header_t) (r + 1);
h->next = h;
h->magic = GRUB_MM_FREE_MAGIC;
h->size = (size >> GRUB_MM_ALIGN_LOG2);

+#define GRUB_MM_MGMT_OVERHEAD_REGION_HEADERsizeof (struct grub_mm_region)
r->first = h;
r->pre_size = (grub_addr_t) r - (grub_addr_t) addr;
r->size = (h->size << GRUB_MM_ALIGN_LOG2);
r->post_size = size - r->size;
+#define GRUB_MM_MGMT_OVERHEAD_TAIL_PADDING (GRUB_MM_ALIGN - 1)


Please do not sprinkle constants definitions over the code. This does
not help in reading. Additionally, I think these extra constants
make code less readable.



My motivation is to force who want to modify this code notice there is a 
constant tightly connected to region layout. So he will not forget 
update the constant if he changes region layout.


I agree this will reduce readability. I will replace this with a comment 
in next version.



/* Find where to insert this region. Put a smaller one before bigger ones,
   to prevent fragmentation.  */
@@ -259,6 +263,17 @@ grub_mm_init_region (void *addr, grub_size_t size)
r->next = q;
  }

+/*
+ * GRUB_MM_MGMT_OVERHEAD is an upper bound of management overhead of
+ * each region, with any possible padding taken into account.
+ * The value should make grub_malloc(size) always success after a successful
+ * call to grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD).
+ */
+#define GRUB_MM_MGMT_OVERHEAD (GRUB_MM_MGMT_OVERHEAD_HEAD_PADDING + \
+  GRUB_MM_MGMT_OVERHEAD_REGION_HEADER + \
+  GRUB_MM_MGMT_OVERHEAD_MM_HEADER + \
+  GRUB_MM_MGMT_OVERHEAD_TAIL_PADDING)


I do not think this math is correct. Please use mine from previous
reply. When it is wrong please say where exactly it is wrong. OK, I can
agree using "sizeof (struct grub_mm_region)" is more readable than
"sizeof (*grub_mm_region_t)". Anything else?



I have to use "sizeof (struct grub_mm_region)", because "sizeof 
(*grub_mm_region_t)" won't compile.


After rethinking, I think the formula need to be refactored. The purpose 
of this constant, same as what's its comment says, is to make 
grub_malloc(size) always success after a successful call to 
grub_mm_init_region(addr, size + GRUB_MM_MGMT_OVERHEAD).


If new region can be merged into an existing region, all of its area can 
be used for forming a free block (grub_mm_header and user usable bytes). 
This is the most simple case. Even in this case, GRUB_MM_MGMT_OVERHEAD 
should be "sizeof(struct grub_mm_header) + (GRUB_MM_ALIGN-1)". This is 
because "size" may not be integral multiple of GRUB_MM_ALIGN. As 
GRUB_MM_MGMT_OVERHEAD is a constant designed to be added to "size", we 
can't use macros like ALIGN_UP. Therefore, we must make 
GRUB_MM_MGMT_OVERHEAD sufficient large, so "x + GRUB_MM_MGMT_OVERHEAD >= 
sizeof(struct grub_mm_header) + ALIGN_UP(x, GRUB_MM_ALIGN)" is always 

Re: [PATCH v2 3/3] pci: Rename GRUB_PCI_CLASS_*

[Daniel Kiper]

On Fri, Aug 26, 2022 at 01:01:45PM +0200, pet...@infradead.org wrote:
> Glenn suggested to rename the existing PCI_CLASS defines to have
> explicit class and subclass names.
>
> Suggested-by: Glenn Washburn 
> Signed-off-by: Peter Zijlstra (Intel) 

Reviewed-by: Daniel Kiper 

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 15/15] docs: Add debugging chapter to development documentation

[Daniel Kiper]

On Thu, Dec 15, 2022 at 11:29:38PM -0600, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn 
> ---
>  docs/grub-dev.texi | 191 +
>  1 file changed, 191 insertions(+)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index f76fc658bf..8171e91c33 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -79,6 +79,7 @@ This edition documents version @value{VERSION}.
>  * Contributing Changes::
>  * Setting up and running test suite::
>  * Updating External Code::
> +* Debugging::
>  * Porting::
>  * Error Handling::
>  * Stack and heap size::
> @@ -595,6 +596,196 @@ cp minilzo-2.10/*.[hc] grub-core/lib/minilzo
>  rm -r minilzo-2.10*
>  @end example
>
> +@node Debugging
> +@chapter Debugging
> +
> +GRUB2 can be difficult to debug because it runs on the bare-metal and thus
> +does not have the debugging facilities normally provided by an operating
> +system. This chapter aims to provide useful information on some ways to
> +debug GRUB2 for some architectures. It by no means intends to be exhaustive.
> +The focus will be one X86_64 and i386 architectures. Luckily for some issues

s/X86_64/x86_64/

> +virtual machines have made the ability to debug GRUB2 much easier, and this
> +chapter will focus debugging via the QEMU virtual machine. We will not be
> +going over debugging of the userland tools (eg. grub-install), there are
> +many tutorials on debugging programs in userland.
> +
> +You will need GDB and the QEMU binaries for your system, on Debian these
> +can be installed with the @samp{gdb} and @samp{qemu-system-x86} packages.
> +Also it is assumed that you have already successfully compiled GRUB2 from
> +source for the target specified in the section below and have some
> +familiarity with GDB. When GRUB2 is built it will create many different
> +binaries. The ones of concern will be in the @file{grub-core}
> +directory of the GRUB2 build dir. To aide in debugging we will want the
> +debugging symbols generated during the build because these symbols are not
> +kept in the binaries which get installed to the boot location. The build
> +process outputs two sets of binaries, one without symbols which gets executed
> +at boot, and another set of ELF images with debugging symbols. The built
> +images with debugging symbols will have a @file{.image} suffix, and the ones
> +without a @file{.img} suffix. Similarly, loadable modules with debugging
> +symbols will have a @file{.module} suffix, and ones without a @file{.mod}
> +suffix. In the case of the kernel the binary with symbols is named
> +@file{kernel.exec}.
> +
> +In the following sections, information will be provided on debugging on
> +various targets using @command{gdb} and the @samp{gdb_grub} GDB script.
> +
> +@menu
> +* i386-pc::
> +* x86_64-efi::
> +@end menu
> +
> +@node i386-pc
> +@section i386-pc
> +
> +The i386-pc target is a good place to start when first debugging GRUB2
> +because in some respects its easier than EFI platforms. The reason being
> +that the initial load address is always known in advance. To start
> +debugging GRUB2 first QEMU must be started in GDB stub mode. The following
> +command is a simple illustration:
> +
> +@example
> +qemu-system-i386 -drive file=disk.img,format=raw \
> +-device virtio-scsi-pci,id=scsi0,num_queues=4 -S -s

I would drop "num_queues=4" as non-essential thing here. And num_queues
seems deprecated today...

> +@end example
> +
> +This will start a QEMU instance booting from @file{disk.img}. It will pause
> +at start waiting for a GDB instance to attach to it. You should change
> +@file{disk.img} to something more appropriate. A block device can be used,
> +but you may need to run QEMU as a privileged user.
> +
> +To connect to this QEMU instance with GDB, the @code{target remote} GDB
> +command must be used. We also need to load a binary image, preferably with
> +symbols. This can be done using the GDB command @code{file kernel.exec}, if
> +GDB is started from the @file{grub-core} directory in the GRUB2 build
> +directory. GRUB2 developers have made this more simple by including a GDB
> +script which does much of the setup. This file at @file{grub-core/gdb_grub}
> +of the build directory and is also installed via @command{make install}.
> +If not building GRUB, the distribution may have a package which installs
> +this GDB script along with debug symbol binaries, such as Debian's
> +@samp{grub-pc-dbg} package. The GDB scripts is intended to by used
> +like so, assuming:
> +
> +@example
> +cd $(dirname /path/to/script/gdb_grub)
> +gdb -x gdb_grub
> +@end example
> +
> +Once GDB has been started with the @file{gdb_grub} script it will
> +automatically connect to the QEMU instance. You can then do things you
> +normally would in GDB like set a break point on @var{grub_main}.
> +
> +Setting breakpoints in modules is trickier since they haven't been loaded
> +yet and are loaded at addresses determined at runtime. The module could be
> +loaded 

Re: [RFC PATCH v2 1/1] kern/dl: Add module version check

[Pete Batard via Grub-devel]

Hi again Zhang,

I hadn't had a chance to properly look at your v2 before replying 
earlier, and it looks like it addresses the elements I had an issue with.


On 2022.12.21 12:11, Zhang Boyang wrote:

This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time.

If GRUB is locked down, modules with mismatched version will be
rejected. This is necessary for implementing external signed modules
securely (in future).

For compatibility reasons, if GRUB isn't locked down, modules can still
be loaded even if they have mismatched version.


This alteration to your original patch looks like a good compromise to 
both our issues. Thanks for this!


I had a worry that lockdown was something that could be enabled for both 
BIOS and UEFI, but looking at the current lockdown.h, grub_is_lockdown() 
always returns GRUB_LOCKDOWN_DISABLED for BIOS, so this looks like a 
good solution to me.



A warning message will be emited in that situation.


For BIOS boot, please don't.

If someone explicitly could not enable lockdown, they are unlikely to 
want lockdown-related warnings to be produced, especially for BIOS where 
lockdown cannot apply anyway.


So I think we want to add a '#ifdef GRUB_MACHINE_EFI' for that warning 
(see below). Is that something that you agree with?


If you submit a v3 with the changes I propose below, then I think I will 
have no objection with this proposal.



It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).

Link: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
Link: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html
Signed-off-by: Zhang Boyang 
---
  grub-core/Makefile.am |  2 +-
  grub-core/genmod.sh.in| 28 +---
  grub-core/kern/dl.c   | 33 +
  util/grub-module-verifierXX.c | 10 ++
  4 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 80e7a83ed..d76aa80f4 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
  CLEANFILES += gentrigtables$(BUILD_EXEEXT)
  
  build-grub-module-verifier$(BUILD_EXEEXT): $(top_srcdir)/util/grub-module-verifier.c $(top_srcdir)/util/grub-module-verifier32.c $(top_srcdir)/util/grub-module-verifier64.c $(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c

-   $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) $(BUILD_CPPFLAGS) 
$(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
+   $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) $(BUILD_CPPFLAGS) 
$(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" 
-DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^
  CLEANFILES += build-grub-module-verifier$(BUILD_EXEEXT)
  
  # trigtables.c

diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index e57c4d920..58a4cdcbe 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
  rm -f $tmpfile $outfile
  
  if test x@TARGET_APPLE_LINKER@ != x1; then

-# stripout .modname and .moddeps sections from input module
-@TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
+# stripout .modname and .moddeps and .modver sections from input module
+@TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile $tmpfile
  
-# Attach .modname and .moddeps sections

+# Attach .modname and .moddeps and .modver sections
  t1=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
  printf "$modname\0" >$t1
  
  t2=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1

  for dep in $deps; do printf "$dep\0" >> $t2; done
  
+t3=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1

+printf "@PACKAGE_VERSION@\0" >$t3
+
  if test -n "$deps"; then
-   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 
$tmpfile
+   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 
--add-section .modver=$t3 $tmpfile
  else
-   @TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
+   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .modver=$t3 
$tmpfile
  fi
-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
  
  	if test x@platform@ != xemu; then

@TARGET_STRIP@ --strip-unneeded \
@@ -71,23 +74,26 @@ else
  tmpfile2=${outfile}.tmp2
  t1=${outfile}.t1.c
  t2=${outfile}.t2.c
+t3=${outfile}.t3.c
  
  # remove old files if any

-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
  
  cp $infile $tmpfile
  
-# Attach .modname and .moddeps sections

Re: [PATCH v4 13/15] gdb: Add extra early initialization symbols for i386-pc

[Daniel Kiper]

On Thu, Dec 15, 2022 at 11:29:36PM -0600, Glenn Washburn wrote:
> Add symbols for boot.image, disk.image, and lzma_decompress.image if the
> target is i386-pc.

Why?

> Signed-off-by: Glenn Washburn 
> ---
>  grub-core/gdb_grub.in | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> index 3b3cea1a4d..a9c9d00430 100644
> --- a/grub-core/gdb_grub.in
> +++ b/grub-core/gdb_grub.in
> @@ -181,12 +181,18 @@ set confirm off
>  # fail.
>
>  set $platform_efi = $_streq("@platform@", "efi")
> +set $target = "@target_cpu@-@platform@"
>
>  if ! $runonce
>   if $platform_efi
>   # Only load the executable file, not the symbols
>   exec-file kernel.exec
>   else
> + if $_streq($target, "i386-pc")
> + add-symbol-file boot.image
> + add-symbol-file diskboot.image
> + add-symbol-file lzma_decompress.image
> + end
>   file kernel.exec
>   run_on_start
>   runtime_load_module

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 12/15] gdb: Allow running user-defined commands at GRUB start

[Daniel Kiper]

On Thu, Dec 15, 2022 at 11:29:35PM -0600, Glenn Washburn wrote:
> A new command, run_on_start, is created which handles some complexities
> of the EFI platform when breaking on GRUB start. If GRUB start is hooked,
> run "onstart" command if it is defned.
>
> Signed-off-by: Glenn Washburn 
> ---
>  grub-core/gdb_grub.in | 38 ++
>  1 file changed, 38 insertions(+)
>
> diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> index 8ae6344edf..3b3cea1a4d 100644
> --- a/grub-core/gdb_grub.in
> +++ b/grub-core/gdb_grub.in
> @@ -36,6 +36,8 @@ end
>  define dynamic_load_symbols
>   dynamic_load_kernel_exec_symbols $arg0
>
> + run_on_start
> +
>   # We may have been very late to loading the kernel.exec symbols and
>   # and modules may already be loaded. So load symbols for any already
>   # loaded.
> @@ -134,6 +136,41 @@ document runtime_load_module
>   Load module symbols at runtime as they are loaded.
>  end
>
> +define run_on_start
> + # TODO: Add check to see if _start symbol is defined, if not, then
> + # the symbols have not yet been loaded and this command will not work.
> + watch *_start
> + set $break_efi_start_bpnum = $bpnum
> + commands
> + silent
> + delete $break_efi_start_bpnum
> + break _start

s/break/hbreak/?

> + commands
> + silent
> + delete $break_efi_start_bpnum
> + set $onstart_name = "onstart"
> + is_user_command $onstart_name
> + if $ret
> + onstart
> + end
> + continue
> + end
> + set $break_efi_start_bpnum = $bpnum
> + continue
> + end
> +end
> +document run_on_start
> + On some targets, such as x86_64-efi, even if you know where the
> + firmware will load the grub image, you can not simply set a break

Nit, s/grub/GRUB/...

> + point before the image is loaded because loading the image
> + overwrites the break point in memory. So setup a hardware watch
> + point, which does not have that problem, and if that gets triggered,
> + then reset the break point. If a user-defined command named
> + "onstart" exists it will be run after the start is hit.
> + NOTE: This assumes symbols have already been correctly loaded for
> + the EFI application.
> +end
> +
>  ###
>
>  set confirm off
> @@ -151,6 +188,7 @@ if ! $runonce
>   exec-file kernel.exec
>   else
>   file kernel.exec
> + run_on_start
>   runtime_load_module
>   end

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Daniel Kiper]

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...

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.

> Co-developed-by: Peter Jones 
> Signed-off-by: Glenn Washburn 
> ---
>  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

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 04/15] gdb: Move runtime module loading into runtime_load_module

[Daniel Kiper]

On Thu, Dec 15, 2022 at 11:29:27PM -0600, Glenn Washburn wrote:

Why? Could you add that to the commit message?

> Signed-off-by: Glenn Washburn 
> ---
>  grub-core/gdb_grub.in | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
> index fc17e3d899..d525a5a11f 100644
> --- a/grub-core/gdb_grub.in
> +++ b/grub-core/gdb_grub.in
> @@ -71,16 +71,22 @@ document load_all_modules
>   Load debugging information for all loaded modules.
>  end
>
> +define runtime_load_module
> + break grub_dl_add
> + commands
> + silent
> + load_module mod
> + cont
> + end
> +end
> +document runtime_load_module
> + Load module symbols at runtime as they are loaded.
> +end
> +
>  ###
>
>  set confirm off
>  file kernel.exec
>  target remote :1234
>
> -# inform when module is loaded
> -break grub_dl_add
> -commands
> - silent
> - load_module mod
> - cont
> -end
> +runtime_load_module

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Pete Batard via Grub-devel]

Hi Zhang,

On 2022.12.21 11:38, Zhang Boyang wrote:

Hi,

On 2022/12/21 17:43, Thomas Schmitt wrote:

Hi,

Pete Batard wrote:

unlike what is the case for UEFI, one can not expect
to be able to pick all the GRUB core files needed to convert a GRUB
based ISO bootable media to a GRUB based USB bootable media [...]
Typically, one of the
missing files will be a 'core.img' that can work for USB BIOS boot of a
MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
image will not readily provide.


Zhang Boyang wrote:

The situation is, for BIOS boot, there is no grub core file in ISO
because it's written to sectors instead of files? Did I understand
correctly?


I think it is rather about the fact that GRUB equipped hybrid ISOs boot
on legacy BIOS from USB stick via the MBR code (~440 byte) to the
El Torito boot image, which is in charge for booting from optical media.
In grub-mkrescue ISOs it is called
   /boot/grub/i386-pc/eltorito.img
The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].

So no core.img is needed and the filesystem module set can be sparse,
because the filesystem type is known from which GRUB will read its 
further

software.




Thanks for explaining! So I think the best solution for Rufus is to 
overwrite distro's .mod files with its owns (if mismatched modules is a 
problem).


I'm sorry but that is not an option because, whereas when distro 
maintainers apply patches to GRUB to be able to boot their distribution, 
core.img is usually "safe" from major alterations, meaning that we are 
able to use a core.img that wasn't specifically compiled alongside the 
modules, the modules are usually the elements that are modified.


So, we can NOT rely on vanilla GRUB modules working at all for booting a 
distro that has had GRUB patches applied. Unlike what is the case for 
core.img, where we very seldom observe breakage, and as you actually 
observed below, this will break boot in a lot of cases.


Plus, since we cannot embed GRUB modules in a 1 MB application that we 
purposefully designed to have a small download footprint, you would now 
be forcing users to download ~10 MB of data, which introduces other 
issues (it's already slightly problematic to have users download a 
core.img since we only embed the one from the latest GRUB release in Rufus).


I do understand that what I am reporting is problematic for the solution 
you are proposing, and that you would like to be able to brush it away 
with a simple "Just do it this other way then", but I can assure you, it 
is not that simple, and, to avoid users running into issues, core.img 
replacement is the much better solution.



Pete Batard wrote:

Rufus usually recommends to write such images in what it calls
"ISO mode" (shorthand for "ISO extraction mode") with the ISO content
extracted to a FAT or NTFS file system and with a GRUB core.img
bootloader


Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
equipment that is needed to make bootable a USB stick with FAT or NTFS.
(The filesystem modules are included in Pete Batard's proposal for the
  /EFI/BOOT tree. But the disk MBR image and core.img are not.)


Yeah, that was really the only other *remotely viable* alternative I 
considered at the time.


This however requires a few factors:
- ISOHybrid to be used (not guaranteed. One great plus of Rufus is that, 
in most case, it can make bootable media of non ISOHybrids, including 
GRUB based ones)
- grub-mkrescue to be used to generate the ISOHybrid (The maintainers 
may choose to use a different method)
- Distro maintainers to be cooperative with the idea of supporting an 
alternate mode of creation of bootable USB media compared to DD (which, 
sadly, is not a given).



Zhang Boyang wrote:

test whether that commit (052e6068be62) breaks Rufus
(e.g. Rufus with latest git GRUB, to boot Debian ISO).


Debian ISOs still boot on legacy BIOS via ISOLINUX.
Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs 
for

both, legacy BIOS and EFI.



Oh, yes, Debian seems using isolinux instead of grub So please use 
Ubuntu ISOs for testing, which uses grub in BIOS mode. By the way, I 
also tried Arch Linux, it also uses isolinux (but it has a theme looks 
like grub).


I did reversed tests (i.e. old GRUB core + new GRUB modules) just now:

1) I used Rufus 3.21 and ubuntu-22.04.1-desktop-amd64.iso (which uses 
GRUB 2.06) to create a USB stick. To simulate mismatched modules, I 
overwrited "\boot\grub\i386-pc\relocator.mod" (on USB stick) with my 
latest git build. Then that USB stick failed to boot in BIOS mode, 
showing "452: out of range pointer: 0x100010" error.


2) I tested supergrub2-2.06s1-beta2-multiarch-CD.iso with latest Arch 
Linux (BIOS mode, grub 2:2.06.r403.g7259d55ff-1). It doesn't have 
problems booting Arch Linux because it has $prefix properly set 

Re: [RESEND PATCH 0/7] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:19AM +1100, Benjamin Herrenschmidt wrote:
> This series adds support for MMIO serial ports and auto-configuration
> via ACPI SPCR.
>
> This is necessary for the serial port to work on AWS EC2 "metal" x86
> systems.
>
> An MMIO serial port can be specified explicitely using the
> "mmio," prefix for the --port argument to the 'serial' command,
> the register access size can also be specified via a suffix,
> and when 'serial' is used without arguments, it will try to use
> an ACPI SPCR entry (if any) to add the default port and configure
> it appropriately (speed, parity etc...)
>
> This was tested using SPCR on an actual c5.metal instance, and using
> explicit instanciation via serial -p mmio on a modified qemu
> hacked to create MMIO PCI serial ports.

First of all sorry for very late reply. I sent you my comments. There
are some important problems plus some nitpicks which have to be fixed.
Though I like the patch set in general. Than you for doing this work.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] serial: Add ability to specify MMIO ports via 'serial'

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:26AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt 
>
> This adds the ability to explicitely add an MMIO based serial port
> via the 'serial' command. The syntax is:
>
> serial --port=mmio,{.b,.w,.l,.q}
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
>  docs/grub.texi  | 26 --
>  grub-core/term/serial.c | 31 ++-
>  2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index bbebc2aef..a25235636 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4167,8 +4167,24 @@ Commands usable anywhere in the menu and in the 
> command-line.
>  @deffn Command serial [@option{--unit=unit}] [@option{--port=port}] 
> [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] 
> [@option{--stop=stop}]
>  Initialize a serial device. @var{unit} is a number in the range 0-3
>  specifying which serial port to use; default is 0, which corresponds to
> -the port often called COM1. @var{port} is the I/O port where the UART
> -is to be found; if specified it takes precedence over @var{unit}.
> +the port often called COM1.
> +
> +@var{port} is the I/O port where the UART is to be found or, if prefixed
> +with @samp{mmio,}, the MMIO address of the UART. If specified it takes
> +precedence over @var{unit}.
> +
> +Additionally, an MMIO address can be suffixed with:
> +@itemize @bullet
> +@item
> +@samp{.b} for bytes access (default)
> +@item
> +@samp{.w} for 16-bit word access
> +@item
> +@samp{.l} for 32-bit long word access or
> +@item
> +@samp{.q} for 64-bit long long word access
> +@end itemize
> +
>  @var{speed} is the transmission speed; default is 9600. @var{word} and
>  @var{stop} are the number of data bits and stop bits. Data bits must
>  be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
> @@ -4184,6 +4200,12 @@ The serial port is not used as a communication channel 
> unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
>
> +Examples:
> +@example
> +serial --port=3f8 --speed=9600
> +serial --port=mmio,fefb.l --speed=115200
> +@end example
> +
>  See also @ref{Serial terminal}.
>  @end deffn
>
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index f57fe45ef..96a5ad725 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -164,6 +164,35 @@ grub_serial_find (const char *name)
>   if (grub_strcmp (port->name, name) == 0)
> break;
>  }
> +  if (!port && grub_memcmp (name, "mmio,", sizeof ("mmio,") - 1) == 0

I think grub_strncmp() in string context would be more appropriate.
Please replace grub_memcmp() with grub_strncmp() where possible.

> +  && grub_isxdigit (name [sizeof ("mmio,") - 1]))
> +{
> +  const char *p1, *p = [sizeof ("mmio,") - 1];
> +  grub_addr_t addr = grub_strtoul (p, , 16);

You blindly assume grub_strtoul() will not fail. It is not true.
Please take a look at commit ac8a37dda (net/http: Allow use of
non-standard TCP/IP ports) how properly detect grub_strtoul() failures.

> +  unsigned int acc_size = 1;
> +  unsigned int nlen = p1 - p;

Please add empty line here.

> +  if (p1[0] == '.')
> +switch(p1[1])
> +   {
> +   case 'w':
> +  acc_size = 2;
> +  break;
> +   case 'l':
> +  acc_size = 3;
> +  break;
> +   case 'q':
> +  acc_size = 4;
> +  break;

Does not compiler complain there is no default here? I think I saw such
warnings somewhere when it is missing.

> +  }
> +  name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
> +  if (!name)
> + return NULL;
> +
> +  FOR_SERIAL_PORTS (port)
> +if (grub_strncmp (port->name, name, nlen) == 0) {
> +   break;
> + }
> +}
>if (!port && grub_strcmp (name, "auto") == 0)
>  {
>/* Look for an SPCR if any. If not, default to com0 */
> @@ -212,7 +241,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>
>if (state[OPTION_PORT].set)
>  {
> -  if (grub_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 0)
> +  if (grub_memcmp (state[OPTION_PORT].arg, "mmio,", 5) == 0)

s/grub_memcmp/grub_strncmp/

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 6/7] ns8250: Support more MMIO access sizes

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:25AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt 
>
> It is common for PCI based UARTs to use larger than one byte access
> sizes. This adds support for this and uses the information present
> in SPCR accordingly.
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
>  grub-core/term/ns8250-spcr.c |  3 ++-
>  grub-core/term/ns8250.c  | 45 +---
>  include/grub/serial.h|  9 ++--
>  3 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> index 0786aee1b..dd589af60 100644
> --- a/grub-core/term/ns8250-spcr.c
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -73,7 +73,8 @@ grub_ns8250_spcr_init (void)
>switch (spcr->base_addr.space_id)
>  {
>case GRUB_ACPI_GENADDR_MEM_SPACE:
> -return grub_serial_ns8250_add_mmio(spcr->base_addr.addr, );
> +return grub_serial_ns8250_add_mmio(spcr->base_addr.addr,
> +   spcr->base_addr.access_size, 
> );
>case GRUB_ACPI_GENADDR_IO_SPACE:
>  return grub_serial_ns8250_add_port(spcr->base_addr.addr, );
>default:
> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index 08dfb99da..76f68c037 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -48,7 +48,26 @@ ns8250_reg_read (struct grub_serial_port *port, 
> grub_addr_t reg)
>  {
>asm volatile("" : : : "memory");
>if (port->mmio)
> -return *((volatile unsigned char *)(port->mmio_base + reg));
> +{
> +  /* Note: we assume MMIO UARTs are little endian. This is not true of 
> all
> +   * embedded platforms but will do for now
> +   */
> +  switch(port->access_size)
> +{
> +default:
> +case 1:
> +  return *((volatile unsigned char  *)(port->mmio_base + reg));

s/unsigned char/grub_uint8_t/a

And now I think ns8250_reg_read()/ns8250_reg_write() in patch #3 probably
should use grub_uint8_t instead of "unsigned char" everywhere...

> +case 2:
> +  return grub_le_to_cpu16(*((volatile unsigned short 
> *)(port->mmio_base + (reg << 1;

s/unsigned short/grub_uint16_t/

> +case 3:
> +  return grub_le_to_cpu32(*((volatile unsigned long 
> *)(port->mmio_base + (reg << 2;

s/unsigned long/grub_uint32_t/

I do not mention cast coding style issue...

> +case 4:
> +  /* This will only work properly on 64-bit systems, thankfully it
> +   * also probably doesn't exist
> +   */
> +  return grub_le_to_cpu64(*((volatile unsigned long long 
> *)(port->mmio_base + (reg << 3;

s/unsigned long long/grub_uint64_t/

Then maybe comment after "case 4" can be dropped...

> +}
> +}
>return grub_inb (port->port + reg);
>  }
>
> @@ -57,7 +76,24 @@ ns8250_reg_write (struct grub_serial_port *port, unsigned 
> char value, grub_addr_
>  {
>asm volatile("" : : : "memory");
>if (port->mmio)
> -*((volatile char *)(port->mmio_base + reg)) = value;
> +{
> +  switch(port->access_size)
> +{
> +default:
> +case 1:
> +  *((volatile unsigned char *)(port->mmio_base + reg)) = value;

s/unsigned char/grub_uint8_t/a

And please fix the similar problems accordingly in this and other patches...

> +  break;
> +case 2:
> +  *((volatile unsigned short *)(port->mmio_base + (reg << 1))) = 
> grub_cpu_to_le16(value);
> +  break;
> +case 3:
> +  *((volatile unsigned long *)(port->mmio_base + (reg << 2))) = 
> grub_cpu_to_le32(value);
> +  break;
> +case 4:
> +  *((volatile unsigned long long *)(port->mmio_base + (reg << 3))) = 
> grub_cpu_to_le64(value);
> +  break;
> +}
> +}
>else
>  grub_outb (value, port->port + reg);
>  }
> @@ -285,6 +321,7 @@ grub_ns8250_init (void)
> grub_print_error ();
>
>   grub_serial_register (_ports[i]);
> + com_ports[i].access_size = 1;
>}
>  }
>
> @@ -338,6 +375,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct 
> grub_serial_config *config
>p->driver = _ns8250_driver;
>p->mmio = 0;
>p->port = port;
> +  p->access_size = 1;
>if (config)
>  grub_serial_port_configure (p, config);
>else
> @@ -348,7 +386,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct 
> grub_serial_config *config
>  }
>
>  char *
> -grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config 
> *config)
> +grub_serial_ns8250_add_mmio(grub_addr_t addr, unsigned int acc_size, struct 
> grub_serial_config *config)
>  {
>struct grub_serial_port *p;
>unsigned i;
> @@ -372,6 +410,7 @@ grub_serial_ns8250_add_mmio(grub_addr_t addr, struct 
> grub_serial_config *config)
>p->driver = _ns8250_driver;
>p->mmio = 1;
>p->mmio_base = addr;
> +  p->access_size = acc_size;
>if (config)
>  

Re: [PATCH 5/7] ns8250: Use ACPI SPCR table when available to configure serial

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:24AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt 
>
> "serial auto" is now equivalent to just "serial" and will use the
> SPCR to discover the port if present, otherwise defaults to "com0"
> as before.
>
> This allows to support MMIO ports specified by ACPI which is needed
> on AWS EC2 "metal" instances, and will enable grub to pickup the

s/grub/GRUB/

> port configuration specified by ACPI in other cases.
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
>  docs/grub.texi   | 10 -
>  grub-core/Makefile.core.def  |  1 +
>  grub-core/term/ns8250-spcr.c | 82 
>  grub-core/term/serial.c  | 13 +-
>  include/grub/serial.h|  1 +
>  5 files changed, 105 insertions(+), 2 deletions(-)
>  create mode 100644 grub-core/term/ns8250-spcr.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 50c811a88..bbebc2aef 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -2719,6 +2719,10 @@ you want to use COM2, you must specify @samp{--unit=1} 
> instead. This
>  command accepts many other options, so please refer to @ref{serial},
>  for more details.
>
> +Without argument or with @samp{--port=auto}, GRUB will attempt to use
> +ACPI when available to auto-detect the default serial port and its
> +configuration.
> +
>  The commands @command{terminal_input} (@pxref{terminal_input}) and
>  @command{terminal_output} (@pxref{terminal_output}) choose which type of
>  terminal you want to use. In the case above, the terminal will be a
> @@ -2737,7 +2741,6 @@ implements few VT100 escape sequences. If you specify 
> this option then
>  GRUB provides you with an alternative menu interface, because the normal
>  menu requires several fancy features of your terminal.
>
> -

Please drop this change.

>  @node Vendor power-on keys
>  @chapter Using GRUB with vendor power-on keys
>
> @@ -4172,6 +4175,11 @@ be in the range 5-8 and stop bits must be 1 or 2. 
> Default is 8 data
>  bits and one stop bit. @var{parity} is one of @samp{no}, @samp{odd},
>  @samp{even} and defaults to @samp{no}.
>
> +In passed no @var{unit} nor @var{port}, or if @var{port} is set to

s/In/If/?

> +@samp{auto} then GRUB will attempt to use ACPI to automatically detect
> +the system default serial port and its configuration. If this information
> +is not available, it will default to @var{unit} 0.
> +
>  The serial port is not used as a communication channel unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 95942fc8c..b656bdb44 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2048,6 +2048,7 @@ module = {
>name = serial;
>common = term/serial.c;
>x86 = term/ns8250.c;
> +  x86 = term/ns8250-spcr.c;
>ieee1275 = term/ieee1275/serial.c;
>mips_arc = term/arc/serial.c;
>efi = term/efi/serial.c;
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> new file mode 100644
> index 0..0786aee1b
> --- /dev/null
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -0,0 +1,82 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010  Free 
> Software Foundation, Inc.

s/2000,2001,2002,2003,2004,2005,2007,2008,2009,2010/2022/

> + *
> + *  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 .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +char *
> +grub_ns8250_spcr_init (void)
> +{
> +  struct grub_acpi_spcr *spcr;
> +  struct grub_serial_config config;
> +
> +  spcr = grub_acpi_find_table (GRUB_ACPI_SPCR_SIGNATURE);
> +  if (!spcr)

spcr == NULL

> +return 0;

Please use NULL instead of 0 in pointer context. Please fix same
problem in the other places/patches too.

> +  if (spcr->hdr.revision < 2)
> +return 0;
> +  if (spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550 &&
> +  spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550X)
> +return 0;
> +  /* For now, we only support byte accesses */
> +  if (spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_BYTE &&
> +  spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_LGCY)
> +return 0;
> +  config.word_len = 8;
> +  config.parity = 

Re: [PATCH 4/7] ns8250: Add configuration parameter when adding ports

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:23AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt 
>
> This will allow ports to be added with a pre-set configuration
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
>  grub-core/term/ns8250.c | 25 +++--
>  grub-core/term/serial.c |  2 +-
>  include/grub/serial.h   |  4 ++--
>  3 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index b640508d0..08dfb99da 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -303,7 +303,7 @@ grub_ns8250_hw_get_port (const unsigned int unit)
>  }
>
>  char *
> -grub_serial_ns8250_add_port (grub_port_t port)
> +grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config 
> *config)
>  {
>struct grub_serial_port *p;
>unsigned i;
> @@ -312,6 +312,9 @@ grub_serial_ns8250_add_port (grub_port_t port)
>{
>   if (dead_ports & (1 << i))
> return NULL;
> + /* give the opportunity for SPCR to configure a default com port */
> + if (config)
> +   grub_serial_port_configure (_ports[i], config);
>   return com_names[i];
>}
>
> @@ -333,22 +336,29 @@ grub_serial_ns8250_add_port (grub_port_t port)
>return NULL;
>  }
>p->driver = _ns8250_driver;
> -  grub_serial_config_defaults (p);
>p->mmio = 0;
>p->port = port;
> +  if (config)
> +grub_serial_port_configure (p, config);
> +  else
> +grub_serial_config_defaults (p);
>grub_serial_register (p);
>
>return p->name;
>  }
>
>  char *
> -grub_serial_ns8250_add_mmio(grub_addr_t addr)
> +grub_serial_ns8250_add_mmio(grub_addr_t addr, struct grub_serial_config 
> *config)

If you touch the code and its coding style is broken please fix it. Here
a space is missing before "(".

>  {
>struct grub_serial_port *p;
>unsigned i;
>for (i = 0; i < GRUB_SERIAL_PORT_NUM; i++)
> -if (com_ports[i].mmio &&  com_ports[i].mmio_base == addr)
> - return com_names[i];
> +if (com_ports[i].mmio && com_ports[i].mmio_base == addr)
> +  {
> +if (config)
> +  grub_serial_port_configure (_ports[i], config);
> +return com_names[i];
> +  }
>
>p = grub_malloc (sizeof (*p));
>if (!p)
> @@ -360,9 +370,12 @@ grub_serial_ns8250_add_mmio(grub_addr_t addr)
>return NULL;
>  }
>p->driver = _ns8250_driver;
> -  grub_serial_config_defaults (p);
>p->mmio = 1;
>p->mmio_base = addr;
> +  if (config)
> +grub_serial_port_configure (p, config);
> +  else
> +grub_serial_config_defaults (p);
>grub_serial_register (p);
>
>return p->name;
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index c8f5f02db..a1707d2f7 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -156,7 +156,7 @@ grub_serial_find (const char *name)
>&& grub_isxdigit (name [sizeof ("port") - 1]))
>  {
>name = grub_serial_ns8250_add_port (grub_strtoul ([sizeof 
> ("port") - 1],
> - 0, 16));
> + 0, 16), NULL);
>if (!name)
>   return NULL;
>
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index ccf464d41..7efc956b0 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -185,8 +185,8 @@ grub_serial_config_defaults (struct grub_serial_port 
> *port)
>
>  #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
>  void grub_ns8250_init (void);
> -char *grub_serial_ns8250_add_port (grub_port_t port);
> -char *grub_serial_ns8250_add_mmio(grub_addr_t addr);
> +char *grub_serial_ns8250_add_port (grub_port_t port, struct 
> grub_serial_config *config);
> +char *grub_serial_ns8250_add_mmio(grub_addr_t addr, struct 
> grub_serial_config *config);

Again, please do not break coding style...

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] ns8250: Add base support for MMIO UARTs

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:22AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt 
>
> This adds the ability for the driver to access UARTs via MMIO instead
> of PIO selectively at runtime, and exposes a new function to add an
> MMIO port.
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
>  grub-core/term/ns8250.c | 78 -
>  grub-core/term/serial.c |  8 +++--
>  include/grub/serial.h   | 11 +-
>  3 files changed, 78 insertions(+), 19 deletions(-)
>
> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index 83b25990c..b640508d0 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -43,6 +43,24 @@ static int dead_ports = 0;
>  #define DEFAULT_BASE_CLOCK 115200
>  #endif
>
> +static inline unsigned char

Please drop inline from here. Then compiler will be free to optimize out
or not this function.

> +ns8250_reg_read (struct grub_serial_port *port, grub_addr_t reg)
> +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +return *((volatile unsigned char *)(port->mmio_base + reg));
> +  return grub_inb (port->port + reg);
> +}
> +
> +static inline void

Ditto...

> +ns8250_reg_write (struct grub_serial_port *port, unsigned char value, 
> grub_addr_t reg)
> +{
> +  asm volatile("" : : : "memory");
> +  if (port->mmio)
> +*((volatile char *)(port->mmio_base + reg)) = value;

Casts require space like this: (volatile char *) (port->mmio_base + reg).
Please fix casts in other places/patches too.

> +  else
> +grub_outb (value, port->port + reg);
> +}
>
>  /* Convert speed to divisor.  */
>  static unsigned short
> @@ -93,43 +111,42 @@ do_real_config (struct grub_serial_port *port)
>divisor = serial_get_divisor (port, >config);
>
>/* Turn off the interrupt.  */
> -  grub_outb (0, port->port + UART_IER);
> +  ns8250_reg_write (port, 0, UART_IER);
>
>/* Set DLAB.  */
> -  grub_outb (UART_DLAB, port->port + UART_LCR);
> +  ns8250_reg_write (port, UART_DLAB, UART_LCR);
>
> -  /* Set the baud rate.  */
> -  grub_outb (divisor & 0xFF, port->port + UART_DLL);
> -  grub_outb (divisor >> 8, port->port + UART_DLH);
> +  ns8250_reg_write (port, divisor & 0xFF, UART_DLL);
> +  ns8250_reg_write (port, divisor >> 8, UART_DLH);
>
>/* Set the line status.  */
>status |= (parities[port->config.parity]
>| (port->config.word_len - 5)
>| stop_bits[port->config.stop_bits]);
> -  grub_outb (status, port->port + UART_LCR);
> +  ns8250_reg_write (port, status, UART_LCR);
>
>if (port->config.rtscts)
>  {
>/* Enable the FIFO.  */
> -  grub_outb (UART_ENABLE_FIFO_TRIGGER1, port->port + UART_FCR);
> +  ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER1, UART_FCR);
>
>/* Turn on DTR and RTS.  */
> -  grub_outb (UART_ENABLE_DTRRTS, port->port + UART_MCR);
> +  ns8250_reg_write (port, UART_ENABLE_DTRRTS, UART_MCR);
>  }
>else
>  {
>/* Enable the FIFO.  */
> -  grub_outb (UART_ENABLE_FIFO_TRIGGER14, port->port + UART_FCR);
> +  ns8250_reg_write (port, UART_ENABLE_FIFO_TRIGGER14, UART_FCR);
>
>/* Turn on DTR, RTS, and OUT2.  */
> -  grub_outb (UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, port->port + 
> UART_MCR);
> +  ns8250_reg_write (port, UART_ENABLE_DTRRTS | UART_ENABLE_OUT2, 
> UART_MCR);
>  }
>
>/* Drain the input buffer.  */
>endtime = grub_get_time_ms () + 1000;
> -  while (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
> +  while (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
>  {
> -  grub_inb (port->port + UART_RX);
> +  ns8250_reg_read (port, UART_RX);
>if (grub_get_time_ms () > endtime)
>   {
> port->broken = 1;
> @@ -145,8 +162,8 @@ static int
>  serial_hw_fetch (struct grub_serial_port *port)
>  {
>do_real_config (port);
> -  if (grub_inb (port->port + UART_LSR) & UART_DATA_READY)
> -return grub_inb (port->port + UART_RX);
> +  if (ns8250_reg_read (port, UART_LSR) & UART_DATA_READY)
> +return ns8250_reg_read (port, UART_RX);
>
>return -1;
>  }
> @@ -166,7 +183,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
>else
>  endtime = grub_get_time_ms () + 200;
>/* Wait until the transmitter holding register is empty.  */
> -  while ((grub_inb (port->port + UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
> +  while ((ns8250_reg_read (port, UART_LSR) & UART_EMPTY_TRANSMITTER) == 0)
>  {
>if (grub_get_time_ms () > endtime)
>   {
> @@ -179,7 +196,7 @@ serial_hw_put (struct grub_serial_port *port, const int c)
>if (port->broken)
>  port->broken--;
>
> -  grub_outb (c, port->port + UART_TX);
> +  ns8250_reg_write (port, c, UART_TX);
>  }
>
>  /* Initialize a serial device. PORT is the port number for a serial device.
> @@ -262,6 +279,7 @@ grub_ns8250_init (void)
>   com_ports[i].name = com_names[i];
>   com_ports[i].driver = _ns8250_driver;
>   com_ports[i].port = 

Re: [PATCH v2 2/3] term/serial: Add support for PCI serial devices

[Daniel Kiper]

Sorry for late reply...

May I ask you to send the patches using "git send-email"?

On Fri, Aug 26, 2022 at 01:01:44PM +0200, pet...@infradead.org wrote:
> Loosely based on early_pci_serial_init() from Linux, allow GRUB to make
> use of PCI serial devices.
>
> Specifically, my Alderlake NUC exposes the Intel AMT SoL UART as a PCI
> enumerated device but doesn't include it in the EFI tables.
>
> Tested and confirmed working on a "Lenovo P360 Tiny" with Intel AMT
> enabled. This specific machine has (from lspci -vv):
>
> 00:16.3 Serial controller: Intel Corporation Device 7aeb (rev 11) (prog-if 02 
> [16550])
> DeviceName: Onboard - Other
> Subsystem: Lenovo Device 330e
> Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- 
> SERR-  Interrupt: pin D routed to IRQ 19
> Region 0: I/O ports at 40a0 [size=8]
> Region 1: Memory at b4224000 (32-bit, non-prefetchable) [size=4K]
> Capabilities: [40] MSI: Enable- Count=1/1 Maskable- 64bit+
> Address:   Data: 
> Capabilities: [50] Power Management version 3
> Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
> PME(D0-,D1-,D2-,D3hot-,D3cold-)
> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
> Kernel driver in use: serial
>
> >From which the following config (/etc/default/grub) gets a working
> serial setup:
>
> GRUB_CMDLINE_LINUX="console=tty0 earlyprintk=pciserial,00:16.3,115200 
> console=ttyS0,115200"
> GRUB_SERIAL_COMMAND="serial --port=0x40a0 --speed=115200"

Please add a blurb how to use this driver to the docs/grub.texi file.

> GRUB_TERMINAL="serial console"
>
> Signed-off-by: Peter Zijlstra (Intel) 
> ---
>
> Index: grub/grub-core/Makefile.core.def
> ===
> --- grub.orig/grub-core/Makefile.core.def
> +++ grub/grub-core/Makefile.core.def
> @@ -2047,6 +2047,7 @@ module = {
>ieee1275 = term/ieee1275/serial.c;
>mips_arc = term/arc/serial.c;
>efi = term/efi/serial.c;
> +  pci = term/pci/serial.c;

I think this should be "x86 = term/pci/serial.c".

>
>enable = terminfomodule;
>enable = ieee1275;
> Index: grub/grub-core/term/pci/serial.c
> ===
> --- /dev/null
> +++ grub/grub-core/term/pci/serial.c
> @@ -0,0 +1,95 @@
> +/*
> + *  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 .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int
> +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)

grub_pci_id_t pciid __attribute__ ((unused))

> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  int *port_num = data;
> +  grub_err_t err;
> +
> +  (void)pciid;

... and please drop this...

> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* 16550 compattible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +   (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +  ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (port == NULL)
> +return 0;
> +
> +  port->name = grub_xasprintf ("pci%d", (*port_num));
> +  if (port->name == NULL)
> +goto error;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->driver = _ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err != GRUB_ERR_NONE)
> +{
> +  grub_print_error ();
> +  goto error;
> +}
> +
> +  err = 

[RFC PATCH v2 1/1] kern/dl: Add module version check

[Zhang Boyang]

This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time.

If GRUB is locked down, modules with mismatched version will be
rejected. This is necessary for implementing external signed modules
securely (in future).

For compatibility reasons, if GRUB isn't locked down, modules can still
be loaded even if they have mismatched version. A warning message will
be emited in that situation.

It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).

Link: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html
Link: https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html
Signed-off-by: Zhang Boyang 
---
 grub-core/Makefile.am |  2 +-
 grub-core/genmod.sh.in| 28 +---
 grub-core/kern/dl.c   | 33 +
 util/grub-module-verifierXX.c | 10 ++
 4 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 80e7a83ed..d76aa80f4 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
 CLEANFILES += gentrigtables$(BUILD_EXEEXT)
 
 build-grub-module-verifier$(BUILD_EXEEXT): 
$(top_srcdir)/util/grub-module-verifier.c 
$(top_srcdir)/util/grub-module-verifier32.c 
$(top_srcdir)/util/grub-module-verifier64.c 
$(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
-   $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) 
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
+   $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS) 
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1 
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" 
-DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^
 CLEANFILES += build-grub-module-verifier$(BUILD_EXEEXT)
 
 # trigtables.c
diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index e57c4d920..58a4cdcbe 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
 rm -f $tmpfile $outfile
 
 if test x@TARGET_APPLE_LINKER@ != x1; then
-# stripout .modname and .moddeps sections from input module
-@TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
+# stripout .modname and .moddeps and .modver sections from input module
+@TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile $tmpfile
 
-# Attach .modname and .moddeps sections
+# Attach .modname and .moddeps and .modver sections
 t1=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
 printf "$modname\0" >$t1
 
 t2=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
 for dep in $deps; do printf "$dep\0" >> $t2; done
 
+t3=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+printf "@PACKAGE_VERSION@\0" >$t3
+
 if test -n "$deps"; then
-   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 
$tmpfile
+   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2 
--add-section .modver=$t3 $tmpfile
 else
-   @TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
+   @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .modver=$t3 
$tmpfile
 fi
-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
 
if test x@platform@ != xemu; then
@TARGET_STRIP@ --strip-unneeded \
@@ -71,23 +74,26 @@ else
 tmpfile2=${outfile}.tmp2
 t1=${outfile}.t1.c
 t2=${outfile}.t2.c
+t3=${outfile}.t3.c
 
 # remove old files if any
-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
 
 cp $infile $tmpfile
 
-# Attach .modname and .moddeps sections
+# Attach .modname and .moddeps and .modver sections
 echo "char modname[]  __attribute__ ((section(\"_modname, _modname\"))) = 
\"$modname\";" >$t1
 
 for dep in $deps; do echo "char moddep_$dep[] __attribute__ 
((section(\"_moddeps, _moddeps\"))) = \"$dep\";" >>$t2; done
 
+echo "char modver[]  __attribute__ ((section(\"_modver, _modver\"))) = 
\"@PACKAGE_VERSION@\";" >$t3
+
 if test -n "$deps"; then
-   @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 
$t2 $tmpfile -Wl,-r
+   @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 
$t2 $t3 $tmpfile -Wl,-r
 else
-   @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 
$tmpfile -Wl,-r
+   @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1 
$t3 $tmpfile -Wl,-r
 fi
-rm -f $t1 $t2 $tmpfile
+rm -f $t1 $t2 $t3 $tmpfile
 mv $tmpfile2 $tmpfile
 
cp $tmpfile $tmpfile.bin
diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index 

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 17:43, Thomas Schmitt wrote:

Hi,

Pete Batard wrote:

unlike what is the case for UEFI, one can not expect
to be able to pick all the GRUB core files needed to convert a GRUB
based ISO bootable media to a GRUB based USB bootable media [...]
Typically, one of the
missing files will be a 'core.img' that can work for USB BIOS boot of a
MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
image will not readily provide.


Zhang Boyang wrote:

The situation is, for BIOS boot, there is no grub core file in ISO
because it's written to sectors instead of files? Did I understand
correctly?


I think it is rather about the fact that GRUB equipped hybrid ISOs boot
on legacy BIOS from USB stick via the MBR code (~440 byte) to the
El Torito boot image, which is in charge for booting from optical media.
In grub-mkrescue ISOs it is called
   /boot/grub/i386-pc/eltorito.img
The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].

So no core.img is needed and the filesystem module set can be sparse,
because the filesystem type is known from which GRUB will read its further
software.




Thanks for explaining! So I think the best solution for Rufus is to 
overwrite distro's .mod files with its owns (if mismatched modules is a 
problem).



Pete Batard wrote:

Rufus usually recommends to write such images in what it calls
"ISO mode" (shorthand for "ISO extraction mode") with the ISO content
extracted to a FAT or NTFS file system and with a GRUB core.img
bootloader


Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
equipment that is needed to make bootable a USB stick with FAT or NTFS.
(The filesystem modules are included in Pete Batard's proposal for the
  /EFI/BOOT tree. But the disk MBR image and core.img are not.)


Zhang Boyang wrote:

test whether that commit (052e6068be62) breaks Rufus
(e.g. Rufus with latest git GRUB, to boot Debian ISO).


Debian ISOs still boot on legacy BIOS via ISOLINUX.
Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs for
both, legacy BIOS and EFI.



Oh, yes, Debian seems using isolinux instead of grub So please use 
Ubuntu ISOs for testing, which uses grub in BIOS mode. By the way, I 
also tried Arch Linux, it also uses isolinux (but it has a theme looks 
like grub).


I did reversed tests (i.e. old GRUB core + new GRUB modules) just now:

1) I used Rufus 3.21 and ubuntu-22.04.1-desktop-amd64.iso (which uses 
GRUB 2.06) to create a USB stick. To simulate mismatched modules, I 
overwrited "\boot\grub\i386-pc\relocator.mod" (on USB stick) with my 
latest git build. Then that USB stick failed to boot in BIOS mode, 
showing "452: out of range pointer: 0x100010" error.


2) I tested supergrub2-2.06s1-beta2-multiarch-CD.iso with latest Arch 
Linux (BIOS mode, grub 2:2.06.r403.g7259d55ff-1). It doesn't have 
problems booting Arch Linux because it has $prefix properly set to its 
own GRUB path in CDROM, so it is using its own modules. If I manually 
run "set prefix=(hd0,msdos1)/boot/grub", it will failed to boot, showing 
"error: symbol `grub_debug_malloc' not found."


By the way, I googled "452: out of range pointer" and there seems a lot 
of them. Probably the root cause of many of them is mismatched modules, 
so I think giving a clear message to user should be helpful (if similar 
things happens in future).


Best Regards,
Zhang Boyang


I think that in any case an ISO made by grub-mkrescue should be tested.
Maybe a distro developer is here who uses it for making a full fledged
installation ISO or a live ISO.


Have a nice day :)

Thomas



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Didier Spaier]

Le 21/12/2022 à 09:43, Thomas Schmitt a écrit :
> I think that in any case an ISO made by grub-mkrescue should be tested.
> Maybe a distro developer is here who uses it for making a full fledged
> installation ISO or a live ISO.

I do for making a full fledged installation ISO in this function, with the
options you suggested privately:

make_iso() {
export MKRESCUE_SED_PARTNO=2
export MKRESCUE_SED_MODE=mjg
grub-mkrescue --locales='' --themes='' --compress=xz -V "$ISOLABEL" \
-o $PATHTOISO -iso_mbr_part_type 0x83 -partition_offset 16 -J \
--xorriso=./grub/grub-mkrescue-sed.sh $ISODIR
}

I'd be glad to help tetsing, if given detailed instructions about what to test
and how, as I am not a developer. However I can't test in Secure Boot enabled
context.

Cheers,
Didier

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Thomas Schmitt]

Hi,

Pete Batard wrote:
> > unlike what is the case for UEFI, one can not expect
> > to be able to pick all the GRUB core files needed to convert a GRUB
> > based ISO bootable media to a GRUB based USB bootable media [...]
> > Typically, one of the
> > missing files will be a 'core.img' that can work for USB BIOS boot of a
> > MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid
> > image will not readily provide.

Zhang Boyang wrote:
> The situation is, for BIOS boot, there is no grub core file in ISO
> because it's written to sectors instead of files? Did I understand
> correctly?

I think it is rather about the fact that GRUB equipped hybrid ISOs boot
on legacy BIOS from USB stick via the MBR code (~440 byte) to the
El Torito boot image, which is in charge for booting from optical media.
In grub-mkrescue ISOs it is called
  /boot/grub/i386-pc/eltorito.img
The MBR code is taken by grub-mkrescue.c from the file "boot_hybrid.img"
in source_dirs[GRUB_INSTALL_PLATFORM_I386_PC].

So no core.img is needed and the filesystem module set can be sparse,
because the filesystem type is known from which GRUB will read its further
software.


Pete Batard wrote:
> > Rufus usually recommends to write such images in what it calls
> > "ISO mode" (shorthand for "ISO extraction mode") with the ISO content
> > extracted to a FAT or NTFS file system and with a GRUB core.img
> > bootloader

Maybe one should not only put a copy of the EFI boot image's /EFI/BOOT
tree into grub-mkrescue ISOs but also an outdoor pack with the GRUB
equipment that is needed to make bootable a USB stick with FAT or NTFS.
(The filesystem modules are included in Pete Batard's proposal for the
 /EFI/BOOT tree. But the disk MBR image and core.img are not.)


Zhang Boyang wrote:
> test whether that commit (052e6068be62) breaks Rufus
> (e.g. Rufus with latest git GRUB, to boot Debian ISO).

Debian ISOs still boot on legacy BIOS via ISOLINUX.
Archlinux, Ubuntu, Fedora, and others meanwhile use GRUB in their ISOs for
both, legacy BIOS and EFI.

I think that in any case an ISO made by grub-mkrescue should be tested.
Maybe a distro developer is here who uses it for making a full fledged
installation ISO or a live ISO.


Have a nice day :)

Thomas


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 10:43, Pete Batard via Grub-devel wrote:

Hello all,

On 2022.12.20 22:58, Robbie Harwood wrote:

Zhang Boyang  writes:


This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time. That module will
be rejected if mismatch is found. This will prevent loading modules from
different versions of GRUB.

It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).


Right now, this only affects non-secureboot scenarios (because we don't
have external signed module support).  I would want to see a resolution
to the external module signing question first so that we don't paint
ourselves into a corner with something like this.

I share Glenn's confusion about what real-world problems this addresses:
my understanding is that grub modules mostly register callbacks, so the
possibility of disaster is low (unless the callback interfaces change of
course, but that generally has not happened).


I'll add a note that integrating this proposal will make life A LOT more 
difficult for Rufus [1] users (and potentially users of other boot media 
creation software) when booting in BIOS/CSM mode.


The reason is that, unlike what is the case for UEFI, one can not expect 
to be able to pick all the GRUB core files needed to convert a GRUB 
based ISO bootable media to a GRUB based USB bootable media when using 
the kind File System Transposition one needs to apply, in order to avoid 
end-users, especially Windows ones, being left confused as to why their 
USB media is not showing the content they expect. Typically, one of the 
missing files will be a 'core.img' that can work for USB BIOS boot of a 
MBR partitioned FAT or NTFS media, and that even a GRUB based ISOHybrid 
image will not readily provide.




The situation is, for BIOS boot, there is no grub core file in ISO 
because it's written to sectors instead of files? Did I understand 
correctly?


As a result, whereas it can also write a GRUB base ISOHybrid image in DD 
mode, Rufus usually recommends to write such images in what it calls 
"ISO mode" (shorthand for "ISO extraction mode") with the ISO content 
extracted to a FAT or NTFS file system and with a GRUB core.img 
bootloader being installed by the application itself rather than being 
provided by the source image. This core.img, which is usually recompiled 
from GRUB dot releases to include support for MBR partition scheme, FAT, 
NTFS and potentially other required elements, then calls into the GRUB 
modules that were extracted from the ISO image.


And whereas, on paper, this may look a recipe for disaster, ~8 years of 
boot media creation for GRUB 2.x, using this default method of mixing a 
GRUB core.img that wasn't built alongside the modules it is using, shows 
the method to actually work quite well, *even* when most distros take it 
upon themselves to cherry-pick an extra selection of GRUB patches to 
their source while waiting for the next dot release.




Please see my reply to Glenn for a real example of mismatched modules 
crashes GRUB ( 
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00229.html ). I 
strongly recommend test whether that commit (052e6068be62) breaks Rufus 
(e.g. Rufus with latest git GRUB, to boot Debian ISO).


However, if this proposal gets applied for BIOS modules, this will break 
the mechanism described above thereby penalising users who have come to 
rely on Rufus on account that most distros, and especially the ones that 
add patches to their GRUB sources, tend to use a distro specific version 
for GRUB (e.g. "2.06~unbuntu_xyz").




This patch lives in core, so even it's enforced (I will not enforce this 
check in next patch reversion, see below), Rufus can still ship its own 
unpatched version of grub core and thing will still work.


Thus, right at the time where we are finally seeing light at the end of 
the tunnel for UEFI, with regards to the root of the problem we are 
trying to address (about Windows users thinking their boot media is not 
created properly when using DD mode, and therefore not even trying to 
boot to Linux -- this is a VERY REAL issue for which I have provided a 
large number of examples already), by ensuring that GRUB will properly 
support File System Transposition for UEFI moving forward (with my 
thanks to Daniel for having reviewed the respective patch series just 
today!) this proposal is now planning to undo the whole File System 
Transposition support *equivalent* we spent a lot of time trying to find 
a workaround for, for BIOS...




Will overwriting .mod files in USB drive with Rufus's build helps?

I therefore have to second asking what kind of real-world problems this 
is meant to address, especially as I can vouch that, if GRUB goes 
through with this proposal 

Re: [RFC PATCH] kern/dl: Add module version check

[Zhang Boyang]

Hi,

On 2022/12/21 03:04, Glenn Washburn wrote:

On Mon, 19 Dec 2022 23:33:29 +0800
Zhang Boyang  wrote:


This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time. That module
will be rejected if mismatch is found. This will prevent loading
modules from different versions of GRUB.

It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is
not noticed by version check (e.g. configure options).


Is this solving a real world problem? I've been loading modules from
other GRUB versions for years and have yet to notice an issue. Though,
I do see where issues could occur.



The main purpose is to implement truly loadable modules in secure boot 
environment (please see my reply to Robbie for details, 
https://lists.gnu.org/archive/html/grub-devel/2022-12/msg00228.html  ). 
But I did encountered strange crashes with mismatched modules, when 
using stock debian grub modules with latest git grub, on both UEFI and 
BIOS. If you are interested, here are reproducible steps:


1) Please install a fresh Debian (without GUI) into a virtual machine 
from debian-11.6.0-amd64-netinst.iso (Secure boot need to be disabled if 
using UEFI)


2) Build latest GRUB from git using these commands
sudo apt update
sudo apt install build-essential git
sudo apt build-dep grub2
git clone https://git.savannah.gnu.org/git/grub.git
cd grub
./bootstrap
./configure --prefix=/usr/local --with-platform=efi  # or 
--with-platform=pc if using BIOS

make -j2
sudo make install

3) Backup stock debian GRUB modules
sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.debian  # replace 
x86_64-efi with i386-pc if using BIOS, the same below


4) Install our build
sudo /usr/local/sbin/grub-install  # add /dev/sda or similar if using BIOS

5) Replace modules with stock modules
sudo mv /boot/grub/x86_64-efi /boot/grub/x86_64-efi.mybuild
sudo mv /boot/grub/x86_64-efi.debian /boot/grub/x86_64-efi

6) Reboot. GRUB should crash with messages like this:
Loading Linux 5.10.0-20-amd64 ...
452: out of range pointer: 0x3ff8da10

Aborted. Press any key to exit.


I ran `git bisect` and it reports this problem is introduced by 
052e6068be62 ("mm: When adding a region, merge with region after as well 
as before"). This commit changed the layout of `struct grub_mm_region`, 
which is both used in main program and "relocator.mod", so the module 
will read the wrong field and crashes GRUB.



There are two use cases that I understand this patch to potentially
break, which I think should continue to be supported. The first use
case is using isos which support the semi-standard loopback.cfg[1]. The
vast majority, if not all, of the uses of the loopback.cfg for loopback
mounted isos (eg. autoiso.cfg and isodetect.cfg) will create menu
entries where $root is set to the loopbacked iso file and then the
loopback.cfg is run. This patch could cause these created menu entries
to be broken if the modules needed for the commands in the loopback.cfg
have not been loaded ahead of time. This is because the $root is now
pointing to the iso device and, assuming $prefix has no device
component (usually the case), then modules will be attempted to be
loaded from the iso, not from the running grub install.

The other use case, which I use to boot my system, is having one GRUB
install load the grub.cfg of another GRUB install. Similar to the first
use case, the issue is that $root changed from the device of the
running GRUB to the device where the grub.cfg to be loaded is located.
So module loading, again, will attempted for modules that are not for
the running GRUB.



There seems no API/ABI compatibility guarantees in GRUB. So I don't 
think it is a good idea to mix different versions of main program and 
modules, and it should be consider unsupported (although it works in 
most situations). It is possible to avoid this by preloading modules 
before changing $root and/or $prefix (insmod will be no-op if given 
module name is already exists).



I'm not opposed in principle to adding a module version check, if these
issues can be resolved/mitigated. At a minimum it should be easy to
disable at build time (ie. a configure option to disable), and perhaps
even having it disabled by default for a release cycle. Another, more
flexible solution is to allow it to be enabled at runtime through a
special variable check (eg. grub_module_check=1).



Since this check is mainly designed for secure boot environments, I 
think the best way to revise this patch is to only enforce this check if 
grub is in lockdown mode (e.g. secure boot enabled). If not in lockdown 
mode, emit a warning but still load such mismatched module. Would you 
mind this solution (a ugly warning will be displayed if mismatched 
module is loaded) ?


Best Regards,
Zhang Boyang


Glenn

[1]