[PATCH v4 0/2] kern/xen: Add Xen command line parsing

[PATCH v4 0/2] kern/xen: Add Xen command line parsing

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 2501 bytes --]

The purpose of this patch is to allow the Xen hypervisor to pass extra
data to GRUB in the form of a kernel command line, allowing the host to
customize the boot process of the guest. The command line from Xen is
parsed, and any variables within that start with the string
`xen_grub_env_` are exposed as environment variables. The grub.cfg
script can then use those environment variables as it sees fit.

The main reason for doing this is to allow implementing boot modes in
Qubes OS while also using in-VM kernels. For more context on Qubes boot
modes, see [1]. In order for this to work with in-VM kernels, it is
necessary for dom0 to pass kernel parameters to the guest without
modifying the guest's grub.cfg manually. This patch allows this to be
done, by allowing dom0 to pass kernel parameters to GRUB, which then
provides them to grub.cfg as an environment variable. The grub.cfg
script within the VM can then append those variables to the kernel
command line.

All of the changes from version 3 of the patch are simply refinements
after a review from Daniel Kiper (coding style cleanups, avoiding
global variables, double-checking NUL-termination after using
grub_strncpy, etc.). The patch's functionality is unchanged.

Since there are substantial changes since version 3, I've re-run the
entire battery of tests that were used against the v3 patch, plus an
extra test to ensure an escaped control character won't be accepted in
variable values. The test results can be seen at [2]. As previously, the
tests were done by booting a patched GRUB in Xen with various different
parameters passed to it via the Xen-provided kernel command line. The
effects of these parameters on the bootloader's environment were then
tested, and then an Arch Linux image was booted to ensure that boot
still worked.

[1] https://github.com/QubesOS/qubes-linux-pvgrub2/pull/16
[2] https://bpa.st/3SBQ

Aaron Rainbolt (2):
  include/xen: Rename MAX_GUEST_CMDLINE to GRUB_XEN_MAX_GUEST_CMDLINE
  kern/xen: Add Xen command line parsing

 docs/grub.texi                |  51 +++++
 grub-core/Makefile.core.def   |   2 +
 grub-core/kern/i386/xen/pvh.c |  23 +++
 grub-core/kern/xen/cmdline.c  | 376 ++++++++++++++++++++++++++++++++++
 grub-core/kern/xen/init.c     |   2 +
 include/grub/xen.h            |   2 +
 include/xen/xen.h             |   4 +-
 7 files changed, 458 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/kern/xen/cmdline.c

-- 
2.50.1


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

[PATCH v4 1/2] include/xen: Rename MAX_GUEST_CMDLINE to GRUB_XEN_MAX_GUEST_CMDLINE

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 1162 bytes --]

The xen.h header was using an overly generic name to refer to the
maximum length of the command line passed from Xen to a guest. Rename it
to avoid confusion or conflicts in the future.

Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
---
 include/xen/xen.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/xen/xen.h b/include/xen/xen.h
index 692f97a..fdf0fc4 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -823,8 +823,8 @@ struct start_info {
                                 /* (PFN of pre-loaded module if           */
                                 /*  SIF_MOD_START_PFN set in flags).      */
     unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
-#define MAX_GUEST_CMDLINE 1024
-    int8_t cmd_line[MAX_GUEST_CMDLINE];
+#define GRUB_XEN_MAX_GUEST_CMDLINE 1024
+    int8_t cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE];
     /* The pfn range here covers both page table and p->m table frames.   */
     unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */
     unsigned long nr_p2m_frames;/* # of pfns forming initial P->M table.  */
-- 
2.50.1


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

[PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 19729 bytes --]

Xen traditionally allows customizing guest behavior by passing arguments
to the VM kernel via the kernel command line. This is no longer possible
when using GRUB with Xen, as the kernel command line is decided by the
GRUB configuration file within the guest, not data passed to the guest
by Xen.

To work around this limitation, enable GRUB to parse a command line
passed to it by Xen, and expose data from the command line to the GRUB
configuration as environment variables. These variables can be used in
the GRUB configuration for any desired purpose, such as extending the
kernel command line passed to the guest. The command line format is
inspired by the Linux kernel's command line format.

To reduce the risk of misuse, abuse, or accidents in production, the
command line will only be parsed if it consists entirely of 7-bit ASCII
characters, only alphabetical characters and underscores are permitted
in variable names, and all variable names must start with the string
"xen_grub_env_". This also allows room for expanding the command line
arguments accepted by GRUB in the future, should other arguments end up
becoming desirable in the future.

Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
---
 docs/grub.texi                |  51 +++++
 grub-core/Makefile.core.def   |   2 +
 grub-core/kern/i386/xen/pvh.c |  23 +++
 grub-core/kern/xen/cmdline.c  | 376 ++++++++++++++++++++++++++++++++++
 grub-core/kern/xen/init.c     |   2 +
 include/grub/xen.h            |   2 +
 6 files changed, 456 insertions(+)
 create mode 100644 grub-core/kern/xen/cmdline.c

diff --git a/docs/grub.texi b/docs/grub.texi
index 34b3484..b58cf98 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3271,6 +3271,7 @@ GRUB.  Others may be used freely in GRUB configuration files.
 @menu
 * Special environment variables::
 * Environment block::
+* Passing environment variables through Xen::
 @end menu
 
 
@@ -3871,6 +3872,56 @@ using BIOS or EFI functions (no ATA, USB or IEEE1275).
 @command{grub-mkconfig} uses this facility to implement
 @samp{GRUB_SAVEDEFAULT} (@pxref{Simple configuration}).
 
