On Wed, 13 Aug 2025 16:27:11 +0200 Daniel Kiper <dki...@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
> > <dki...@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 <arraybo...@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
pgpPCluQEK4GQ.pgp
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
