* Eric Blake (ebl...@redhat.com) 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 <fo...@de.ibm.com>
> > Signed-off-by: Collin Walling <wall...@linux.ibm.com>
> > ---
> > 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 / dgilb...@redhat.com / Manchester, UK