+@node Passing environment variables through Xen
+@section Passing environment variables through Xen
+
+If you are using a GRUB image as the kernel for a PV or PVH Xen virtual
+machine, you can pass environment variables from Xen's dom0 to the VM through
+the Xen-provided kernel command line. When combined with a properly configured
+guest, this can be used to customize the guest's behavior on bootup via the
+VM's Xen configuration file.
+
+GRUB will parse the kernel command line passed to it by Xen during bootup.
+The command line will be split into space-delimited words. Single and
+double quotes may be used to quote words or portions of words that contain
+spaces. Single quotes will be considered part of a word if inside double
+quotes, and vice versa. Arbitrary characters may be backslash-escaped to make
+them a literal component of a word rather than being parsed as quotes or word
+separators. The command line must consist entirely of printable 7-bit ASCII
+characters and spaces. If a non-printing ASCII character is found anywhere in
+the command line, the entire command line will be ignored by GRUB.
+
+Each word should be a variable assignment in the format ``variable'' or
+``variable=value''. Variable names must contain only the characters A-Z, a-z,
+and underscore (``_''). Variable names must begin with the string
+``xen_grub_env_''. Variable values can contain arbitrary printable 7-bit
+ASCII characters and space. If any variable contains an illegal name, that
+variable will be ignored.
+
+If a variable name and value are both specified, the variable will be set to
+the specified value. If only a variable name is specified, the variable's
+value will be set to ``1''.
+
+The following is a simple example of how to use this functionality to append
+arbitrary variables to a guest's kernel command line:
+
+@example
+# In the Xen configuration file for the guest
+name = "linux_vm"
+type = "pvh"
+kernel = "/path/to/grub-i386-xen_pvh.bin"
+extra = "xen_grub_env_linux_append='loglevel=3'"
+memory = 1024
+disk = [ "file:/srv/vms/linux_vm.img,sda,w" ]
+
+# In the guest's GRUB configuration file
+menuentry "Linux VM with dom0-specified kernel parameters" @{
+    search --set=root --label linux_vm --hint hd0,msdos1
+    linux /boot/vmlinuz root=LABEL=linux_vm $@{xen_grub_env_linux_append@}
+    initrd /boot/initrd.img
+@}
+@end example
+
 @node Modules
 @chapter Modules
 
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index b3f7119..df0f266 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -248,6 +248,7 @@ kernel = {
   xen = term/xen/console.c;
   xen = disk/xen/xendisk.c;
   xen = commands/boot.c;
+  xen = kern/xen/cmdline.c;
 
   i386_xen_pvh = commands/boot.c;
   i386_xen_pvh = disk/xen/xendisk.c;
@@ -255,6 +256,7 @@ kernel = {
   i386_xen_pvh = kern/i386/xen/tsc.c;
   i386_xen_pvh = kern/i386/xen/pvh.c;
   i386_xen_pvh = kern/xen/init.c;
+  i386_xen_pvh = kern/xen/cmdline.c;
   i386_xen_pvh = term/xen/console.c;
 
   ia64_efi = kern/ia64/efi/startup.S;
diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
index 91fbca8..a8988d2 100644
--- a/grub-core/kern/i386/xen/pvh.c
+++ b/grub-core/kern/i386/xen/pvh.c
@@ -321,6 +321,8 @@ void
 grub_xen_setup_pvh (void)
 {
   grub_addr_t par;
+  const char *xen_cmdline;
+  int i;
 
   grub_xen_cpuid_base ();
   grub_xen_setup_hypercall_page ();
@@ -352,6 +354,27 @@ grub_xen_setup_pvh (void)
   grub_xen_mm_init_regions ();
 
   grub_rsdp_addr = pvh_start_info->rsdp_paddr;
+
+  xen_cmdline = (const char *) pvh_start_info->cmdline_paddr;
+  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
+    {
+      if (xen_cmdline[i] == '\0')
+        {
+          grub_strncpy ((char *) grub_xen_start_page_addr->cmd_line,
+			(char *) pvh_start_info->cmdline_paddr,
+			GRUB_XEN_MAX_GUEST_CMDLINE);
+
+          if (grub_xen_start_page_addr->cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE - 1] != '\0')
+            {
+              grub_error (GRUB_ERR_BUG,
+			  "Xen command line is not NUL-terminated");
+              grub_print_error ();
+              grub_exit ();
+            }
+
+          break;
+        }
+    }
 }
 
 grub_err_t
diff --git a/grub-core/kern/xen/cmdline.c b/grub-core/kern/xen/cmdline.c
new file mode 100644
index 0000000..46a9998
--- /dev/null
+++ b/grub-core/kern/xen/cmdline.c
@@ -0,0 +1,376 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2025  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/env.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/xen.h>
+
+enum splitter_state
+{
+  SPLITTER_HIT_BACKSLASH = 0x1,
+  SPLITTER_IN_SINGLE_QUOTES = 0x2,
+  SPLITTER_IN_DOUBLE_QUOTES = 0x4,
+};
+typedef enum splitter_state splitter_state_t;
+
+/*
+ * The initial size of the current_word buffer. The buffer may be resized as
+ * needed.
+ */
+#define PARSER_BASE_WORD_SIZE 32
+
+struct parser_state
+{
+  grub_size_t word_list_len;
+  char **word_list;
+  grub_size_t current_word_len;
+  grub_size_t current_word_pos;
+  char *current_word;
+};
+typedef struct parser_state parser_state_t;
+
+static bool
+append_char_to_word (parser_state_t *s, char c, bool allow_null)
+{
+  /*
+   * We ban any chars that are not in the ASCII printable range. If
+   * allow_null == true, we make an exception for NUL. (This is needed so that
+   * append_word_to_list can add a NUL terminator to the word).
+   */
+  if (grub_isprint (c) == false && allow_null == false)
+    return false;
+  else if (allow_null == true && c != '\0')
+    return false;
+
+  if (s->current_word_pos == s->current_word_len)
+    {
+      s->current_word = grub_realloc (s->current_word, s->current_word_len *= 2);
+      if (s->current_word == NULL)
+        {
+          s->current_word_len /= 2;
+          return false;
+        }
+    }
+
+  s->current_word[s->current_word_pos++] = c;
+  return true;
+}
+
+static bool
+append_word_to_list (parser_state_t *s)
+{
+  /* No-op on empty words. */
+  if (s->current_word_pos == 0)
+    return true;
+
+  if (append_char_to_word (s, '\0', true) == false)
+    {
+      grub_error (GRUB_ERR_BUG,
+		  "couldn't append NUL terminator to word during Xen cmdline parsing");
+      grub_print_error ();
+      grub_exit ();
+    }
+
+  s->current_word_len = grub_strlen (s->current_word) + 1;
+  s->current_word = grub_realloc (s->current_word, s->current_word_len);
+  if (s->current_word == NULL)
+    return false;
+  s->word_list = grub_realloc (s->word_list, ++s->word_list_len * sizeof (char *));
+  if (s->word_list == NULL)
+    return false;
+  s->word_list[s->word_list_len - 1] = s->current_word;
+
+  s->current_word_len = PARSER_BASE_WORD_SIZE;
+  s->current_word_pos = 0;
+  s->current_word = grub_malloc (s->current_word_len);
+  if (s->current_word == NULL)
+    return false;
+
+  return true;
+}
+
+static bool
+is_key_safe (char *key, grub_size_t len)
+{
+  grub_size_t i;
+
+  for (i = 0; i < len; i++)
+    {
+      if (! (grub_isalpha (key[i]) || key[i] == '_'))
+        return false;
+    }
+
+  return true;
+}
+
+void
+grub_parse_xen_cmdline (void)
+{
+  parser_state_t *s = NULL;
+
+  const char *cmdline = (const char *) grub_xen_start_page_addr->cmd_line;
+  grub_size_t cmdline_len;
+  bool cmdline_valid = false;
+  char **param_keys = NULL;
+  char **param_vals = NULL;
+  grub_size_t param_dict_len = 0;
+  grub_size_t param_dict_pos = 0;
+  splitter_state_t splitter_state = 0;
+  char current_char = '\0';
+  grub_size_t i = 0;
+
+  s = grub_malloc (sizeof (parser_state_t));
+  if (s == NULL)
+    goto cleanup_final;
+
+  /*
+   * The following algorithm is used to parse the Xen command line:
+   *
+   * - The command line is split into space-separated words.
+   *   - Single and double quotes may be used to suppress the splitting
+   *     behavior of spaces.
+   *   - Double quotes are appended to the current word verbatim if they
+   *     appear within a single-quoted string portion, and vice versa.
+   *   - Backslashes may be used to cause the next character to be
+   *     appended to the current word verbatim. This is only useful when
+   *     used to escape quotes, spaces, and backslashes, but for simplicity
+   *     we allow backslash-escaping anything.
+   * - After splitting the command line into words, each word is checked to
+   *   see if it contains an equals sign.
+   *   - If it does, it is split on the equals sign into a key-value pair. The
+   *     key is then treated as an variable name, and the value is treated as
+   *     the variable's value.
+   *   - If it does not, the entire word is treated as a variable name. The
+   *     variable's value is implicitly considered to be `1`.
+   * - All variables detected on the command line are checked to see if their
+   *   names begin with the string `xen_grub_env_`. Variables that do not pass
+   *   this check are discarded, variables that do pass this check are
+   *   exported so they are available to the GRUB configuration.
+   */
+
+  s->current_word_len = PARSER_BASE_WORD_SIZE;
+  s->current_word = grub_malloc (s->current_word_len);
+  if (s->current_word == NULL)
+    goto cleanup_main;
+
+  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
+    {
+      if (cmdline[i] == '\0')
+        {
+          cmdline_valid = true;
+          break;
+        }
+    }
+
+  if (cmdline_valid == false)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT,
+		  "command line from Xen is not NUL-terminated");
+      grub_print_error ();
+      goto cleanup_main;
+    }
+
+  cmdline_len = grub_strlen (cmdline);
+  for (i = 0; i < cmdline_len; i++)
+    {
+      current_char = cmdline[i];
+
+      /*
+       * If the previous character was a backslash, append the current
+       * character to the word verbatim
+       */
+      if (splitter_state & SPLITTER_HIT_BACKSLASH)
+        {
+          splitter_state &= ~SPLITTER_HIT_BACKSLASH;
+          if (append_char_to_word (s, current_char, false) == false)
+            goto cleanup_main;
+          continue;
+        }
+
+      switch (current_char)
+        {
+        case '\\':
+          /* Backslashes escape arbitrary characters. */
+          splitter_state |= SPLITTER_HIT_BACKSLASH;
+          continue;
+
+        case '\'':
+          /*
+           * Single quotes suppress word splitting and double quoting until
+           * the next single quote is encountered.
+           */
+          if (splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
+            {
+              if (append_char_to_word (s, current_char, false) == false)
+                goto cleanup_main;
+              continue;
+            }
+
+          splitter_state ^= SPLITTER_IN_SINGLE_QUOTES;
+          continue;
+
+        case '"':
+          /*
+           * Double quotes suppress word splitting and single quoting until
+           * the next double quote is encountered.
+           */
+          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES)
+            {
+              if (append_char_to_word (s, current_char, false) == false)
+                goto cleanup_main;
+              continue;
+            }
+
+          splitter_state ^= SPLITTER_IN_DOUBLE_QUOTES;
+          continue;
+
+        case ' ':
+          /* Spaces separate words in the command line from each other. */
+          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES ||
+              splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
+            {
+              if (append_char_to_word (s, current_char, false) == false)
+                goto cleanup_main;
+              continue;
+            }
+
+          if (append_word_to_list (s) == false)
+            goto cleanup_main;
+          continue;
+        }
+
+      if (append_char_to_word (s, current_char, false) == false)
+        goto cleanup_main;
+    }
+
+  if (append_word_to_list (s) == false)
+    goto cleanup_main;
+
+  param_keys = grub_malloc (s->word_list_len * sizeof (char *));
+  if (param_keys == NULL)
+    goto cleanup_main;
+  param_vals = grub_malloc (s->word_list_len * sizeof (char *));
+  if (param_vals == NULL)
+    goto cleanup_main;
+
+  for (i = 0; i < s->word_list_len; i++)
+    {
+      char *current_word_eq_ptr;
+
+      s->current_word = s->word_list[i];
+      s->current_word_len = grub_strlen (s->current_word) + 1;
+      current_word_eq_ptr = grub_strchr (s->current_word, '=');
+
+      if (current_word_eq_ptr != NULL)
+        {
+          /*
+           * Both pre_eq_len and post_eq_len represent substring lengths
+           * without a NUL terminator.
+           */
+          grub_size_t pre_eq_len = (grub_size_t) (current_word_eq_ptr - s->current_word);
+          /*
+           * s->current_word_len includes the NUL terminator, so we subtract
+           * one to get rid of the terminator, and one more to get rid of the
+           * equals sign.
+           */
+          grub_size_t post_eq_len = (s->current_word_len - 2) - pre_eq_len;
+
+          if (is_key_safe (s->current_word, pre_eq_len) == true)
+            {
+              param_dict_pos = param_dict_len++;
+              param_keys[param_dict_pos] = grub_malloc (pre_eq_len + 1);
+              if (param_keys == NULL)
+                goto cleanup_main;
+              param_vals[param_dict_pos] = grub_malloc (post_eq_len + 1);
+              if (param_vals == NULL)
+                goto cleanup_main;
+
+              grub_strncpy (param_keys[param_dict_pos], s->current_word, pre_eq_len);
+              grub_strncpy (param_vals[param_dict_pos],
+			    s->current_word + pre_eq_len + 1, post_eq_len);
+              param_keys[param_dict_pos][pre_eq_len] = '\0';
+              param_vals[param_dict_pos][post_eq_len] = '\0';
+            }
+        }
+      else
+        {
+          if (is_key_safe (s->current_word, s->current_word_len - 1) == true)
+            {
+              param_dict_pos = param_dict_len++;
+              param_keys[param_dict_pos] = grub_malloc (s->current_word_len);
+              if (param_keys == NULL)
+                goto cleanup_main;
+              param_vals[param_dict_pos] = grub_malloc (2);
+              if (param_vals == NULL)
+                goto cleanup_main;
+
+              grub_strncpy (param_keys[param_dict_pos], s->current_word,
+			    s->current_word_len);
+              if (param_keys[param_dict_pos][s->current_word_len - 1] != '\0' )
+                {
+                  grub_error (GRUB_ERR_BUG,
+			      "NUL terminator missing from key during Xen cmdline parsing");
+                  grub_print_error ();
+                  grub_exit ();
+                }
+              grub_strcpy (param_vals[param_dict_pos], "1");
+            }
+        }
+    }
+
+  for (i = 0; i < param_dict_len; i++)
+    {
+      /*
+       * Find keys that start with "xen_grub_env_" and export them
+       * as environment variables.
+       */
+      if (grub_strncmp (param_keys[i],
+			"xen_grub_env_",
+			sizeof ("xen_grub_env_") - 1) != 0)
+        continue;
+
+      if (grub_env_set (param_keys[i], param_vals[i]) != GRUB_ERR_NONE)
+        {
+          grub_printf ("warning: could not set environment variable `%s' to value `%s'\n",
+		       param_keys[i], param_vals[i]);
+          continue;
+        }
+
+      if (grub_env_export (param_keys[i]) != GRUB_ERR_NONE)
+        grub_printf ("warning: could not export environment variable `%s'",
+		     param_keys[i]);
+    }
+
+ cleanup_main:
+  for (i = 0; i < s->word_list_len; i++)
+    grub_free (s->word_list[i]);
+
+  for (i = 0; i < param_dict_len; i++)
+    {
+      grub_free (param_keys[i]);
+      grub_free (param_vals[i]);
+    }
+
+  grub_free (param_keys);
+  grub_free (param_vals);
+  grub_free (s->word_list);
+
+ cleanup_final:
+  grub_free (s);
+}
diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
index 782ca72..69cf59f 100644
--- a/grub-core/kern/xen/init.c
+++ b/grub-core/kern/xen/init.c
@@ -581,6 +581,8 @@ grub_machine_init (void)
   grub_xendisk_init ();
 
   grub_boot_init ();
+
+  grub_parse_xen_cmdline ();
 }
 
 void
diff --git a/include/grub/xen.h b/include/grub/xen.h
index 91cb7cf..7f9efee 100644
--- a/include/grub/xen.h
+++ b/include/grub/xen.h
@@ -89,6 +89,8 @@ void grub_console_init (void);
 void grub_xendisk_fini (void);
 void grub_xendisk_init (void);
 
+void grub_parse_xen_cmdline (void);
+
 #ifdef __x86_64__
 typedef grub_uint64_t grub_xen_mfn_t;
 #else
-- 
2.50.1


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 1677 bytes --]

On Mon, 4 Aug 2025 23:50:09 -0500
Aaron Rainbolt <arraybolt3@gmail.com> wrote:

> Xen traditionally allows customizing guest behavior by passing
> arguments to the VM kernel via the kernel command line. This is no
> longer possible when using GRUB with Xen, as the kernel command line
> is decided by the GRUB configuration file within the guest, not data
> passed to the guest by Xen.
> 
> To work around this limitation, enable GRUB to parse a command line
> passed to it by Xen, and expose data from the command line to the GRUB
> configuration as environment variables. These variables can be used in
> the GRUB configuration for any desired purpose, such as extending the
> kernel command line passed to the guest. The command line format is
> inspired by the Linux kernel's command line format.
> 
> To reduce the risk of misuse, abuse, or accidents in production, the
> command line will only be parsed if it consists entirely of 7-bit
> ASCII characters, only alphabetical characters and underscores are
> permitted in variable names, and all variable names must start with
> the string "xen_grub_env_". This also allows room for expanding the
> command line arguments accepted by GRUB in the future, should other
> arguments end up becoming desirable in the future.

Darn. I sent this patch as a reply to the previous patch in the stack,
not as a reply to the cover letter. That's probably not the right way
to do things.

I should just set up git-send-email and stop trying to make Claws Mail
barely work for this kind of thing... Will resend the patch soon-ish,
this time formatted right (unless that would just cause needless noise).

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Daniel Kiper]

