Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> On 05/04/2018 09:49 AM, Collin Walling wrote:
> > When a user incorrectly provides an hmp command, an error response will be
> > printed that prompts the user to try "help <command name>". However, when
> > the command contains multiple parts e.g. "info skeys", only the last
> > whitespace delimited string will be reported (in this example "info" will
> > be dropped and the message will read "Try "help skeys" for more 
> > information",
> > which is incorrect).
> 
> What's the exact formula for reproducing this?  I tried:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic --monitor stdio
> QEMU 2.12.50 monitor - type 'help' for more information
> (qemu) info skeys
> unknown command: 'info skeys'


Ah, so I fixed the 'info skeys' case in 250b8197640
and that ended up using cmdp_start

> Oh, I see now:
> 
> (qemu) info uuid blah
> uuid: extraneous characters at the end of line
> Try "help uuid" for more information

Yep that's fair that needs fixing.

Dave

> (qemu) help uuid
> (qemu)

> You'll want to update your commit message to document something that is
> reproducible (you may be adding an 'info skeys', but until that is in, it
> doesn't make a good example).
> 
> > 
> > Let's correct this by capturing the full name of the command as we recurse
> > through the function monitor_parse_command.
> > 
> > Reported-by: Mikhail Fokin <address@hidden>
> > Signed-off-by: Collin Walling <address@hidden>
> > ---
> >   monitor.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 39f8ee1..d4844b4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const 
> > mon_cmd_t *disp_table,
> >   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >                                                 const char *cmdp_start,
> >                                                 const char **cmdp,
> > -                                              mon_cmd_t *table)
> > +                                              mon_cmd_t *table,
> > +                                              char *fullname)
> 
> Umm, how is fullname any better than cmdp_start that we already have?
> 
> >   {
> >       const char *p;
> >       const mon_cmd_t *cmd;
> > @@ -2987,10 +2988,14 @@ static const mon_cmd_t 
> > *monitor_parse_command(Monitor *mon,
> >           p++;
> >       }
> > +    strncat(fullname, cmdname, strlen(cmdname));
> 
> gcc 8 is pickier about using strncat() [perhaps too picky - see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602], but it is generally NOT
> the function you want to be using.
> 
> > +
> >       *cmdp = p;
> >       /* search sub command */
> >       if (cmd->sub_table != NULL && *p != '\0') {
> > -        return monitor_parse_command(mon, cmdp_start, cmdp, 
> > cmd->sub_table);
> > +        strncat(fullname, " ", 1);
> > +        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table,
> > +                                     fullname);
> 
> See, you're reconstructing a command into fullname, which already matches
> the original command in cmdp_start, so I see no reason to change the
> signature.
> 
> >       }
> >       return cmd;
> > @@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const 
> > char *cmdline)
> >   {
> >       QDict *qdict;
> >       const mon_cmd_t *cmd;
> > +    char fullname[256];
> 
> EWWW. Don't do that.  You are just ASKING for a buffer overflow exploit that
> prints the wrong thing or causes a security hole, when I intentionally type
> a super-long garbage command into HMP.
> 
> >       trace_handle_hmp_command(mon, cmdline);
> > -    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> > +    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table,
> > +                                fullname);
> 
> Note that even without your patch, this call updates 'cmdline' to point to
> the position within the original string (although that position has already
> skipped spaces).
> 
> >       if (!cmd) {
> >           return;
> >       }
> > @@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const 
> > char *cmdline)
> >       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
> >       if (!qdict) {
> >           monitor_printf(mon, "Try \"help %s\" for more information\n",
> > -                       cmd->name);
> > +                       fullname);
> 
> So rather than trying to reconstruct a string, you could reuse what you
> already have.  This is a shorter patch that I think accomplishes the same
> goal:
> 
> diff --git i/monitor.c w/monitor.c
> index 39f8ee17ba7..38736b3a20d 100644
> --- i/monitor.c
> +++ w/monitor.c
> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const
> char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    const char *cmd_start = cmdline;
> 
>      trace_handle_hmp_command(mon, cmdline);
> 
> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const
> char *cmdline)
> 
>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>      if (!qdict) {
> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> +            cmdline--;
> +        }
> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> +                       (int)(cmdline - cmd_start), cmd_start);
>          return;
>      }
> 
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK