[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on erro
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error |
Date: |
Fri, 4 May 2018 10:19:18 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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'
Oh, I see now:
(qemu) info uuid blah
uuid: extraneous characters at the end of line
Try "help uuid" for more information
(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
- [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Collin Walling, 2018/05/04
- Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error,
Eric Blake <=
- Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Dr. David Alan Gilbert, 2018/05/04
- Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Collin Walling, 2018/05/04
- Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Eric Blake, 2018/05/04
- Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Collin Walling, 2018/05/07
- Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error, Collin Walling, 2018/05/07
- Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error, Eric Blake, 2018/05/07
- Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error, Collin Walling, 2018/05/07
- Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error, Markus Armbruster, 2018/05/24
- Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error, Dr. David Alan Gilbert, 2018/05/30
Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error, Eric Blake, 2018/05/04