On Mon, Aug 04, 2025 at 11:50:09PM -0500, Aaron Rainbolt wrote:
> Xen traditionally allows customizing guest behavior by passing arguments
> to the VM kernel via the kernel command line. This is no longer possible
> when using GRUB with Xen, as the kernel command line is decided by the
> GRUB configuration file within the guest, not data passed to the guest
> by Xen.
>
> To work around this limitation, enable GRUB to parse a command line
> passed to it by Xen, and expose data from the command line to the GRUB
> configuration as environment variables. These variables can be used in
> the GRUB configuration for any desired purpose, such as extending the
> kernel command line passed to the guest. The command line format is
> inspired by the Linux kernel's command line format.
>
> To reduce the risk of misuse, abuse, or accidents in production, the
> command line will only be parsed if it consists entirely of 7-bit ASCII
> characters, only alphabetical characters and underscores are permitted
> in variable names, and all variable names must start with the string
> "xen_grub_env_". This also allows room for expanding the command line
> arguments accepted by GRUB in the future, should other arguments end up
> becoming desirable in the future.
>
> Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
> ---
>  docs/grub.texi                |  51 +++++
>  grub-core/Makefile.core.def   |   2 +
>  grub-core/kern/i386/xen/pvh.c |  23 +++
>  grub-core/kern/xen/cmdline.c  | 376 ++++++++++++++++++++++++++++++++++
>  grub-core/kern/xen/init.c     |   2 +
>  include/grub/xen.h            |   2 +
>  6 files changed, 456 insertions(+)
>  create mode 100644 grub-core/kern/xen/cmdline.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 34b3484..b58cf98 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3271,6 +3271,7 @@ GRUB.  Others may be used freely in GRUB configuration files.
>  @menu
>  * Special environment variables::
>  * Environment block::
> +* Passing environment variables through Xen::
>  @end menu
>
>
> @@ -3871,6 +3872,56 @@ using BIOS or EFI functions (no ATA, USB or IEEE1275).
>  @command{grub-mkconfig} uses this facility to implement
>  @samp{GRUB_SAVEDEFAULT} (@pxref{Simple configuration}).
>
> +@node Passing environment variables through Xen
> +@section Passing environment variables through Xen
> +
> +If you are using a GRUB image as the kernel for a PV or PVH Xen virtual
> +machine, you can pass environment variables from Xen's dom0 to the VM through
> +the Xen-provided kernel command line. When combined with a properly configured
> +guest, this can be used to customize the guest's behavior on bootup via the
> +VM's Xen configuration file.
> +
> +GRUB will parse the kernel command line passed to it by Xen during bootup.
> +The command line will be split into space-delimited words. Single and
> +double quotes may be used to quote words or portions of words that contain
> +spaces. Single quotes will be considered part of a word if inside double
> +quotes, and vice versa. Arbitrary characters may be backslash-escaped to make
> +them a literal component of a word rather than being parsed as quotes or word
> +separators. The command line must consist entirely of printable 7-bit ASCII
> +characters and spaces. If a non-printing ASCII character is found anywhere in
> +the command line, the entire command line will be ignored by GRUB.
> +
> +Each word should be a variable assignment in the format ``variable'' or
> +``variable=value''. Variable names must contain only the characters A-Z, a-z,
> +and underscore (``_''). Variable names must begin with the string
> +``xen_grub_env_''. Variable values can contain arbitrary printable 7-bit
> +ASCII characters and space. If any variable contains an illegal name, that
> +variable will be ignored.
> +
> +If a variable name and value are both specified, the variable will be set to
> +the specified value. If only a variable name is specified, the variable's
> +value will be set to ``1''.
> +
> +The following is a simple example of how to use this functionality to append
> +arbitrary variables to a guest's kernel command line:
> +
> +@example
> +# In the Xen configuration file for the guest
> +name = "linux_vm"
> +type = "pvh"
> +kernel = "/path/to/grub-i386-xen_pvh.bin"
> +extra = "xen_grub_env_linux_append='loglevel=3'"
> +memory = 1024
> +disk = [ "file:/srv/vms/linux_vm.img,sda,w" ]
> +
> +# In the guest's GRUB configuration file
> +menuentry "Linux VM with dom0-specified kernel parameters" @{
> +    search --set=root --label linux_vm --hint hd0,msdos1
> +    linux /boot/vmlinuz root=LABEL=linux_vm $@{xen_grub_env_linux_append@}
> +    initrd /boot/initrd.img
> +@}
> +@end example
> +
>  @node Modules
>  @chapter Modules
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index b3f7119..df0f266 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -248,6 +248,7 @@ kernel = {
>    xen = term/xen/console.c;
>    xen = disk/xen/xendisk.c;
>    xen = commands/boot.c;
> +  xen = kern/xen/cmdline.c;
>
>    i386_xen_pvh = commands/boot.c;
>    i386_xen_pvh = disk/xen/xendisk.c;
> @@ -255,6 +256,7 @@ kernel = {
>    i386_xen_pvh = kern/i386/xen/tsc.c;
>    i386_xen_pvh = kern/i386/xen/pvh.c;
>    i386_xen_pvh = kern/xen/init.c;
> +  i386_xen_pvh = kern/xen/cmdline.c;
>    i386_xen_pvh = term/xen/console.c;
>
>    ia64_efi = kern/ia64/efi/startup.S;
> diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
> index 91fbca8..a8988d2 100644
> --- a/grub-core/kern/i386/xen/pvh.c
> +++ b/grub-core/kern/i386/xen/pvh.c
> @@ -321,6 +321,8 @@ void
>  grub_xen_setup_pvh (void)
>  {
>    grub_addr_t par;
> +  const char *xen_cmdline;
> +  int i;
>
>    grub_xen_cpuid_base ();
>    grub_xen_setup_hypercall_page ();
> @@ -352,6 +354,27 @@ grub_xen_setup_pvh (void)
>    grub_xen_mm_init_regions ();
>
>    grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> +
> +  xen_cmdline = (const char *) pvh_start_info->cmdline_paddr;
> +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> +    {
> +      if (xen_cmdline[i] == '\0')

This code still does not make a lot of sense for me. You have NUL check
in grub_parse_xen_cmdline(). So, you duplicate the check here...

I would just fire grub_strncpy() here and forget...

> +        {
> +          grub_strncpy ((char *) grub_xen_start_page_addr->cmd_line,
> +			(char *) pvh_start_info->cmdline_paddr,
> +			GRUB_XEN_MAX_GUEST_CMDLINE);
> +
> +          if (grub_xen_start_page_addr->cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE - 1] != '\0')

If you convince me this code is still needed then I am afraid that this
is not what you meant here...

> +            {
> +              grub_error (GRUB_ERR_BUG,
> +			  "Xen command line is not NUL-terminated");
> +              grub_print_error ();
> +              grub_exit ();

grub_fatal() and you are done...

> +            }
> +
> +          break;
> +        }
> +    }
>  }
>
>  grub_err_t
> diff --git a/grub-core/kern/xen/cmdline.c b/grub-core/kern/xen/cmdline.c
> new file mode 100644
> index 0000000..46a9998
> --- /dev/null
> +++ b/grub-core/kern/xen/cmdline.c
> @@ -0,0 +1,376 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2025  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/env.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/xen.h>
> +
> +enum splitter_state
> +{
> +  SPLITTER_HIT_BACKSLASH = 0x1,
> +  SPLITTER_IN_SINGLE_QUOTES = 0x2,
> +  SPLITTER_IN_DOUBLE_QUOTES = 0x4,
> +};
> +typedef enum splitter_state splitter_state_t;
> +
> +/*
> + * The initial size of the current_word buffer. The buffer may be resized as
> + * needed.
> + */
> +#define PARSER_BASE_WORD_SIZE 32
> +
> +struct parser_state
> +{
> +  grub_size_t word_list_len;

s/word_list_len/words_count/

And I would put it behind word_list...

> +  char **word_list;

s/word_list/words/

> +  grub_size_t current_word_len;
> +  grub_size_t current_word_pos;
> +  char *current_word;
> +};
> +typedef struct parser_state parser_state_t;
> +
> +static bool

s/bool/grub_err_t/

> +append_char_to_word (parser_state_t *s, char c, bool allow_null)
> +{
> +  /*
> +   * We ban any chars that are not in the ASCII printable range. If
> +   * allow_null == true, we make an exception for NUL. (This is needed so that
> +   * append_word_to_list can add a NUL terminator to the word).
> +   */
> +  if (grub_isprint (c) == false && allow_null == false)

grub_isprint() et consortes return int instead of bool. So, it should be
"!grub_isprint(c)" here...

> +    return false;
> +  else if (allow_null == true && c != '\0')
> +    return false;
> +
> +  if (s->current_word_pos == s->current_word_len)
> +    {
> +      s->current_word = grub_realloc (s->current_word, s->current_word_len *= 2);
> +      if (s->current_word == NULL)
> +        {
> +          s->current_word_len /= 2;
> +          return false;
> +        }
> +    }
> +
> +  s->current_word[s->current_word_pos++] = c;
> +  return true;
> +}
> +
> +static bool

s/bool/grub_err_t/

> +append_word_to_list (parser_state_t *s)
> +{
> +  /* No-op on empty words. */
> +  if (s->current_word_pos == 0)
> +    return true;
> +
> +  if (append_char_to_word (s, '\0', true) == false)
> +    {
> +      grub_error (GRUB_ERR_BUG,
> +		  "couldn't append NUL terminator to word during Xen cmdline parsing");
> +      grub_print_error ();
> +      grub_exit ();

grub_fatal()

> +    }
> +
> +  s->current_word_len = grub_strlen (s->current_word) + 1;
> +  s->current_word = grub_realloc (s->current_word, s->current_word_len);
> +  if (s->current_word == NULL)
> +    return false;

return grub_errno;

> +  s->word_list = grub_realloc (s->word_list, ++s->word_list_len * sizeof (char *));
> +  if (s->word_list == NULL)
> +    return false;

return grub_errno;

...

I think many (related) functions in this code returning bool should
really return grub_err_t...

> +  s->word_list[s->word_list_len - 1] = s->current_word;
> +
> +  s->current_word_len = PARSER_BASE_WORD_SIZE;
> +  s->current_word_pos = 0;
> +  s->current_word = grub_malloc (s->current_word_len);
> +  if (s->current_word == NULL)
> +    return false;
> +
> +  return true;
> +}
> +
> +static bool

But this bool makes sense...

> +is_key_safe (char *key, grub_size_t len)
> +{
> +  grub_size_t i;
> +
> +  for (i = 0; i < len; i++)
> +    {
> +      if (! (grub_isalpha (key[i]) || key[i] == '_'))

Please drop space after "!"...

> +        return false;
> +    }

You can drop curly braces from here...

> +
> +  return true;
> +}
> +
> +void
> +grub_parse_xen_cmdline (void)
> +{
> +  parser_state_t *s = NULL;

parser_state_t ps = {0};

... and you do not need grub_malloc(s) and stuff any longer below...

And I would put it next to splitter_state...

> +  const char *cmdline = (const char *) grub_xen_start_page_addr->cmd_line;
> +  grub_size_t cmdline_len;
> +  bool cmdline_valid = false;
> +  char **param_keys = NULL;
> +  char **param_vals = NULL;
> +  grub_size_t param_dict_len = 0;
> +  grub_size_t param_dict_pos = 0;
> +  splitter_state_t splitter_state = 0;

You nicely define an enum and then assign plain number. Sigh...
I think you should define SPLITTER_NORMAL or something similar
as well...

And s/splitter_state/ss/...

> +  char current_char = '\0';
> +  grub_size_t i = 0;
> +
> +  s = grub_malloc (sizeof (parser_state_t));
> +  if (s == NULL)
> +    goto cleanup_final;
> +
> +  /*
> +   * The following algorithm is used to parse the Xen command line:
> +   *
> +   * - The command line is split into space-separated words.
> +   *   - Single and double quotes may be used to suppress the splitting
> +   *     behavior of spaces.
> +   *   - Double quotes are appended to the current word verbatim if they
> +   *     appear within a single-quoted string portion, and vice versa.
> +   *   - Backslashes may be used to cause the next character to be
> +   *     appended to the current word verbatim. This is only useful when
> +   *     used to escape quotes, spaces, and backslashes, but for simplicity
> +   *     we allow backslash-escaping anything.
> +   * - After splitting the command line into words, each word is checked to
> +   *   see if it contains an equals sign.
> +   *   - If it does, it is split on the equals sign into a key-value pair. The
> +   *     key is then treated as an variable name, and the value is treated as
> +   *     the variable's value.
> +   *   - If it does not, the entire word is treated as a variable name. The
> +   *     variable's value is implicitly considered to be `1`.
> +   * - All variables detected on the command line are checked to see if their
> +   *   names begin with the string `xen_grub_env_`. Variables that do not pass
> +   *   this check are discarded, variables that do pass this check are
> +   *   exported so they are available to the GRUB configuration.
> +   */
> +
> +  s->current_word_len = PARSER_BASE_WORD_SIZE;
> +  s->current_word = grub_malloc (s->current_word_len);
> +  if (s->current_word == NULL)
> +    goto cleanup_main;
> +
> +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> +    {
> +      if (cmdline[i] == '\0')
> +        {
> +          cmdline_valid = true;
> +          break;
> +        }
> +    }
> +
> +  if (cmdline_valid == false)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		  "command line from Xen is not NUL-terminated");
> +      grub_print_error ();

grub_fatal()?

> +      goto cleanup_main;
> +    }
> +
> +  cmdline_len = grub_strlen (cmdline);
> +  for (i = 0; i < cmdline_len; i++)
> +    {
> +      current_char = cmdline[i];
> +
> +      /*
> +       * If the previous character was a backslash, append the current
> +       * character to the word verbatim
> +       */
> +      if (splitter_state & SPLITTER_HIT_BACKSLASH)
> +        {
> +          splitter_state &= ~SPLITTER_HIT_BACKSLASH;
> +          if (append_char_to_word (s, current_char, false) == false)
> +            goto cleanup_main;
> +          continue;
> +        }
> +
> +      switch (current_char)
> +        {
> +        case '\\':
> +          /* Backslashes escape arbitrary characters. */
> +          splitter_state |= SPLITTER_HIT_BACKSLASH;
> +          continue;
> +
> +        case '\'':
> +          /*
> +           * Single quotes suppress word splitting and double quoting until
> +           * the next single quote is encountered.
> +           */
> +          if (splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> +            {
> +              if (append_char_to_word (s, current_char, false) == false)
> +                goto cleanup_main;
> +              continue;
> +            }
> +
> +          splitter_state ^= SPLITTER_IN_SINGLE_QUOTES;
> +          continue;
> +
> +        case '"':
> +          /*
> +           * Double quotes suppress word splitting and single quoting until
> +           * the next double quote is encountered.
> +           */
> +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES)
> +            {
> +              if (append_char_to_word (s, current_char, false) == false)
> +                goto cleanup_main;
> +              continue;
> +            }
> +
> +          splitter_state ^= SPLITTER_IN_DOUBLE_QUOTES;
> +          continue;
> +
> +        case ' ':
> +          /* Spaces separate words in the command line from each other. */
> +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES ||
> +              splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> +            {
> +              if (append_char_to_word (s, current_char, false) == false)
> +                goto cleanup_main;
> +              continue;
> +            }
> +
> +          if (append_word_to_list (s) == false)
> +            goto cleanup_main;

I think this is not fully correct. You should not run append_word_to_list()
until the closing quote. So, here you should have "else" for the first "if",
i.e., "if (splitter_state & ..." and call append_word_to_list() for closing
\" and \' above.

> +          continue;
> +        }
> +
> +      if (append_char_to_word (s, current_char, false) == false)
> +        goto cleanup_main;

This should be part of "default:" for the switch above... Even if it
works now...

Then many "continue" should be converted to more natural "break"...

> +    }
> +
> +  if (append_word_to_list (s) == false)
> +    goto cleanup_main;
> +
> +  param_keys = grub_malloc (s->word_list_len * sizeof (char *));
> +  if (param_keys == NULL)
> +    goto cleanup_main;
> +  param_vals = grub_malloc (s->word_list_len * sizeof (char *));
> +  if (param_vals == NULL)
> +    goto cleanup_main;
> +
> +  for (i = 0; i < s->word_list_len; i++)
> +    {
> +      char *current_word_eq_ptr;

s/current_word_eq_ptr/eq_pos/

> +      s->current_word = s->word_list[i];
> +      s->current_word_len = grub_strlen (s->current_word) + 1;
> +      current_word_eq_ptr = grub_strchr (s->current_word, '=');
> +
> +      if (current_word_eq_ptr != NULL)
> +        {
> +          /*
> +           * Both pre_eq_len and post_eq_len represent substring lengths
> +           * without a NUL terminator.
> +           */
> +          grub_size_t pre_eq_len = (grub_size_t) (current_word_eq_ptr - s->current_word);
> +          /*
> +           * s->current_word_len includes the NUL terminator, so we subtract
> +           * one to get rid of the terminator, and one more to get rid of the
> +           * equals sign.
> +           */
> +          grub_size_t post_eq_len = (s->current_word_len - 2) - pre_eq_len;
> +
> +          if (is_key_safe (s->current_word, pre_eq_len) == true)
> +            {
> +              param_dict_pos = param_dict_len++;
> +              param_keys[param_dict_pos] = grub_malloc (pre_eq_len + 1);
> +              if (param_keys == NULL)
> +                goto cleanup_main;
> +              param_vals[param_dict_pos] = grub_malloc (post_eq_len + 1);
> +              if (param_vals == NULL)
> +                goto cleanup_main;
> +
> +              grub_strncpy (param_keys[param_dict_pos], s->current_word, pre_eq_len);
> +              grub_strncpy (param_vals[param_dict_pos],
> +			    s->current_word + pre_eq_len + 1, post_eq_len);
> +              param_keys[param_dict_pos][pre_eq_len] = '\0';
> +              param_vals[param_dict_pos][post_eq_len] = '\0';
> +            }
> +        }
> +      else

else if (is_key_safe (s->current_word, s->current_word_len - 1) == true)

... and you can drop an extra indention...

> +        {
> +          if (is_key_safe (s->current_word, s->current_word_len - 1) == true)
> +            {
> +              param_dict_pos = param_dict_len++;
> +              param_keys[param_dict_pos] = grub_malloc (s->current_word_len);
> +              if (param_keys == NULL)
> +                goto cleanup_main;
> +              param_vals[param_dict_pos] = grub_malloc (2);
> +              if (param_vals == NULL)
> +                goto cleanup_main;
> +
> +              grub_strncpy (param_keys[param_dict_pos], s->current_word,
> +			    s->current_word_len);
> +              if (param_keys[param_dict_pos][s->current_word_len - 1] != '\0' )
> +                {
> +                  grub_error (GRUB_ERR_BUG,
> +			      "NUL terminator missing from key during Xen cmdline parsing");
> +                  grub_print_error ();
> +                  grub_exit ();

grub_fatal()

> +                }
> +              grub_strcpy (param_vals[param_dict_pos], "1");
> +            }
> +        }
> +    }

Daniel

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

Re: [PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 27913 bytes --]

On Tue, 12 Aug 2025 19:02:11 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Aug 04, 2025 at 11:50:09PM -0500, Aaron Rainbolt wrote:
> > Xen traditionally allows customizing guest behavior by passing
> > arguments to the VM kernel via the kernel command line. This is no
> > longer possible when using GRUB with Xen, as the kernel command
> > line is decided by the GRUB configuration file within the guest,
> > not data passed to the guest by Xen.
> >
> > To work around this limitation, enable GRUB to parse a command line
> > passed to it by Xen, and expose data from the command line to the
> > GRUB configuration as environment variables. These variables can be
> > used in the GRUB configuration for any desired purpose, such as
> > extending the kernel command line passed to the guest. The command
> > line format is inspired by the Linux kernel's command line format.
> >
> > To reduce the risk of misuse, abuse, or accidents in production, the
> > command line will only be parsed if it consists entirely of 7-bit
> > ASCII characters, only alphabetical characters and underscores are
> > permitted in variable names, and all variable names must start with
> > the string "xen_grub_env_". This also allows room for expanding the
> > command line arguments accepted by GRUB in the future, should other
> > arguments end up becoming desirable in the future.
> >
> > Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
> > ---
> >  docs/grub.texi                |  51 +++++
> >  grub-core/Makefile.core.def   |   2 +
> >  grub-core/kern/i386/xen/pvh.c |  23 +++
> >  grub-core/kern/xen/cmdline.c  | 376
> > ++++++++++++++++++++++++++++++++++ grub-core/kern/xen/init.c     |
> >  2 + include/grub/xen.h            |   2 +
> >  6 files changed, 456 insertions(+)
> >  create mode 100644 grub-core/kern/xen/cmdline.c
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 34b3484..b58cf98 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -3271,6 +3271,7 @@ GRUB.  Others may be used freely in GRUB
> > configuration files. @menu
> >  * Special environment variables::
> >  * Environment block::
> > +* Passing environment variables through Xen::
> >  @end menu
> >
> >
> > @@ -3871,6 +3872,56 @@ using BIOS or EFI functions (no ATA, USB or
> > IEEE1275). @command{grub-mkconfig} uses this facility to implement
> >  @samp{GRUB_SAVEDEFAULT} (@pxref{Simple configuration}).
> >
> > +@node Passing environment variables through Xen
> > +@section Passing environment variables through Xen
> > +
> > +If you are using a GRUB image as the kernel for a PV or PVH Xen
> > virtual +machine, you can pass environment variables from Xen's
> > dom0 to the VM through +the Xen-provided kernel command line. When
> > combined with a properly configured +guest, this can be used to
> > customize the guest's behavior on bootup via the +VM's Xen
> > configuration file. +
> > +GRUB will parse the kernel command line passed to it by Xen during
> > bootup. +The command line will be split into space-delimited words.
> > Single and +double quotes may be used to quote words or portions of
> > words that contain +spaces. Single quotes will be considered part
> > of a word if inside double +quotes, and vice versa. Arbitrary
> > characters may be backslash-escaped to make +them a literal
> > component of a word rather than being parsed as quotes or word
> > +separators. The command line must consist entirely of printable
> > 7-bit ASCII +characters and spaces. If a non-printing ASCII
> > character is found anywhere in +the command line, the entire
> > command line will be ignored by GRUB. + +Each word should be a
> > variable assignment in the format ``variable'' or
> > +``variable=value''. Variable names must contain only the
> > characters A-Z, a-z, +and underscore (``_''). Variable names must
> > begin with the string +``xen_grub_env_''. Variable values can
> > contain arbitrary printable 7-bit +ASCII characters and space. If
> > any variable contains an illegal name, that +variable will be
> > ignored. + +If a variable name and value are both specified, the
> > variable will be set to +the specified value. If only a variable
> > name is specified, the variable's +value will be set to ``1''.
> > +
> > +The following is a simple example of how to use this functionality
> > to append +arbitrary variables to a guest's kernel command line:
> > +
> > +@example
> > +# In the Xen configuration file for the guest
> > +name = "linux_vm"
> > +type = "pvh"
> > +kernel = "/path/to/grub-i386-xen_pvh.bin"
> > +extra = "xen_grub_env_linux_append='loglevel=3'"
> > +memory = 1024
> > +disk = [ "file:/srv/vms/linux_vm.img,sda,w" ]
> > +
> > +# In the guest's GRUB configuration file
> > +menuentry "Linux VM with dom0-specified kernel parameters" @{
> > +    search --set=root --label linux_vm --hint hd0,msdos1
> > +    linux /boot/vmlinuz root=LABEL=linux_vm
> > $@{xen_grub_env_linux_append@}
> > +    initrd /boot/initrd.img
> > +@}
> > +@end example
> > +
> >  @node Modules
> >  @chapter Modules
> >
> > diff --git a/grub-core/Makefile.core.def
> > b/grub-core/Makefile.core.def index b3f7119..df0f266 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -248,6 +248,7 @@ kernel = {
> >    xen = term/xen/console.c;
> >    xen = disk/xen/xendisk.c;
> >    xen = commands/boot.c;
> > +  xen = kern/xen/cmdline.c;
> >
> >    i386_xen_pvh = commands/boot.c;
> >    i386_xen_pvh = disk/xen/xendisk.c;
> > @@ -255,6 +256,7 @@ kernel = {
> >    i386_xen_pvh = kern/i386/xen/tsc.c;
> >    i386_xen_pvh = kern/i386/xen/pvh.c;
> >    i386_xen_pvh = kern/xen/init.c;
> > +  i386_xen_pvh = kern/xen/cmdline.c;
> >    i386_xen_pvh = term/xen/console.c;
> >
> >    ia64_efi = kern/ia64/efi/startup.S;
> > diff --git a/grub-core/kern/i386/xen/pvh.c
> > b/grub-core/kern/i386/xen/pvh.c index 91fbca8..a8988d2 100644
> > --- a/grub-core/kern/i386/xen/pvh.c
> > +++ b/grub-core/kern/i386/xen/pvh.c
> > @@ -321,6 +321,8 @@ void
> >  grub_xen_setup_pvh (void)
> >  {
> >    grub_addr_t par;
> > +  const char *xen_cmdline;
> > +  int i;
> >
> >    grub_xen_cpuid_base ();
> >    grub_xen_setup_hypercall_page ();
> > @@ -352,6 +354,27 @@ grub_xen_setup_pvh (void)
> >    grub_xen_mm_init_regions ();
> >
> >    grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> > +
> > +  xen_cmdline = (const char *) pvh_start_info->cmdline_paddr;
> > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > +    {
> > +      if (xen_cmdline[i] == '\0')  
> 
> This code still does not make a lot of sense for me. You have NUL
> check in grub_parse_xen_cmdline(). So, you duplicate the check here...
> 
> I would just fire grub_strncpy() here and forget...

I guess it depends on how you view grub_xen_start_page_addr->cmd_line's
semantics. In my mind, cmd_line is a NUL-terminated string, always. If
you boot in PV mode, Xen ensures it's a NUL-terminated string, so it's
reasonable for code in GRUB to assume it will be one. If you boot in
PVH mode, it starts out initialized to all zeros which is technically a
NUL-terminated string, and the code that exists here ensures that if we
copy the kernel command line to cmd_line, it will still be a
NUL-terminated string. If we use a "bare" grub_strncpy() here, then if
someone passes a kernel command line larger than
GRUB_XEN_MAX_GUEST_CMDLINE - 1, cmd_line will end up not being
NUL-terminated anymore, and any code added to GRUB in the future that
assumes it is a NUL-terminated string may buffer overflow.

One could argue "let's keep the NUL check here and remove it from
grub_parse_xen_cmdline()", but that doesn't work either because we only
get to control the contents of cmd_line if we boot in PVH mode. If
instead we boot in PV mode, cmd_line is initialized by Xen itself. GRUB
receives a pre-populated and ready-to-use start_info struct directly
from the hypervisor in this scenario. Xen is supposed to ensure that
start_info is always NUL-terminated, but if there's ever a bug in Xen
that breaks that assumption, that could result in bad things happening,
the same as if we didn't do the NUL check in grub_xen_setup_pvh(). Now
of course there's nothing we can do about Xen possibly being buggy (if
it gives us a GRUB_XEN_MAX_GUEST_CMDLINE-long buffer with no NUL
terminator, too bad), but we can at least make sure that we're ready
for that eventuality. That's why I like having both NUL checks - we're
ready for if Xen does things wrong, and any future code that isn't
ready for that eventuality will still work if things go wrong, at least
in PVH mode.

(Arguably any new code that depends on cmd_line *should* check it for a
NUL terminator. But I don't want to assume that all new code *will* do
so.)

> > +        {
> > +          grub_strncpy ((char *)
> > grub_xen_start_page_addr->cmd_line,
> > +			(char *) pvh_start_info->cmdline_paddr,
> > +			GRUB_XEN_MAX_GUEST_CMDLINE);
> > +
> > +          if
> > (grub_xen_start_page_addr->cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE - 1]
> > != '\0')  
> 
> If you convince me this code is still needed then I am afraid that
> this is not what you meant here...

grub_strncpy doesn't just NUL-terminate the command line, it also fills
the entire remainder of the buffer with NUL characters. Therefore if
grub_strncpy NUL-terminated the copied string, the last character of
the buffer will always be NUL. This is a redundant check since the code
above goes to great lengths to avoid ever putting anything
non-NUL-terminated into the buffer, but I was under the impression you
wanted the check added just in case. I'm happy to remove it if it's not
desirable.

> > +            {
> > +              grub_error (GRUB_ERR_BUG,
> > +			  "Xen command line is not
> > NUL-terminated");
> > +              grub_print_error ();
> > +              grub_exit ();  
> 
> grub_fatal() and you are done...

Will change.

> > +            }
> > +
> > +          break;
> > +        }
> > +    }
> >  }
> >
> >  grub_err_t
> > diff --git a/grub-core/kern/xen/cmdline.c
> > b/grub-core/kern/xen/cmdline.c new file mode 100644
> > index 0000000..46a9998
> > --- /dev/null
> > +++ b/grub-core/kern/xen/cmdline.c
> > @@ -0,0 +1,376 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2025  Free Software Foundation, Inc.
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as
> > published by
> > + *  the Free Software Foundation, either version 3 of the License,
> > or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public
> > License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/env.h>
> > +#include <grub/misc.h>
> > +#include <grub/mm.h>
> > +#include <grub/xen.h>
> > +
> > +enum splitter_state
> > +{
> > +  SPLITTER_HIT_BACKSLASH = 0x1,
> > +  SPLITTER_IN_SINGLE_QUOTES = 0x2,
> > +  SPLITTER_IN_DOUBLE_QUOTES = 0x4,
> > +};
> > +typedef enum splitter_state splitter_state_t;
> > +
> > +/*
> > + * The initial size of the current_word buffer. The buffer may be
> > resized as
> > + * needed.
> > + */
> > +#define PARSER_BASE_WORD_SIZE 32
> > +
> > +struct parser_state
> > +{
> > +  grub_size_t word_list_len;  
> 
> s/word_list_len/words_count/
> 
> And I would put it behind word_list...

Will change.

> > +  char **word_list;  
> 
> s/word_list/words/

Will change.

> > +  grub_size_t current_word_len;
> > +  grub_size_t current_word_pos;
> > +  char *current_word;
> > +};
> > +typedef struct parser_state parser_state_t;
> > +
> > +static bool  
> 
> s/bool/grub_err_t/

Will change.

> > +append_char_to_word (parser_state_t *s, char c, bool allow_null)
> > +{
> > +  /*
> > +   * We ban any chars that are not in the ASCII printable range. If
> > +   * allow_null == true, we make an exception for NUL. (This is
> > needed so that
> > +   * append_word_to_list can add a NUL terminator to the word).
> > +   */
> > +  if (grub_isprint (c) == false && allow_null == false)  
> 
> grub_isprint() et consortes return int instead of bool. So, it should
> be "!grub_isprint(c)" here...

Argh, right, because I can't assume that false == 0 and true ==
non-zero. Will fix.

> > +    return false;
> > +  else if (allow_null == true && c != '\0')
> > +    return false;
> > +
> > +  if (s->current_word_pos == s->current_word_len)
> > +    {
> > +      s->current_word = grub_realloc (s->current_word,
> > s->current_word_len *= 2);
> > +      if (s->current_word == NULL)
> > +        {
> > +          s->current_word_len /= 2;
> > +          return false;
> > +        }
> > +    }
> > +
> > +  s->current_word[s->current_word_pos++] = c;
> > +  return true;
> > +}
> > +
> > +static bool  
> 
> s/bool/grub_err_t/

Will change.

> > +append_word_to_list (parser_state_t *s)
> > +{
> > +  /* No-op on empty words. */
> > +  if (s->current_word_pos == 0)
> > +    return true;
> > +
> > +  if (append_char_to_word (s, '\0', true) == false)
> > +    {
> > +      grub_error (GRUB_ERR_BUG,
> > +		  "couldn't append NUL terminator to word during
> > Xen cmdline parsing");
> > +      grub_print_error ();
> > +      grub_exit ();  
> 
> grub_fatal()

Will change.

> > +    }
> > +
> > +  s->current_word_len = grub_strlen (s->current_word) + 1;
> > +  s->current_word = grub_realloc (s->current_word,
> > s->current_word_len);
> > +  if (s->current_word == NULL)
> > +    return false;  
> 
> return grub_errno;

Will change.

> > +  s->word_list = grub_realloc (s->word_list, ++s->word_list_len *
> > sizeof (char *));
> > +  if (s->word_list == NULL)
> > +    return false;  
> 
> return grub_errno;
> 
> ...
> 
> I think many (related) functions in this code returning bool should
> really return grub_err_t...

That makes sense to me, will change where appropriate.

> > +  s->word_list[s->word_list_len - 1] = s->current_word;
> > +
> > +  s->current_word_len = PARSER_BASE_WORD_SIZE;
> > +  s->current_word_pos = 0;
> > +  s->current_word = grub_malloc (s->current_word_len);
> > +  if (s->current_word == NULL)
> > +    return false;
> > +
> > +  return true;
> > +}
> > +
> > +static bool  
> 
> But this bool makes sense...

OK.

> > +is_key_safe (char *key, grub_size_t len)
> > +{
> > +  grub_size_t i;
> > +
> > +  for (i = 0; i < len; i++)
> > +    {
> > +      if (! (grub_isalpha (key[i]) || key[i] == '_'))  
> 
> Please drop space after "!"...

Will change.

> > +        return false;
> > +    }  
> 
> You can drop curly braces from here...

Will change.

(I initially kept this written the way it was though because the GNU
coding standards seemed to indicate I should keep the braces. "When
you have an if-else statement nested in another if statement, always
put braces around the if-else." [1] This isn't a nested if, but it's an
if within a for which is very similar.)

Is there any recommended coding style documentation I should be looking
at other than the GNU coding standards and the GRUB coding style
guidelines? It seems I'm making an awful lot of style mistakes and
would like to avoid that going forward.

> > +
> > +  return true;
> > +}
> > +
> > +void
> > +grub_parse_xen_cmdline (void)
> > +{
> > +  parser_state_t *s = NULL;  
> 
> parser_state_t ps = {0};
> 
> ... and you do not need grub_malloc(s) and stuff any longer below...
> 
> And I would put it next to splitter_state...

Will change.

> > +  const char *cmdline = (const char *)
> > grub_xen_start_page_addr->cmd_line;
> > +  grub_size_t cmdline_len;
> > +  bool cmdline_valid = false;
> > +  char **param_keys = NULL;
> > +  char **param_vals = NULL;
> > +  grub_size_t param_dict_len = 0;
> > +  grub_size_t param_dict_pos = 0;
> > +  splitter_state_t splitter_state = 0;  
> 
> You nicely define an enum and then assign plain number. Sigh...
> I think you should define SPLITTER_NORMAL or something similar
> as well...
>
> And s/splitter_state/ss/...

Will implement both changes.

> > +  char current_char = '\0';
> > +  grub_size_t i = 0;
> > +
> > +  s = grub_malloc (sizeof (parser_state_t));
> > +  if (s == NULL)
> > +    goto cleanup_final;
> > +
> > +  /*
> > +   * The following algorithm is used to parse the Xen command line:
> > +   *
> > +   * - The command line is split into space-separated words.
> > +   *   - Single and double quotes may be used to suppress the
> > splitting
> > +   *     behavior of spaces.
> > +   *   - Double quotes are appended to the current word verbatim
> > if they
> > +   *     appear within a single-quoted string portion, and vice
> > versa.
> > +   *   - Backslashes may be used to cause the next character to be
> > +   *     appended to the current word verbatim. This is only
> > useful when
> > +   *     used to escape quotes, spaces, and backslashes, but for
> > simplicity
> > +   *     we allow backslash-escaping anything.
> > +   * - After splitting the command line into words, each word is
> > checked to
> > +   *   see if it contains an equals sign.
> > +   *   - If it does, it is split on the equals sign into a
> > key-value pair. The
> > +   *     key is then treated as an variable name, and the value is
> > treated as
> > +   *     the variable's value.
> > +   *   - If it does not, the entire word is treated as a variable
> > name. The
> > +   *     variable's value is implicitly considered to be `1`.
> > +   * - All variables detected on the command line are checked to
> > see if their
> > +   *   names begin with the string `xen_grub_env_`. Variables that
> > do not pass
> > +   *   this check are discarded, variables that do pass this check
> > are
> > +   *   exported so they are available to the GRUB configuration.
> > +   */
> > +
> > +  s->current_word_len = PARSER_BASE_WORD_SIZE;
> > +  s->current_word = grub_malloc (s->current_word_len);
> > +  if (s->current_word == NULL)
> > +    goto cleanup_main;
> > +
> > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > +    {
> > +      if (cmdline[i] == '\0')
> > +        {
> > +          cmdline_valid = true;
> > +          break;
> > +        }
> > +    }
> > +
> > +  if (cmdline_valid == false)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +		  "command line from Xen is not NUL-terminated");
> > +      grub_print_error ();  
> 
> grub_fatal()?

That would probably be a bad idea here. We use (or, at least, will use)
grub_fatal() in grub_xen_setup_pvh() because if grub_strncpy() doesn't
NUL-terminate the string it copies, it indicates a bug in GRUB (either
in grub_strncpy, or more likely in the nearby NUL-checking code). On
the other hand, cmdline_valid may equal false if we boot in PV mode and
Xen incorrectly hands us a non-NUL-terminated string. That's bad and
prevents us from parsing the command line safely, but I'd argue it
shouldn't entirely block boot. (Then again, maybe if it does block
boot, that will make this kind of theoretical Xen bug be much more
noticeable and help it get fixed quicker. If you'd prefer that we bail
out entirely here, I'm happy to change it.)

> > +      goto cleanup_main;
> > +    }
> > +
> > +  cmdline_len = grub_strlen (cmdline);
> > +  for (i = 0; i < cmdline_len; i++)
> > +    {
> > +      current_char = cmdline[i];
> > +
> > +      /*
> > +       * If the previous character was a backslash, append the
> > current
> > +       * character to the word verbatim
> > +       */
> > +      if (splitter_state & SPLITTER_HIT_BACKSLASH)
> > +        {
> > +          splitter_state &= ~SPLITTER_HIT_BACKSLASH;
> > +          if (append_char_to_word (s, current_char, false) ==
> > false)
> > +            goto cleanup_main;
> > +          continue;
> > +        }
> > +
> > +      switch (current_char)
> > +        {
> > +        case '\\':
> > +          /* Backslashes escape arbitrary characters. */
> > +          splitter_state |= SPLITTER_HIT_BACKSLASH;
> > +          continue;
> > +
> > +        case '\'':
> > +          /*
> > +           * Single quotes suppress word splitting and double
> > quoting until
> > +           * the next single quote is encountered.
> > +           */
> > +          if (splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > +            {
> > +              if (append_char_to_word (s, current_char, false) ==
> > false)
> > +                goto cleanup_main;
> > +              continue;
> > +            }
> > +
> > +          splitter_state ^= SPLITTER_IN_SINGLE_QUOTES;
> > +          continue;
> > +
> > +        case '"':
> > +          /*
> > +           * Double quotes suppress word splitting and single
> > quoting until
> > +           * the next double quote is encountered.
> > +           */
> > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES)
> > +            {
> > +              if (append_char_to_word (s, current_char, false) ==
> > false)
> > +                goto cleanup_main;
> > +              continue;
> > +            }
> > +
> > +          splitter_state ^= SPLITTER_IN_DOUBLE_QUOTES;
> > +          continue;
> > +
> > +        case ' ':
> > +          /* Spaces separate words in the command line from each
> > other. */
> > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES ||
> > +              splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > +            {
> > +              if (append_char_to_word (s, current_char, false) ==
> > false)
> > +                goto cleanup_main;
> > +              continue;
> > +            }
> > +
> > +          if (append_word_to_list (s) == false)
> > +            goto cleanup_main;  
> 
> I think this is not fully correct. You should not run
> append_word_to_list() until the closing quote. So, here you should
> have "else" for the first "if", i.e., "if (splitter_state & ..." and
> call append_word_to_list() for closing \" and \' above.

What closing quote? If we're in quotes, the `if (splitter_state &...`
block will run. That block runs `continue`, restarting the loop before
we get to the `append_word_to_list (s)` call. If we hit a closing
quote, either the `case '\''` or the `case '"'` block will be
triggered, changing the splitter's state so that it knows it's no
longer within a quote block, and then the next space (or the end of the
string) will cause `append_word_to_list()` to be called, adding the
quoted word to the list. Even if this `continue` changes to a `break`
like suggested below, the logic and control flow will remain the same.

Calling `append_word_to_list()` for closing quotes would break the
splitter. I'm trying to implement splitting behavior similar to Bash or
GRUB's configuration language, both of which parse
`xen_grub_env_var="abc def"ghi` into the variable `xen_grub_env_var`
with the value `abc defghi`. If we split on closing quotes too, the
input above will be parsed into two words, `xen_grub_env_var=abc def`
and an extra word `ghi`, resulting in the environment variable
`xen_grub_env_var` being set to `abc def` and the `ghi` being lost
entirely. One could argue that this is reasonable behavior, but it
isn't consistent with the other splitting behavior in GRUB.

> > +          continue;
> > +        }
> > +
> > +      if (append_char_to_word (s, current_char, false) == false)
> > +        goto cleanup_main;  
> 
> This should be part of "default:" for the switch above... Even if it
> works now...
> 
> Then many "continue" should be converted to more natural "break"...

Good point, will change.

> > +    }
> > +
> > +  if (append_word_to_list (s) == false)
> > +    goto cleanup_main;
> > +
> > +  param_keys = grub_malloc (s->word_list_len * sizeof (char *));
> > +  if (param_keys == NULL)
> > +    goto cleanup_main;
> > +  param_vals = grub_malloc (s->word_list_len * sizeof (char *));
> > +  if (param_vals == NULL)
> > +    goto cleanup_main;
> > +
> > +  for (i = 0; i < s->word_list_len; i++)
> > +    {
> > +      char *current_word_eq_ptr;  
> 
> s/current_word_eq_ptr/eq_pos/

Will change.

> > +      s->current_word = s->word_list[i];
> > +      s->current_word_len = grub_strlen (s->current_word) + 1;
> > +      current_word_eq_ptr = grub_strchr (s->current_word, '=');
> > +
> > +      if (current_word_eq_ptr != NULL)
> > +        {
> > +          /*
> > +           * Both pre_eq_len and post_eq_len represent substring
> > lengths
> > +           * without a NUL terminator.
> > +           */
> > +          grub_size_t pre_eq_len = (grub_size_t)
> > (current_word_eq_ptr - s->current_word);
> > +          /*
> > +           * s->current_word_len includes the NUL terminator, so
> > we subtract
> > +           * one to get rid of the terminator, and one more to get
> > rid of the
> > +           * equals sign.
> > +           */
> > +          grub_size_t post_eq_len = (s->current_word_len - 2) -
> > pre_eq_len; +
> > +          if (is_key_safe (s->current_word, pre_eq_len) == true)
> > +            {
> > +              param_dict_pos = param_dict_len++;
> > +              param_keys[param_dict_pos] = grub_malloc (pre_eq_len
> > + 1);
> > +              if (param_keys == NULL)
> > +                goto cleanup_main;
> > +              param_vals[param_dict_pos] = grub_malloc
> > (post_eq_len + 1);
> > +              if (param_vals == NULL)
> > +                goto cleanup_main;
> > +
> > +              grub_strncpy (param_keys[param_dict_pos],
> > s->current_word, pre_eq_len);
> > +              grub_strncpy (param_vals[param_dict_pos],
> > +			    s->current_word + pre_eq_len + 1,
> > post_eq_len);
> > +              param_keys[param_dict_pos][pre_eq_len] = '\0';
> > +              param_vals[param_dict_pos][post_eq_len] = '\0';
> > +            }
> > +        }
> > +      else  
> 
> else if (is_key_safe (s->current_word, s->current_word_len - 1) ==
> true)
> 
> ... and you can drop an extra indention...

Will change.

> > +        {
> > +          if (is_key_safe (s->current_word, s->current_word_len -
> > 1) == true)
> > +            {
> > +              param_dict_pos = param_dict_len++;
> > +              param_keys[param_dict_pos] = grub_malloc
> > (s->current_word_len);
> > +              if (param_keys == NULL)
> > +                goto cleanup_main;
> > +              param_vals[param_dict_pos] = grub_malloc (2);
> > +              if (param_vals == NULL)
> > +                goto cleanup_main;
> > +
> > +              grub_strncpy (param_keys[param_dict_pos],
> > s->current_word,
> > +			    s->current_word_len);
> > +              if (param_keys[param_dict_pos][s->current_word_len -
> > 1] != '\0' )
> > +                {
> > +                  grub_error (GRUB_ERR_BUG,
> > +			      "NUL terminator missing from key
> > during Xen cmdline parsing");
> > +                  grub_print_error ();
> > +                  grub_exit ();  
> 
> grub_fatal()

Will change.

> > +                }
> > +              grub_strcpy (param_vals[param_dict_pos], "1");
> > +            }
> > +        }
> > +    }  
> 
> Daniel


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Daniel Kiper]

On Tue, Aug 12, 2025 at 06:55:15PM -0500, Aaron Rainbolt wrote:
> On Tue, 12 Aug 2025 19:02:11 +0200 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, Aug 04, 2025 at 11:50:09PM -0500, Aaron Rainbolt wrote:
> > > Xen traditionally allows customizing guest behavior by passing
> > > arguments to the VM kernel via the kernel command line. This is no
> > > longer possible when using GRUB with Xen, as the kernel command
> > > line is decided by the GRUB configuration file within the guest,
> > > not data passed to the guest by Xen.
> > >
> > > To work around this limitation, enable GRUB to parse a command line
> > > passed to it by Xen, and expose data from the command line to the
> > > GRUB configuration as environment variables. These variables can be
> > > used in the GRUB configuration for any desired purpose, such as
> > > extending the kernel command line passed to the guest. The command
> > > line format is inspired by the Linux kernel's command line format.
> > >
> > > To reduce the risk of misuse, abuse, or accidents in production, the
> > > command line will only be parsed if it consists entirely of 7-bit
> > > ASCII characters, only alphabetical characters and underscores are
> > > permitted in variable names, and all variable names must start with
> > > the string "xen_grub_env_". This also allows room for expanding the
> > > command line arguments accepted by GRUB in the future, should other
> > > arguments end up becoming desirable in the future.
> > >
> > > Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>

[...]

> > > diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c index 91fbca8..a8988d2 100644
> > > --- a/grub-core/kern/i386/xen/pvh.c
> > > +++ b/grub-core/kern/i386/xen/pvh.c
> > > @@ -321,6 +321,8 @@ void
> > >  grub_xen_setup_pvh (void)
> > >  {
> > >    grub_addr_t par;
> > > +  const char *xen_cmdline;
> > > +  int i;
> > >
> > >    grub_xen_cpuid_base ();
> > >    grub_xen_setup_hypercall_page ();
> > > @@ -352,6 +354,27 @@ grub_xen_setup_pvh (void)
> > >    grub_xen_mm_init_regions ();
> > >
> > >    grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> > > +
> > > +  xen_cmdline = (const char *) pvh_start_info->cmdline_paddr;
> > > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > > +    {
> > > +      if (xen_cmdline[i] == '\0')
> >
> > This code still does not make a lot of sense for me. You have NUL
> > check in grub_parse_xen_cmdline(). So, you duplicate the check here...
> >
> > I would just fire grub_strncpy() here and forget...
>
> I guess it depends on how you view grub_xen_start_page_addr->cmd_line's
> semantics. In my mind, cmd_line is a NUL-terminated string, always. If
> you boot in PV mode, Xen ensures it's a NUL-terminated string, so it's
> reasonable for code in GRUB to assume it will be one. If you boot in
> PVH mode, it starts out initialized to all zeros which is technically a
> NUL-terminated string, and the code that exists here ensures that if we
> copy the kernel command line to cmd_line, it will still be a
> NUL-terminated string. If we use a "bare" grub_strncpy() here, then if
> someone passes a kernel command line larger than
> GRUB_XEN_MAX_GUEST_CMDLINE - 1, cmd_line will end up not being
> NUL-terminated anymore, and any code added to GRUB in the future that
> assumes it is a NUL-terminated string may buffer overflow.
>
> One could argue "let's keep the NUL check here and remove it from
> grub_parse_xen_cmdline()", but that doesn't work either because we only
> get to control the contents of cmd_line if we boot in PVH mode. If
> instead we boot in PV mode, cmd_line is initialized by Xen itself. GRUB
> receives a pre-populated and ready-to-use start_info struct directly
> from the hypervisor in this scenario. Xen is supposed to ensure that
> start_info is always NUL-terminated, but if there's ever a bug in Xen
> that breaks that assumption, that could result in bad things happening,
> the same as if we didn't do the NUL check in grub_xen_setup_pvh(). Now
> of course there's nothing we can do about Xen possibly being buggy (if
> it gives us a GRUB_XEN_MAX_GUEST_CMDLINE-long buffer with no NUL
> terminator, too bad), but we can at least make sure that we're ready
> for that eventuality. That's why I like having both NUL checks - we're
> ready for if Xen does things wrong, and any future code that isn't
> ready for that eventuality will still work if things go wrong, at least
> in PVH mode.

OK, it looks like this explanation, in shortened form, should land
around this code... Or not... Please look below...

> (Arguably any new code that depends on cmd_line *should* check it for a
> NUL terminator. But I don't want to assume that all new code *will* do
> so.)
>
> > > +        {
> > > +          grub_strncpy ((char *)
> > > grub_xen_start_page_addr->cmd_line,
> > > +			(char *) pvh_start_info->cmdline_paddr,
> > > +			GRUB_XEN_MAX_GUEST_CMDLINE);
> > > +
> > > +          if
> > > (grub_xen_start_page_addr->cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE - 1]
> > > != '\0')
> >
> > If you convince me this code is still needed then I am afraid that
> > this is not what you meant here...
>
> grub_strncpy doesn't just NUL-terminate the command line, it also fills
> the entire remainder of the buffer with NUL characters. Therefore if

I am afraid we are looking at different grub_strncpy() functions...

> grub_strncpy NUL-terminated the copied string, the last character of
> the buffer will always be NUL. This is a redundant check since the code
> above goes to great lengths to avoid ever putting anything
> non-NUL-terminated into the buffer, but I was under the impression you
> wanted the check added just in case. I'm happy to remove it if it's not
> desirable.

I think we misunderstood each other. This code is redundant now...

[...]

> > > +is_key_safe (char *key, grub_size_t len)
> > > +{
> > > +  grub_size_t i;
> > > +
> > > +  for (i = 0; i < len; i++)
> > > +    {
> > > +      if (! (grub_isalpha (key[i]) || key[i] == '_'))
> >
> > Please drop space after "!"...
>
> Will change.
>
> > > +        return false;
> > > +    }
> >
> > You can drop curly braces from here...
>
> Will change.
>
> (I initially kept this written the way it was though because the GNU
> coding standards seemed to indicate I should keep the braces. "When
> you have an if-else statement nested in another if statement, always
> put braces around the if-else." [1] This isn't a nested if, but it's an
> if within a for which is very similar.)
>
> Is there any recommended coding style documentation I should be looking
> at other than the GNU coding standards and the GRUB coding style
> guidelines? It seems I'm making an awful lot of style mistakes and
> would like to avoid that going forward.

Yes, it is but sadly it is not fully/properly/.. documented [1]...

[...]

> > > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > > +    {
> > > +      if (cmdline[i] == '\0')
> > > +        {
> > > +          cmdline_valid = true;
> > > +          break;
> > > +        }
> > > +    }
> > > +
> > > +  if (cmdline_valid == false)
> > > +    {
> > > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > +		  "command line from Xen is not NUL-terminated");
> > > +      grub_print_error ();
> >
> > grub_fatal()?
>
> That would probably be a bad idea here. We use (or, at least, will use)
> grub_fatal() in grub_xen_setup_pvh() because if grub_strncpy() doesn't
> NUL-terminate the string it copies, it indicates a bug in GRUB (either
> in grub_strncpy, or more likely in the nearby NUL-checking code). On
> the other hand, cmdline_valid may equal false if we boot in PV mode and
> Xen incorrectly hands us a non-NUL-terminated string. That's bad and
> prevents us from parsing the command line safely, but I'd argue it
> shouldn't entirely block boot. (Then again, maybe if it does block
> boot, that will make this kind of theoretical Xen bug be much more
> noticeable and help it get fixed quicker. If you'd prefer that we bail
> out entirely here, I'm happy to change it.)

As I said earlier, I would simply call grub_strncpy() in grub_xen_setup_pvh()
without additional NUL-check there and fail if non-NUL-terminated string
is detected here...

> > > +      goto cleanup_main;
> > > +    }
> > > +
> > > +  cmdline_len = grub_strlen (cmdline);
> > > +  for (i = 0; i < cmdline_len; i++)
> > > +    {
> > > +      current_char = cmdline[i];
> > > +
> > > +      /*
> > > +       * If the previous character was a backslash, append the current
> > > +       * character to the word verbatim
> > > +       */
> > > +      if (splitter_state & SPLITTER_HIT_BACKSLASH)
> > > +        {
> > > +          splitter_state &= ~SPLITTER_HIT_BACKSLASH;
> > > +          if (append_char_to_word (s, current_char, false) == false)
> > > +            goto cleanup_main;
> > > +          continue;
> > > +        }
> > > +
> > > +      switch (current_char)
> > > +        {
> > > +        case '\\':
> > > +          /* Backslashes escape arbitrary characters. */
> > > +          splitter_state |= SPLITTER_HIT_BACKSLASH;
> > > +          continue;
> > > +
> > > +        case '\'':
> > > +          /*
> > > +           * Single quotes suppress word splitting and double quoting until
> > > +           * the next single quote is encountered.
> > > +           */
> > > +          if (splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > > +            {
> > > +              if (append_char_to_word (s, current_char, false) == false)
> > > +                goto cleanup_main;
> > > +              continue;
> > > +            }
> > > +
> > > +          splitter_state ^= SPLITTER_IN_SINGLE_QUOTES;
> > > +          continue;
> > > +
> > > +        case '"':
> > > +          /*
> > > +           * Double quotes suppress word splitting and single quoting until
> > > +           * the next double quote is encountered.
> > > +           */
> > > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES)
> > > +            {
> > > +              if (append_char_to_word (s, current_char, false) == false)
> > > +                goto cleanup_main;
> > > +              continue;
> > > +            }
> > > +
> > > +          splitter_state ^= SPLITTER_IN_DOUBLE_QUOTES;
> > > +          continue;
> > > +
> > > +        case ' ':
> > > +          /* Spaces separate words in the command line from each other. */
> > > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES ||
> > > +              splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > > +            {
> > > +              if (append_char_to_word (s, current_char, false) == false)
> > > +                goto cleanup_main;
> > > +              continue;
> > > +            }
> > > +
> > > +          if (append_word_to_list (s) == false)
> > > +            goto cleanup_main;
> >
> > I think this is not fully correct. You should not run
> > append_word_to_list() until the closing quote. So, here you should
> > have "else" for the first "if", i.e., "if (splitter_state & ..." and
> > call append_word_to_list() for closing \" and \' above.
>
> What closing quote? If we're in quotes, the `if (splitter_state &...`
> block will run. That block runs `continue`, restarting the loop before
> we get to the `append_word_to_list (s)` call. If we hit a closing
> quote, either the `case '\''` or the `case '"'` block will be
> triggered, changing the splitter's state so that it knows it's no
> longer within a quote block, and then the next space (or the end of the
> string) will cause `append_word_to_list()` to be called, adding the
> quoted word to the list. Even if this `continue` changes to a `break`
> like suggested below, the logic and control flow will remain the same.
>
> Calling `append_word_to_list()` for closing quotes would break the
> splitter. I'm trying to implement splitting behavior similar to Bash or
> GRUB's configuration language, both of which parse
> `xen_grub_env_var="abc def"ghi` into the variable `xen_grub_env_var`
> with the value `abc defghi`. If we split on closing quotes too, the
> input above will be parsed into two words, `xen_grub_env_var=abc def`
> and an extra word `ghi`, resulting in the environment variable
> `xen_grub_env_var` being set to `abc def` and the `ghi` being lost
> entirely. One could argue that this is reasonable behavior, but it
> isn't consistent with the other splitting behavior in GRUB.

OK, makes sense for me... Maybe you should mention somewhere in the code
current splitter behavior...

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Coding-style

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

Re: [PATCH v4 2/2] kern/xen: Add Xen command line parsing

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 14468 bytes --]

On Wed, 13 Aug 2025 16:27:11 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Aug 12, 2025 at 06:55:15PM -0500, Aaron Rainbolt wrote:
> > On Tue, 12 Aug 2025 19:02:11 +0200 Daniel Kiper
> > <dkiper@net-space.pl> wrote:  
> > > On Mon, Aug 04, 2025 at 11:50:09PM -0500, Aaron Rainbolt wrote:  
> > > > Xen traditionally allows customizing guest behavior by passing
> > > > arguments to the VM kernel via the kernel command line. This is
> > > > no longer possible when using GRUB with Xen, as the kernel
> > > > command line is decided by the GRUB configuration file within
> > > > the guest, not data passed to the guest by Xen.
> > > >
> > > > To work around this limitation, enable GRUB to parse a command
> > > > line passed to it by Xen, and expose data from the command line
> > > > to the GRUB configuration as environment variables. These
> > > > variables can be used in the GRUB configuration for any desired
> > > > purpose, such as extending the kernel command line passed to
> > > > the guest. The command line format is inspired by the Linux
> > > > kernel's command line format.
> > > >
> > > > To reduce the risk of misuse, abuse, or accidents in
> > > > production, the command line will only be parsed if it consists
> > > > entirely of 7-bit ASCII characters, only alphabetical
> > > > characters and underscores are permitted in variable names, and
> > > > all variable names must start with the string "xen_grub_env_".
> > > > This also allows room for expanding the command line arguments
> > > > accepted by GRUB in the future, should other arguments end up
> > > > becoming desirable in the future.
> > > >
> > > > Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>  
> 
> [...]
> 
> > > > diff --git a/grub-core/kern/i386/xen/pvh.c
> > > > b/grub-core/kern/i386/xen/pvh.c index 91fbca8..a8988d2 100644
> > > > --- a/grub-core/kern/i386/xen/pvh.c +++
> > > > b/grub-core/kern/i386/xen/pvh.c @@ -321,6 +321,8 @@ void
> > > >  grub_xen_setup_pvh (void)
> > > >  {
> > > >    grub_addr_t par;
> > > > +  const char *xen_cmdline;
> > > > +  int i;
> > > >
> > > >    grub_xen_cpuid_base ();
> > > >    grub_xen_setup_hypercall_page ();
> > > > @@ -352,6 +354,27 @@ grub_xen_setup_pvh (void)
> > > >    grub_xen_mm_init_regions ();
> > > >
> > > >    grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> > > > +
> > > > +  xen_cmdline = (const char *) pvh_start_info->cmdline_paddr;
> > > > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > > > +    {
> > > > +      if (xen_cmdline[i] == '\0')  
> > >
> > > This code still does not make a lot of sense for me. You have NUL
> > > check in grub_parse_xen_cmdline(). So, you duplicate the check
> > > here...
> > >
> > > I would just fire grub_strncpy() here and forget...  
> >
> > I guess it depends on how you view
> > grub_xen_start_page_addr->cmd_line's semantics. In my mind,
> > cmd_line is a NUL-terminated string, always. If you boot in PV
> > mode, Xen ensures it's a NUL-terminated string, so it's reasonable
> > for code in GRUB to assume it will be one. If you boot in PVH mode,
> > it starts out initialized to all zeros which is technically a
> > NUL-terminated string, and the code that exists here ensures that
> > if we copy the kernel command line to cmd_line, it will still be a
> > NUL-terminated string. If we use a "bare" grub_strncpy() here, then
> > if someone passes a kernel command line larger than
> > GRUB_XEN_MAX_GUEST_CMDLINE - 1, cmd_line will end up not being
> > NUL-terminated anymore, and any code added to GRUB in the future
> > that assumes it is a NUL-terminated string may buffer overflow.
> >
> > One could argue "let's keep the NUL check here and remove it from
> > grub_parse_xen_cmdline()", but that doesn't work either because we
> > only get to control the contents of cmd_line if we boot in PVH
> > mode. If instead we boot in PV mode, cmd_line is initialized by Xen
> > itself. GRUB receives a pre-populated and ready-to-use start_info
> > struct directly from the hypervisor in this scenario. Xen is
> > supposed to ensure that start_info is always NUL-terminated, but if
> > there's ever a bug in Xen that breaks that assumption, that could
> > result in bad things happening, the same as if we didn't do the NUL
> > check in grub_xen_setup_pvh(). Now of course there's nothing we can
> > do about Xen possibly being buggy (if it gives us a
> > GRUB_XEN_MAX_GUEST_CMDLINE-long buffer with no NUL terminator, too
> > bad), but we can at least make sure that we're ready for that
> > eventuality. That's why I like having both NUL checks - we're ready
> > for if Xen does things wrong, and any future code that isn't ready
> > for that eventuality will still work if things go wrong, at least
> > in PVH mode.  
> 
> OK, it looks like this explanation, in shortened form, should land
> around this code... Or not... Please look below...
> 
> > (Arguably any new code that depends on cmd_line *should* check it
> > for a NUL terminator. But I don't want to assume that all new code
> > *will* do so.)
> >  
> > > > +        {
> > > > +          grub_strncpy ((char *)
> > > > grub_xen_start_page_addr->cmd_line,
> > > > +			(char *) pvh_start_info->cmdline_paddr,
> > > > +			GRUB_XEN_MAX_GUEST_CMDLINE);
> > > > +
> > > > +          if
> > > > (grub_xen_start_page_addr->cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE
> > > > - 1] != '\0')  
> > >
> > > If you convince me this code is still needed then I am afraid that
> > > this is not what you meant here...  
> >
> > grub_strncpy doesn't just NUL-terminate the command line, it also
> > fills the entire remainder of the buffer with NUL characters.
> > Therefore if  
> 
> I am afraid we are looking at different grub_strncpy() functions...

Crud. I was going off the semantics of `strncpy` documented in `man
3 stpncpy`. I see now that GRUB's implementation doesn't have the NUL
padding behavior. I'll take a closer look at all uses of grub_strncpy()
and make sure I'm not relying on that behavior anywhere else.

> > grub_strncpy NUL-terminated the copied string, the last character of
> > the buffer will always be NUL. This is a redundant check since the
> > code above goes to great lengths to avoid ever putting anything
> > non-NUL-terminated into the buffer, but I was under the impression
> > you wanted the check added just in case. I'm happy to remove it if
> > it's not desirable.  
> 
> I think we misunderstood each other. This code is redundant now...

Alright, will remove.

> [...]
> 
> > > > +is_key_safe (char *key, grub_size_t len)
> > > > +{
> > > > +  grub_size_t i;
> > > > +
> > > > +  for (i = 0; i < len; i++)
> > > > +    {
> > > > +      if (! (grub_isalpha (key[i]) || key[i] == '_'))  
> > >
> > > Please drop space after "!"...  
> >
> > Will change.
> >  
> > > > +        return false;
> > > > +    }  
> > >
> > > You can drop curly braces from here...  
> >
> > Will change.
> >
> > (I initially kept this written the way it was though because the GNU
> > coding standards seemed to indicate I should keep the braces. "When
> > you have an if-else statement nested in another if statement, always
> > put braces around the if-else." [1] This isn't a nested if, but
> > it's an if within a for which is very similar.)
> >
> > Is there any recommended coding style documentation I should be
> > looking at other than the GNU coding standards and the GRUB coding
> > style guidelines? It seems I'm making an awful lot of style
> > mistakes and would like to avoid that going forward.  
> 
> Yes, it is but sadly it is not fully/properly/.. documented [1]...
> 
> [...]
> 
> > > > +  for (i = 0; i < GRUB_XEN_MAX_GUEST_CMDLINE; i++)
> > > > +    {
> > > > +      if (cmdline[i] == '\0')
> > > > +        {
> > > > +          cmdline_valid = true;
> > > > +          break;
> > > > +        }
> > > > +    }
> > > > +
> > > > +  if (cmdline_valid == false)
> > > > +    {
> > > > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > > +		  "command line from Xen is not
> > > > NUL-terminated");
> > > > +      grub_print_error ();  
> > >
> > > grub_fatal()?  
> >
> > That would probably be a bad idea here. We use (or, at least, will
> > use) grub_fatal() in grub_xen_setup_pvh() because if grub_strncpy()
> > doesn't NUL-terminate the string it copies, it indicates a bug in
> > GRUB (either in grub_strncpy, or more likely in the nearby
> > NUL-checking code). On the other hand, cmdline_valid may equal
> > false if we boot in PV mode and Xen incorrectly hands us a
> > non-NUL-terminated string. That's bad and prevents us from parsing
> > the command line safely, but I'd argue it shouldn't entirely block
> > boot. (Then again, maybe if it does block boot, that will make this
> > kind of theoretical Xen bug be much more noticeable and help it get
> > fixed quicker. If you'd prefer that we bail out entirely here, I'm
> > happy to change it.)  
> 
> As I said earlier, I would simply call grub_strncpy() in
> grub_xen_setup_pvh() without additional NUL-check there and fail if
> non-NUL-terminated string is detected here...

OK, I'll do that then. It will be of critical importance that if any
new code is introduced in GRUB in the future, that it does not assume
cmd_line is NUL-terminated. I'll add a comment to the appropriate
header so that future developers are aware of the danger.

> > > > +      goto cleanup_main;
> > > > +    }
> > > > +
> > > > +  cmdline_len = grub_strlen (cmdline);
> > > > +  for (i = 0; i < cmdline_len; i++)
> > > > +    {
> > > > +      current_char = cmdline[i];
> > > > +
> > > > +      /*
> > > > +       * If the previous character was a backslash, append the
> > > > current
> > > > +       * character to the word verbatim
> > > > +       */
> > > > +      if (splitter_state & SPLITTER_HIT_BACKSLASH)
> > > > +        {
> > > > +          splitter_state &= ~SPLITTER_HIT_BACKSLASH;
> > > > +          if (append_char_to_word (s, current_char, false) ==
> > > > false)
> > > > +            goto cleanup_main;
> > > > +          continue;
> > > > +        }
> > > > +
> > > > +      switch (current_char)
> > > > +        {
> > > > +        case '\\':
> > > > +          /* Backslashes escape arbitrary characters. */
> > > > +          splitter_state |= SPLITTER_HIT_BACKSLASH;
> > > > +          continue;
> > > > +
> > > > +        case '\'':
> > > > +          /*
> > > > +           * Single quotes suppress word splitting and double
> > > > quoting until
> > > > +           * the next single quote is encountered.
> > > > +           */
> > > > +          if (splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > > > +            {
> > > > +              if (append_char_to_word (s, current_char, false)
> > > > == false)
> > > > +                goto cleanup_main;
> > > > +              continue;
> > > > +            }
> > > > +
> > > > +          splitter_state ^= SPLITTER_IN_SINGLE_QUOTES;
> > > > +          continue;
> > > > +
> > > > +        case '"':
> > > > +          /*
> > > > +           * Double quotes suppress word splitting and single
> > > > quoting until
> > > > +           * the next double quote is encountered.
> > > > +           */
> > > > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES)
> > > > +            {
> > > > +              if (append_char_to_word (s, current_char, false)
> > > > == false)
> > > > +                goto cleanup_main;
> > > > +              continue;
> > > > +            }
> > > > +
> > > > +          splitter_state ^= SPLITTER_IN_DOUBLE_QUOTES;
> > > > +          continue;
> > > > +
> > > > +        case ' ':
> > > > +          /* Spaces separate words in the command line from
> > > > each other. */
> > > > +          if (splitter_state & SPLITTER_IN_SINGLE_QUOTES ||
> > > > +              splitter_state & SPLITTER_IN_DOUBLE_QUOTES)
> > > > +            {
> > > > +              if (append_char_to_word (s, current_char, false)
> > > > == false)
> > > > +                goto cleanup_main;
> > > > +              continue;
> > > > +            }
> > > > +
> > > > +          if (append_word_to_list (s) == false)
> > > > +            goto cleanup_main;  
> > >
> > > I think this is not fully correct. You should not run
> > > append_word_to_list() until the closing quote. So, here you should
> > > have "else" for the first "if", i.e., "if (splitter_state & ..."
> > > and call append_word_to_list() for closing \" and \' above.  
> >
> > What closing quote? If we're in quotes, the `if (splitter_state
> > &...` block will run. That block runs `continue`, restarting the
> > loop before we get to the `append_word_to_list (s)` call. If we hit
> > a closing quote, either the `case '\''` or the `case '"'` block
> > will be triggered, changing the splitter's state so that it knows
> > it's no longer within a quote block, and then the next space (or
> > the end of the string) will cause `append_word_to_list()` to be
> > called, adding the quoted word to the list. Even if this `continue`
> > changes to a `break` like suggested below, the logic and control
> > flow will remain the same.
> >
> > Calling `append_word_to_list()` for closing quotes would break the
> > splitter. I'm trying to implement splitting behavior similar to
> > Bash or GRUB's configuration language, both of which parse
> > `xen_grub_env_var="abc def"ghi` into the variable `xen_grub_env_var`
> > with the value `abc defghi`. If we split on closing quotes too, the
> > input above will be parsed into two words, `xen_grub_env_var=abc
> > def` and an extra word `ghi`, resulting in the environment variable
> > `xen_grub_env_var` being set to `abc def` and the `ghi` being lost
> > entirely. One could argue that this is reasonable behavior, but it
> > isn't consistent with the other splitting behavior in GRUB.  
> 
> OK, makes sense for me... Maybe you should mention somewhere in the
> code current splitter behavior...

Technically the behavior is documented in the comment immediately
before the splitter code, but the rationale isn't entirely explained,
so I'll add some extra info to the comment so it's more clear.

--
Aaron

> Daniel
> 
> [1]
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Coding-style


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH v4 1/2] include/xen: Rename MAX_GUEST_CMDLINE to GRUB_XEN_MAX_GUEST_CMDLINE

[Daniel Kiper]

On Mon, Aug 04, 2025 at 11:49:11PM -0500, Aaron Rainbolt wrote:
> The xen.h header was using an overly generic name to refer to the
> maximum length of the command line passed from Xen to a guest. Rename it
> to avoid confusion or conflicts in the future.
>
> Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

Re: [PATCH v4 1/2] include/xen: Rename MAX_GUEST_CMDLINE to GRUB_XEN_MAX_GUEST_CMDLINE

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 1664 bytes --]

I believe this file is copied from xen as-is. That's why it's under xen/
and not grub/. Did you encounter actual conflicts? What does a new version
of xen.h from xen says?

Regards
Vladimir 'phcoder' Serbinenko

Le mar. 5 août 2025, 07:49, Aaron Rainbolt <arraybolt3@gmail.com> a écrit :

> The xen.h header was using an overly generic name to refer to the
> maximum length of the command line passed from Xen to a guest. Rename it
> to avoid confusion or conflicts in the future.
>
> Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
> ---
>  include/xen/xen.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index 692f97a..fdf0fc4 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -823,8 +823,8 @@ struct start_info {
>                                  /* (PFN of pre-loaded module if
>  */
>                                  /*  SIF_MOD_START_PFN set in flags).
> */
>      unsigned long mod_len;      /* Size (bytes) of pre-loaded module.
>  */
> -#define MAX_GUEST_CMDLINE 1024
> -    int8_t cmd_line[MAX_GUEST_CMDLINE];
> +#define GRUB_XEN_MAX_GUEST_CMDLINE 1024
> +    int8_t cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE];
>      /* The pfn range here covers both page table and p->m table frames.
>  */
>      unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.
> */
>      unsigned long nr_p2m_frames;/* # of pfns forming initial P->M table.
> */
> --
> 2.50.1
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2442 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH v4 1/2] include/xen: Rename MAX_GUEST_CMDLINE to GRUB_XEN_MAX_GUEST_CMDLINE

[Aaron Rainbolt]

[-- Attachment #1.1: Type: text/plain, Size: 10829 bytes --]

On Tue, 12 Aug 2025 16:00:05 +0300
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> I believe this file is copied from xen as-is.

It does appear so, yes. It looks like it's a copy of
xen/xen/include/public/xen.h.

> That's why it's under xen/ and not grub/. Did you encounter actual
> conflicts?

I did not, but Daniel requested I rename the define since it was too
generic previously.

> What does a new version of xen.h from xen says?

It's pretty similar, with most but not all of the changes being
comment-only. A unified diff between the version in the tip of GRUB's
master branch and the version in the tip of Xen's stable-4.20 branch is
provided below. [1]

--
Aaron

> Regards
> Vladimir 'phcoder' Serbinenko
> 
> Le mar. 5 août 2025, 07:49, Aaron Rainbolt <arraybolt3@gmail.com> a
> écrit :
> 
> > The xen.h header was using an overly generic name to refer to the
> > maximum length of the command line passed from Xen to a guest.
> > Rename it to avoid confusion or conflicts in the future.
> >
> > Signed-off-by: Aaron Rainbolt <arraybolt3@gmail.com>
> > ---
> >  include/xen/xen.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/xen/xen.h b/include/xen/xen.h
> > index 692f97a..fdf0fc4 100644
> > --- a/include/xen/xen.h
> > +++ b/include/xen/xen.h
> > @@ -823,8 +823,8 @@ struct start_info {
> >                                  /* (PFN of pre-loaded module if
> >  */
> >                                  /*  SIF_MOD_START_PFN set in
> > flags). */
> >      unsigned long mod_len;      /* Size (bytes) of pre-loaded
> > module. */
> > -#define MAX_GUEST_CMDLINE 1024
> > -    int8_t cmd_line[MAX_GUEST_CMDLINE];
> > +#define GRUB_XEN_MAX_GUEST_CMDLINE 1024
> > +    int8_t cmd_line[GRUB_XEN_MAX_GUEST_CMDLINE];
> >      /* The pfn range here covers both page table and p->m table
> > frames. */
> >      unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M
> > table. */
> >      unsigned long nr_p2m_frames;/* # of pfns forming initial P->M
> > table. */
> > --
> > 2.50.1
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >  

[1] The diff mentioned above:

--- grub/include/xen/xen.h      2025-08-12 17:33:48.086097055 -0500
+++ xen/xen/include/public/xen.h        2025-08-12 17:11:01.399108823 -0500
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
 /******************************************************************************
  * xen.h
  *
  * Guest OS interface to Xen.
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
  * Copyright (c) 2004, K A Fraser
  */
 
@@ -33,6 +16,10 @@
 #include "arch-x86/xen.h"
 #elif defined(__arm__) || defined (__aarch64__)
 #include "arch-arm.h"
+#elif defined(__powerpc64__)
+#include "arch-ppc.h"
+#elif defined(__riscv)
+#include "arch-riscv.h"
 #else
 #error "Unsupported architecture"
 #endif
@@ -53,6 +40,15 @@
 DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
 DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
+/* Define a variable length array (depends on compiler). */
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
+#define XEN_FLEX_ARRAY_DIM
+#elif defined(__GNUC__)
+#define XEN_FLEX_ARRAY_DIM  0
+#else
+#define XEN_FLEX_ARRAY_DIM  1 /* variable size */
+#endif
+
 /* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U
 #define __xen_mk_ulong(x) x ## UL
@@ -118,9 +114,10 @@
 #define __HYPERVISOR_domctl               36
 #define __HYPERVISOR_kexec_op             37
 #define __HYPERVISOR_tmem_op              38
-#define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
+#define __HYPERVISOR_argo_op              39
 #define __HYPERVISOR_xenpmu_op            40
 #define __HYPERVISOR_dm_op                41
+#define __HYPERVISOR_hypfs_op             42
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
@@ -177,8 +174,8 @@
 #define VIRQ_XENOPROF   7  /* V. XenOprofile interrupt: new sample available */
 #define VIRQ_CON_RING   8  /* G. (DOM0) Bytes received on console            */
 #define VIRQ_PCPU_STATE 9  /* G. (DOM0) PCPU state changed                   */
-#define VIRQ_MEM_EVENT  10 /* G. (DOM0) A memory event has occured           */
-#define VIRQ_XC_RESERVED 11 /* G. Reserved for XenClient                     */
+#define VIRQ_MEM_EVENT  10 /* G. (DOM0) A memory event has occurred          */
+#define VIRQ_ARGO       11 /* G. Argo interdomain message notification       */
 #define VIRQ_ENOMEM     12 /* G. (DOM0) Low on heap memory       */
 #define VIRQ_XENPMU     13 /* V.  PMC interrupt                              */
 
@@ -268,6 +265,10 @@
  * As MMU_NORMAL_PT_UPDATE above, but A/D bits currently in the PTE are ORed
  * with those in @val.
  *
+ * ptr[1:0] == MMU_PT_UPDATE_NO_TRANSLATE:
+ * As MMU_NORMAL_PT_UPDATE above, but @val is not translated though FD
+ * page tables.
+ *
  * @val is usually the machine frame number along with some attributes.
  * The attributes by default follow the architecture defined bits. Meaning that
  * if this is a X86_64 machine and four page table layout is used, the layout
@@ -334,9 +335,11 @@
  *
  * PAT (bit 7 on) --> PWT (bit 3 on) and clear bit 7.
  */
-#define MMU_NORMAL_PT_UPDATE      0 /* checked '*ptr = val'. ptr is MA.      */
-#define MMU_MACHPHYS_UPDATE       1 /* ptr = MA of frame to modify entry for */
-#define MMU_PT_UPDATE_PRESERVE_AD 2 /* atomically: *ptr = val | (*ptr&(A|D)) */
+#define MMU_NORMAL_PT_UPDATE       0 /* checked '*ptr = val'. ptr is MA.      */
+#define MMU_MACHPHYS_UPDATE        1 /* ptr = MA of frame to modify entry for */
+#define MMU_PT_UPDATE_PRESERVE_AD  2 /* atomically: *ptr = val | (*ptr&(A|D)) */
+#define MMU_PT_UPDATE_NO_TRANSLATE 3 /* checked '*ptr = val'. ptr is MA.      */
+                                     /* val never translated.                 */
 
 /*
  * MMU EXTENDED OPERATIONS
@@ -480,7 +483,28 @@
 /* ` } */
 
 /*
- * Commands to HYPERVISOR_console_io().
+ * ` int
+ * ` HYPERVISOR_console_io(unsigned int cmd,
+ * `                       unsigned int count,
+ * `                       char buffer[]);
+ *
+ * @cmd: Command (see below)
+ * @count: Size of the buffer to read/write
+ * @buffer: Pointer in the guest memory
+ *
+ * List of commands:
+ *
+ *  * CONSOLEIO_write: Write the buffer to Xen console.
+ *      For the hardware domain, all the characters in the buffer will
+ *      be written. Characters will be printed directly to the console.
+ *      For all the other domains, only the printable characters will be
+ *      written. Characters may be buffered until a newline (i.e '\n') is
+ *      found.
+ *      @return 0 on success, otherwise return an error code.
+ *  * CONSOLEIO_read: Attempts to read up to @count characters from Xen
+ *      console. The maximum buffer size (i.e. @count) supported is 2GB.
+ *      @return the number of characters read on success, otherwise return
+ *      an error code.
  */
 #define CONSOLEIO_write         0
 #define CONSOLEIO_read          1
@@ -578,6 +602,9 @@
 /* Idle domain. */
 #define DOMID_IDLE           xen_mk_uint(0x7FFF)
 
+/* Mask for valid domain id values */
+#define DOMID_MASK           xen_mk_uint(0x7FFF)
+
 #ifndef __ASSEMBLY__
 
 typedef uint16_t domid_t;
@@ -596,7 +623,7 @@
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
- * `                      uint32_t nr_calls);
+ * `                      unsigned long nr_calls);
  *
  * NB. The fields are logically the natural register size for this
  * architecture. In cases where xen_ulong_t is larger than this then
@@ -686,7 +713,7 @@
 #endif /* XEN_HAVE_PV_UPCALL_MASK */
     xen_ulong_t evtchn_pending_sel;
     struct arch_vcpu_info arch;
-    struct vcpu_time_info time;
+    vcpu_time_info_t time;
 }; /* 64 bytes (x86) */
 #ifndef __XEN__
 typedef struct vcpu_info vcpu_info_t;
@@ -739,12 +766,17 @@
     xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
 
     /*
-     * Wallclock time: updated only by control software. Guests should base
-     * their gettimeofday() syscall on this wallclock-base value.
+     * Wallclock time: updated by control software or RTC emulation.
+     * Guests should base their gettimeofday() syscall on this
+     * wallclock-base value.
+     * The values of wc_sec and wc_nsec are offsets from the Unix epoch
+     * adjusted by the domain's 'time offset' (in seconds) as set either
+     * by XEN_DOMCTL_settimeoffset, or adjusted via a guest write to the
+     * emulated RTC.
      */
     uint32_t wc_version;      /* Version counter: see vcpu_time_info_t. */
-    uint32_t wc_sec;          /* Secs  00:00:00 UTC, Jan 1, 1970.  */
-    uint32_t wc_nsec;         /* Nsecs 00:00:00 UTC, Jan 1, 1970.  */
+    uint32_t wc_sec;
+    uint32_t wc_nsec;
 #if !defined(__i386__)
     uint32_t wc_sec_hi;
 # define xen_wc_sec_hi wc_sec_hi
@@ -916,6 +948,11 @@
             uint32_t gbl_caps;
             /* Mode attributes (offset 0x0, VESA command 0x4f01). */
             uint16_t mode_attrs;
+            uint16_t pad;
+#endif
+#if __XEN_INTERFACE_VERSION__ >= 0x00040d00
+            /* high 32 bits of lfb_base */
+            uint32_t ext_lfb_base;
 #endif
         } vesa_lfb;
     } u;
@@ -981,6 +1018,7 @@
     XEN_GUEST_HANDLE_64(uint8) bitmap;
     uint32_t nr_bits;
 };
+typedef struct xenctl_bitmap xenctl_bitmap_t;
 #endif
 
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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