chardev/char.c | 2 +- gdbstub.c | 2 +- include/monitor/monitor.h | 2 +- monitor.c | 82 +++++++++++++++++++++++++++++++++++++-- qemu-options.hx | 8 +++- stubs/monitor.c | 2 +- vl.c | 5 ++- 7 files changed, 92 insertions(+), 11 deletions(-)
The whitelist option allows to run a reduced monitor with a subset of
QMP commands. This allows the monitor to run in secure mode, which is
convenient for sending commands via the WebSocket monitor using the
web UI. This is planned to be done on micro:bit board.
The list of allowed commands should be written to a file, one per line.
The command line will look like this:
-mon chardev_name,mode=control,whitelist=path_to_file
Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
chardev/char.c | 2 +-
gdbstub.c | 2 +-
include/monitor/monitor.h | 2 +-
monitor.c | 82 +++++++++++++++++++++++++++++++++++++--
qemu-options.hx | 8 +++-
stubs/monitor.c | 2 +-
vl.c | 5 ++-
7 files changed, 92 insertions(+), 11 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index ccba36bafb..9af8690806 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -726,7 +726,7 @@ Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
if (qemu_opt_get_bool(opts, "mux", 0)) {
assert(permit_mux_mon);
- monitor_init(chr, MONITOR_USE_READLINE);
+ monitor_init(chr, MONITOR_USE_READLINE, NULL);
}
out:
diff --git a/gdbstub.c b/gdbstub.c
index 3129b5c284..a91a47e79b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2542,7 +2542,7 @@ int gdbserver_start(const char *device)
/* Initialize a monitor terminal for gdb */
mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
NULL, &error_abort);
- monitor_init(mon_chr, 0);
+ monitor_init(mon_chr, 0, NULL);
} else {
qemu_chr_fe_deinit(&s->chr, true);
mon_chr = s->mon_chr;
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c1b40a9cac..a0d7ad2ba2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -19,7 +19,7 @@ extern __thread Monitor *cur_mon;
bool monitor_cur_is_qmp(void);
void monitor_init_globals(void);
-void monitor_init(Chardev *chr, int flags);
+void monitor_init(Chardev *chr, int flags, const char *whitelist_file);
void monitor_cleanup(void);
int monitor_suspend(Monitor *mon);
diff --git a/monitor.c b/monitor.c
index c09fa63940..a26ddf72bb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -238,6 +238,7 @@ struct Monitor {
guint out_watch;
/* Read under either BQL or mon_lock, written with BQL+mon_lock. */
int mux_out;
+ GHashTable *whitelist;
};
/* Shared monitor I/O thread */
@@ -358,6 +359,22 @@ static void qmp_request_free(QMPRequest *req)
g_free(req);
}
+static void monitor_qmp_cleanup_commands(Monitor *mon)
+{
+ QmpCommand *cmd, *next_cmd;
+
+ if (!monitor_is_qmp(mon) ||
+ mon->qmp.commands == &qmp_cap_negotiation_commands) {
+ return;
+ }
+
+ QTAILQ_FOREACH_SAFE(cmd, mon->qmp.commands, node, next_cmd) {
+ QTAILQ_REMOVE(mon->qmp.commands, cmd, node);
+ g_free(cmd);
+ }
+ g_free(mon->qmp.commands);
+}
+
/* Caller must hold mon->qmp.qmp_queue_lock */
static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
{
@@ -739,6 +756,10 @@ static void monitor_data_destroy(Monitor *mon)
qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
monitor_qmp_cleanup_req_queue_locked(mon);
g_queue_free(mon->qmp.qmp_requests);
+ if (mon->whitelist) {
+ g_hash_table_destroy(mon->whitelist);
+ }
+ monitor_qmp_cleanup_commands(mon);
}
char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -1242,10 +1263,28 @@ static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
return true;
}
+static void monitor_copy_qmp_commands(Monitor *mon, QmpCommandList *cmds)
+{
+ QmpCommand *cmd;
+
+ mon->qmp.commands = g_malloc0(sizeof(*cmds));
+ QTAILQ_INIT(mon->qmp.commands);
+ QTAILQ_FOREACH(cmd, cmds, node) {
+ QmpCommand *new_cmd = g_memdup(cmd, sizeof(*cmd));
+
+ if (mon->whitelist) {
+ new_cmd->enabled = g_hash_table_contains(mon->whitelist,
+ new_cmd->name);
+ }
+ QTAILQ_INSERT_TAIL(mon->qmp.commands, new_cmd, node);
+ }
+}
+
void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
Error **errp)
{
- if (cur_mon->qmp.commands == &qmp_commands) {
+ if (cur_mon->qmp.commands &&
+ cur_mon->qmp.commands != &qmp_cap_negotiation_commands) {
error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
"Capabilities negotiation is already complete, command "
"ignored");
@@ -1256,7 +1295,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
return;
}
- cur_mon->qmp.commands = &qmp_commands;
+ monitor_copy_qmp_commands(cur_mon, &qmp_commands);
}
/* Set the current CPU defined by the user. Callers must hold BQL. */
@@ -2621,7 +2660,6 @@ static mon_cmd_t mon_cmds[] = {
static const char *pch;
static sigjmp_buf expr_env;
-
static void GCC_FMT_ATTR(2, 3) QEMU_NORETURN
expr_error(Monitor *mon, const char *fmt, ...)
{
@@ -4562,7 +4600,32 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
monitor_list_append(mon);
}
-void monitor_init(Chardev *chr, int flags)
+static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
+{
+ char cmd_name[256];
+ FILE *fd = fopen(whitelist_file, "r");
+
+ if (fd == NULL) {
+ error_report("Could not open whitelist file: %s", strerror(errno));
+ exit(1);
+ }
+
+ mon->whitelist = g_hash_table_new_full(g_str_hash,
+ g_str_equal,
+ g_free,
+ NULL);
+
+ g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
+ g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
+
+ while (fscanf(fd, "%255s", cmd_name) == 1) {
+ g_hash_table_add(mon->whitelist, g_strdup(cmd_name));
+ }
+
+ fclose(fd);
+}
+
+void monitor_init(Chardev *chr, int flags, const char *whitelist_file)
{
Monitor *mon = g_malloc(sizeof(*mon));
bool use_readline = flags & MONITOR_USE_READLINE;
@@ -4583,6 +4646,14 @@ void monitor_init(Chardev *chr, int flags)
monitor_read_command(mon, 0);
}
+ if (whitelist_file) {
+ if (monitor_is_qmp(mon)) {
+ process_whitelist_file(mon, whitelist_file);
+ } else {
+ warn_report("HMP doesn't support whitelist option: file ignored");
+ }
+ }
+
if (monitor_is_qmp(mon)) {
qemu_chr_fe_set_echo(&mon->chr, true);
json_message_parser_init(&mon->qmp.parser, handle_qmp_command,
@@ -4666,6 +4737,9 @@ QemuOptsList qemu_mon_opts = {
},{
.name = "pretty",
.type = QEMU_OPT_BOOL,
+ },{
+ .name = "whitelist",
+ .type = QEMU_OPT_STRING,
},
{ /* end of list */ }
},
diff --git a/qemu-options.hx b/qemu-options.hx
index 521511ec13..e5d1b7dfb5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3195,12 +3195,16 @@ Like -qmp but uses pretty JSON formatting.
ETEXI
DEF("mon", HAS_ARG, QEMU_OPTION_mon, \
- "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]\n", QEMU_ARCH_ALL)
+ "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]" \
+ "[,whitelist=file]\n", QEMU_ARCH_ALL)
STEXI
-@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]
+@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]][,whitelist=@var{file}]
@findex -mon
Setup monitor on chardev @var{name}. @code{pretty} turns on JSON pretty printing
easing human reading and debugging.
+The @code{whitelist} option disables all commands except those specified in
+@var{file}. The file must contain one command name per line. This option is only
+avaliable in 'control' mode.
ETEXI
DEF("debugcon", HAS_ARG, QEMU_OPTION_debugcon, \
diff --git a/stubs/monitor.c b/stubs/monitor.c
index 32bd7012c3..36725b8090 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -12,7 +12,7 @@ int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
return -1;
}
-void monitor_init(Chardev *chr, int flags)
+void monitor_init(Chardev *chr, int flags, const char *whitelist_file)
{
}
diff --git a/vl.c b/vl.c
index bc9fbec654..81278a0155 100644
--- a/vl.c
+++ b/vl.c
@@ -2337,6 +2337,7 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
Chardev *chr;
const char *chardev;
const char *mode;
+ const char *whitelist_file;
int flags;
mode = qemu_opt_get(opts, "mode");
@@ -2366,7 +2367,9 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
return -1;
}
- monitor_init(chr, flags);
+ whitelist_file = qemu_opt_get(opts, "whitelist");
+
+ monitor_init(chr, flags, whitelist_file);
return 0;
}
--
2.17.1
Patchew URL: https://patchew.org/QEMU/20190131202637.4062-1-jusual@mail.ru/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] monitor: Add whitelist support for QMP commands Type: series Message-id: 20190131202637.4062-1-jusual@mail.ru === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 5b7f9be0bb monitor: Add whitelist support for QMP commands === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> total: 1 errors, 0 warnings, 206 lines checked Commit 5b7f9be0bbd6 (monitor: Add whitelist support for QMP commands) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190131202637.4062-1-jusual@mail.ru/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190131202637.4062-1-jusual@mail.ru/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20190131202637.4062-1-jusual@mail.ru Subject: [Qemu-devel] [PATCH] monitor: Add whitelist support for QMP commands === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20190130155733.32742-1-david@redhat.com -> patchew/20190130155733.32742-1-david@redhat.com * [new tag] patchew/20190131202637.4062-1-jusual@mail.ru -> patchew/20190131202637.4062-1-jusual@mail.ru Switched to a new branch 'test' 5b7f9be monitor: Add whitelist support for QMP commands === OUTPUT BEGIN === ERROR: Author email address is mangled by the mailing list #2: Author: Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> total: 1 errors, 0 warnings, 206 lines checked Commit 5b7f9be0bbd6 (monitor: Add whitelist support for QMP commands) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190131202637.4062-1-jusual@mail.ru/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: > The whitelist option allows to run a reduced monitor with a subset of > QMP commands. This allows the monitor to run in secure mode, which is For a value of "secure". I'm not saying this can't be useful, just tempering expecations. I guess you intend to use this to restrict the monitor to sufficiently harmless commands, such as commands that merely return information without changing anything. However, even such commands can be abused for denial of service. Whether that's an issue depends on your use case. > convenient for sending commands via the WebSocket monitor using the > web UI. This is planned to be done on micro:bit board. > > The list of allowed commands should be written to a file, one per line. > The command line will look like this: > -mon chardev_name,mode=control,whitelist=path_to_file > > Signed-off-by: Julia Suvorova <jusual@mail.ru> Please describe your intended use case in more detail, and provide at least a rough security analysis that includes the denial of service aspect.
On 01.02.2019 12:14, Markus Armbruster wrote: > Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: > >> The whitelist option allows to run a reduced monitor with a subset of >> QMP commands. This allows the monitor to run in secure mode, which is > > For a value of "secure". I'm not saying this can't be useful, just > tempering expecations. I guess you intend to use this to restrict the > monitor to sufficiently harmless commands, such as commands that merely > return information without changing anything. However, even such > commands can be abused for denial of service. Whether that's an issue > depends on your use case. > >> convenient for sending commands via the WebSocket monitor using the >> web UI. This is planned to be done on micro:bit board. >> >> The list of allowed commands should be written to a file, one per line. >> The command line will look like this: >> -mon chardev_name,mode=control,whitelist=path_to_file >> >> Signed-off-by: Julia Suvorova <jusual@mail.ru> > > Please describe your intended use case in more detail, and provide at > least a rough security analysis that includes the denial of service > aspect. It is planned to use the web interface for micro:bit board, e.g. send a button press as a QMP command and send a LED display changing as a QMP event, and send them via WebSocket protocol from QEMU to a web page. The monitor will also send commands from some other sensors, for example, an accelerometer/magnetometer. Therefore, it is convenient to limit the monitor so that it can send only these commands of buttons/sensors. Best regards, Julia Suvorova.
Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: > On 01.02.2019 12:14, Markus Armbruster wrote: >> Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: >> >>> The whitelist option allows to run a reduced monitor with a subset of >>> QMP commands. This allows the monitor to run in secure mode, which is >> >> For a value of "secure". I'm not saying this can't be useful, just >> tempering expecations. I guess you intend to use this to restrict the >> monitor to sufficiently harmless commands, such as commands that merely >> return information without changing anything. However, even such >> commands can be abused for denial of service. Whether that's an issue >> depends on your use case. >> >>> convenient for sending commands via the WebSocket monitor using the >>> web UI. This is planned to be done on micro:bit board. >>> >>> The list of allowed commands should be written to a file, one per line. >>> The command line will look like this: >>> -mon chardev_name,mode=control,whitelist=path_to_file >>> >>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> >> Please describe your intended use case in more detail, and provide at >> least a rough security analysis that includes the denial of service >> aspect. > > It is planned to use the web interface for micro:bit board, e.g. send a > button press as a QMP command and send a LED display changing as a QMP > event, and send them via WebSocket protocol from QEMU to a web page. > The monitor will also send commands from some other sensors, for > example, an accelerometer/magnetometer. Therefore, it is convenient to > limit the monitor so that it can send only these commands of > buttons/sensors. Covers "describe your intended use case" nicely, thanks. Still missing: "a rough security analysis that includes the denial of service aspect". Here are two questions to help you get started. Is "the web interface" trusted? If not, what if it floods QEMU with button presses?
On 12.02.2019 10:13, Markus Armbruster wrote: > Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: > >> On 01.02.2019 12:14, Markus Armbruster wrote: >>> Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: >>> >>>> The whitelist option allows to run a reduced monitor with a subset of >>>> QMP commands. This allows the monitor to run in secure mode, which is >>> >>> For a value of "secure". I'm not saying this can't be useful, just >>> tempering expecations. I guess you intend to use this to restrict the >>> monitor to sufficiently harmless commands, such as commands that merely >>> return information without changing anything. However, even such >>> commands can be abused for denial of service. Whether that's an issue >>> depends on your use case. >>> >>>> convenient for sending commands via the WebSocket monitor using the >>>> web UI. This is planned to be done on micro:bit board. >>>> >>>> The list of allowed commands should be written to a file, one per line. >>>> The command line will look like this: >>>> -mon chardev_name,mode=control,whitelist=path_to_file >>>> >>>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >>> >>> Please describe your intended use case in more detail, and provide at >>> least a rough security analysis that includes the denial of service >>> aspect. >> >> It is planned to use the web interface for micro:bit board, e.g. send a >> button press as a QMP command and send a LED display changing as a QMP >> event, and send them via WebSocket protocol from QEMU to a web page. >> The monitor will also send commands from some other sensors, for >> example, an accelerometer/magnetometer. Therefore, it is convenient to >> limit the monitor so that it can send only these commands of >> buttons/sensors. > > Covers "describe your intended use case" nicely, thanks. Still missing: > "a rough security analysis that includes the denial of service aspect". > Here are two questions to help you get started. Is "the web interface" > trusted? If not, what if it floods QEMU with button presses? In the case of a trusted connection, we don't even need a whitelist. The oob execution must be disabled in order to prohibit the direct execution of speculative series of commands. The regular command suspends the monitor and causes it to stop reading the commands until the execution is completed, which prevents the client from capturing too many resources. And since the suspension of the monitor doesn't stop the sending of events, the display will work fine. Best regards, Julia Suvorova.
I apologize for the long delay. Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: > On 12.02.2019 10:13, Markus Armbruster wrote: >> Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: >> >>> On 01.02.2019 12:14, Markus Armbruster wrote: >>>> Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> writes: >>>> >>>>> The whitelist option allows to run a reduced monitor with a subset of >>>>> QMP commands. This allows the monitor to run in secure mode, which is >>>> >>>> For a value of "secure". I'm not saying this can't be useful, just >>>> tempering expecations. I guess you intend to use this to restrict the >>>> monitor to sufficiently harmless commands, such as commands that merely >>>> return information without changing anything. However, even such >>>> commands can be abused for denial of service. Whether that's an issue >>>> depends on your use case. >>>> >>>>> convenient for sending commands via the WebSocket monitor using the >>>>> web UI. This is planned to be done on micro:bit board. >>>>> >>>>> The list of allowed commands should be written to a file, one per line. >>>>> The command line will look like this: >>>>> -mon chardev_name,mode=control,whitelist=path_to_file >>>>> >>>>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >>>> >>>> Please describe your intended use case in more detail, and provide at >>>> least a rough security analysis that includes the denial of service >>>> aspect. >>> >>> It is planned to use the web interface for micro:bit board, e.g. send a >>> button press as a QMP command and send a LED display changing as a QMP >>> event, and send them via WebSocket protocol from QEMU to a web page. >>> The monitor will also send commands from some other sensors, for >>> example, an accelerometer/magnetometer. Therefore, it is convenient to >>> limit the monitor so that it can send only these commands of >>> buttons/sensors. >> >> Covers "describe your intended use case" nicely, thanks. Still missing: >> "a rough security analysis that includes the denial of service aspect". >> Here are two questions to help you get started. Is "the web interface" >> trusted? If not, what if it floods QEMU with button presses? > > In the case of a trusted connection, we don't even need a whitelist. > The oob execution must be disabled in order to prohibit the direct > execution of speculative series of commands. The regular command > suspends the monitor and causes it to stop reading the commands until > the execution is completed, which prevents the client from capturing too > many resources. It could still peg one core, at least in theory. > And since the suspension of the monitor doesn't stop the > sending of events, the display will work fine. This is not an objection; I asked for a rough security analysis not to challenge its premises, but to make sure you have one. Now put it on record by working it into your commit message. I still have to review the actual patch. You may want to wait for that before you respin.
On Thu, Jan 31, 2019 at 11:26:37PM +0300, Julia Suvorova wrote:
> The whitelist option allows to run a reduced monitor with a subset of
> QMP commands. This allows the monitor to run in secure mode, which is
> convenient for sending commands via the WebSocket monitor using the
> web UI. This is planned to be done on micro:bit board.
>
> The list of allowed commands should be written to a file, one per line.
> The command line will look like this:
> -mon chardev_name,mode=control,whitelist=path_to_file
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
Please include a test case. tests/qmp-cmd-test.c looks like a starting
point.
Interesting cases:
1. QMP negotiation still works.
2. A whitelisted command succeeds.
3. A forbidden command fails with the expected error.
> +static void monitor_qmp_cleanup_commands(Monitor *mon)
> +{
> + QmpCommand *cmd, *next_cmd;
> +
> + if (!monitor_is_qmp(mon) ||
> + mon->qmp.commands == &qmp_cap_negotiation_commands) {
> + return;
> + }
Checking side-effects can be avoided with a bool flag:
if (!mon->qmp_commands_needs_free) {
return;
}
Any place that allocates qmp.commands must set this flag and then we
don't need to worry whether we're checking the right side-effects in the
cleanup function.
I think this makes the code easier to read but it's just a suggestion.
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 521511ec13..e5d1b7dfb5 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3195,12 +3195,16 @@ Like -qmp but uses pretty JSON formatting.
> ETEXI
>
> DEF("mon", HAS_ARG, QEMU_OPTION_mon, \
> - "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]\n", QEMU_ARCH_ALL)
> + "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]" \
> + "[,whitelist=file]\n", QEMU_ARCH_ALL)
> STEXI
> -@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]
> +@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]][,whitelist=@var{file}]
Your change reminded me that "[chardev=]name" should be
"[chardev=]@var{name}" since 'name' isn't a string literal but a
variable.
This is a pre-existing issue but please add a patch to this series to
fix it.
> @findex -mon
> Setup monitor on chardev @var{name}. @code{pretty} turns on JSON pretty printing
> easing human reading and debugging.
> +The @code{whitelist} option disables all commands except those specified in
> +@var{file}. The file must contain one command name per line. This option is only
> +avaliable in 'control' mode.
s/avaliable/available/
On 1/31/19 2:26 PM, Julia Suvorova via Qemu-devel wrote:
> The whitelist option allows to run a reduced monitor with a subset of
> QMP commands. This allows the monitor to run in secure mode, which is
> convenient for sending commands via the WebSocket monitor using the
> web UI. This is planned to be done on micro:bit board.
>
> The list of allowed commands should be written to a file, one per line.
> The command line will look like this:
> -mon chardev_name,mode=control,whitelist=path_to_file
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>
> -void monitor_init(Chardev *chr, int flags)
> +static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
> +{
> + char cmd_name[256];
> + FILE *fd = fopen(whitelist_file, "r");
If you use qemu_open() here (followed by fdopen if you still prefer
fscanf over read), then you can support "/dev/fdset/NNN" to
auto-magically support someone passing in the whitelist via an inherited
file descriptor, rather than having to be somewhere on disk that qemu
can directly open().
> +
> + if (fd == NULL) {
> + error_report("Could not open whitelist file: %s", strerror(errno));
> + exit(1);
> + }
> +
> + mon->whitelist = g_hash_table_new_full(g_str_hash,
> + g_str_equal,
> + g_free,
> + NULL);
> +
> + g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
> + g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
> +
> + while (fscanf(fd, "%255s", cmd_name) == 1) {
%255s fits your cmd_name array declaration and stops consuming at either
255 bytes or at the first whitespace encountered, but where do you check
for overflow from a file that passes more than 255 non-whitespace bytes
without a newline? Also, this is a bit sloppy in that it skips all
leading whitespace, rather than ensuring that the user actually passed
newline-separated command names. Does glib provide any interfaces for
more easily reading in an array of lines from a file?
> +++ b/qemu-options.hx
> @@ -3195,12 +3195,16 @@ Like -qmp but uses pretty JSON formatting.
> ETEXI
>
> DEF("mon", HAS_ARG, QEMU_OPTION_mon, \
> - "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]\n", QEMU_ARCH_ALL)
> + "-mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]" \
> + "[,whitelist=file]\n", QEMU_ARCH_ALL)
> STEXI
> -@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]
> +@item -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]][,whitelist=@var{file}]
> @findex -mon
> Setup monitor on chardev @var{name}. @code{pretty} turns on JSON pretty printing
> easing human reading and debugging.
> +The @code{whitelist} option disables all commands except those specified in
> +@var{file}. The file must contain one command name per line. This option is only
> +avaliable in 'control' mode.
s/avaliable/available/
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 01.02.2019 0:03, Eric Blake wrote:
> On 1/31/19 2:26 PM, Julia Suvorova via Qemu-devel wrote:
>> The whitelist option allows to run a reduced monitor with a subset of
>> QMP commands. This allows the monitor to run in secure mode, which is
>> convenient for sending commands via the WebSocket monitor using the
>> web UI. This is planned to be done on micro:bit board.
>>
>> The list of allowed commands should be written to a file, one per line.
>> The command line will look like this:
>> -mon chardev_name,mode=control,whitelist=path_to_file
>>
>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>> ---
>
>>
>> -void monitor_init(Chardev *chr, int flags)
>> +static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
>> +{
>> + char cmd_name[256];
>> + FILE *fd = fopen(whitelist_file, "r");
>
> If you use qemu_open() here (followed by fdopen if you still prefer
> fscanf over read), then you can support "/dev/fdset/NNN" to
> auto-magically support someone passing in the whitelist via an inherited
> file descriptor, rather than having to be somewhere on disk that qemu
> can directly open().
Cool, thanks!
>> +
>> + if (fd == NULL) {
>> + error_report("Could not open whitelist file: %s", strerror(errno));
>> + exit(1);
>> + }
>> +
>> + mon->whitelist = g_hash_table_new_full(g_str_hash,
>> + g_str_equal,
>> + g_free,
>> + NULL);
>> +
>> + g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
>> + g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
>> +
>> + while (fscanf(fd, "%255s", cmd_name) == 1) {
>
> %255s fits your cmd_name array declaration and stops consuming at either
> 255 bytes or at the first whitespace encountered, but where do you check
> for overflow from a file that passes more than 255 non-whitespace bytes
> without a newline? Also, this is a bit sloppy in that it skips all
> leading whitespace, rather than ensuring that the user actually passed
> newline-separated command names. Does glib provide any interfaces for
> more easily reading in an array of lines from a file?
With glib it would be something like that:
g_mapped_file_new_from_fd() : fd -> GMappedFile
g_mapped_file_get_contents() : GMappedFile -> char *
g_strsplit_set() : char * -> char **
All this would need to be released afterwards.
Do you think it would be better?
Best regards, Julia Suvorova.
On Thu, Jan 31, 2019 at 03:03:21PM -0600, Eric Blake wrote:
> On 1/31/19 2:26 PM, Julia Suvorova via Qemu-devel wrote:
> > The whitelist option allows to run a reduced monitor with a subset of
> > QMP commands. This allows the monitor to run in secure mode, which is
> > convenient for sending commands via the WebSocket monitor using the
> > web UI. This is planned to be done on micro:bit board.
> >
> > The list of allowed commands should be written to a file, one per line.
> > The command line will look like this:
> > -mon chardev_name,mode=control,whitelist=path_to_file
> >
> > Signed-off-by: Julia Suvorova <jusual@mail.ru>
> > ---
>
> >
> > -void monitor_init(Chardev *chr, int flags)
> > +static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
> > +{
> > + char cmd_name[256];
> > + FILE *fd = fopen(whitelist_file, "r");
>
> If you use qemu_open() here (followed by fdopen if you still prefer
> fscanf over read), then you can support "/dev/fdset/NNN" to
> auto-magically support someone passing in the whitelist via an inherited
> file descriptor, rather than having to be somewhere on disk that qemu
> can directly open().
>
> > +
> > + if (fd == NULL) {
> > + error_report("Could not open whitelist file: %s", strerror(errno));
> > + exit(1);
> > + }
> > +
> > + mon->whitelist = g_hash_table_new_full(g_str_hash,
> > + g_str_equal,
> > + g_free,
> > + NULL);
> > +
> > + g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
> > + g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
> > +
> > + while (fscanf(fd, "%255s", cmd_name) == 1) {
>
> %255s fits your cmd_name array declaration and stops consuming at either
> 255 bytes or at the first whitespace encountered, but where do you check
> for overflow from a file that passes more than 255 non-whitespace bytes
> without a newline? Also, this is a bit sloppy in that it skips all
> leading whitespace, rather than ensuring that the user actually passed
> newline-separated command names. Does glib provide any interfaces for
> more easily reading in an array of lines from a file?
With glib, normally you'd use:
char *content;
gsize len;
GError *err = NULL;
char **lines;
g_file_get_contents(filename, &contnet, &len, &err)
lines = g_str_split(content, "\n", 0);
g_free(content);
...do something with lines
g_strfreev(lines);
The GIO library provides higher level functions for I/O but we don't
use that in QEMU
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11.02.2019 18:51, Daniel P. Berrangé wrote:
> On Thu, Jan 31, 2019 at 03:03:21PM -0600, Eric Blake wrote:
>> On 1/31/19 2:26 PM, Julia Suvorova via Qemu-devel wrote:
>>> The whitelist option allows to run a reduced monitor with a subset of
>>> QMP commands. This allows the monitor to run in secure mode, which is
>>> convenient for sending commands via the WebSocket monitor using the
>>> web UI. This is planned to be done on micro:bit board.
>>>
>>> The list of allowed commands should be written to a file, one per line.
>>> The command line will look like this:
>>> -mon chardev_name,mode=control,whitelist=path_to_file
>>>
>>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>>> ---
>>
>>>
>>> -void monitor_init(Chardev *chr, int flags)
>>> +static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
>>> +{
>>> + char cmd_name[256];
>>> + FILE *fd = fopen(whitelist_file, "r");
>>
>> If you use qemu_open() here (followed by fdopen if you still prefer
>> fscanf over read), then you can support "/dev/fdset/NNN" to
>> auto-magically support someone passing in the whitelist via an inherited
>> file descriptor, rather than having to be somewhere on disk that qemu
>> can directly open().
>>
>>> +
>>> + if (fd == NULL) {
>>> + error_report("Could not open whitelist file: %s", strerror(errno));
>>> + exit(1);
>>> + }
>>> +
>>> + mon->whitelist = g_hash_table_new_full(g_str_hash,
>>> + g_str_equal,
>>> + g_free,
>>> + NULL);
>>> +
>>> + g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
>>> + g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
>>> +
>>> + while (fscanf(fd, "%255s", cmd_name) == 1) {
>>
>> %255s fits your cmd_name array declaration and stops consuming at either
>> 255 bytes or at the first whitespace encountered, but where do you check
>> for overflow from a file that passes more than 255 non-whitespace bytes
>> without a newline? Also, this is a bit sloppy in that it skips all
>> leading whitespace, rather than ensuring that the user actually passed
>> newline-separated command names. Does glib provide any interfaces for
>> more easily reading in an array of lines from a file?
>
> With glib, normally you'd use:
>
> char *content;
> gsize len;
> GError *err = NULL;
> char **lines;
>
> g_file_get_contents(filename, &contnet, &len, &err)
With g_file_get_contents() I won't be able to do qemu_open() and support
"/dev/fdset/NNN". The workaround seems to me unnecessarily complex.
> lines = g_str_split(content, "\n", 0);
>
> g_free(content);
>
> ...do something with lines
>
> g_strfreev(lines);
>
>
> The GIO library provides higher level functions for I/O but we don't
> use that in QEMU
On Mon, Feb 11, 2019 at 07:15:58PM +0300, Julia Suvorova wrote:
>
>
> On 11.02.2019 18:51, Daniel P. Berrangé wrote:
> > On Thu, Jan 31, 2019 at 03:03:21PM -0600, Eric Blake wrote:
> > > On 1/31/19 2:26 PM, Julia Suvorova via Qemu-devel wrote:
> > > > The whitelist option allows to run a reduced monitor with a subset of
> > > > QMP commands. This allows the monitor to run in secure mode, which is
> > > > convenient for sending commands via the WebSocket monitor using the
> > > > web UI. This is planned to be done on micro:bit board.
> > > >
> > > > The list of allowed commands should be written to a file, one per line.
> > > > The command line will look like this:
> > > > -mon chardev_name,mode=control,whitelist=path_to_file
> > > >
> > > > Signed-off-by: Julia Suvorova <jusual@mail.ru>
> > > > ---
> > >
> > > > -void monitor_init(Chardev *chr, int flags)
> > > > +static void process_whitelist_file(Monitor *mon, const char *whitelist_file)
> > > > +{
> > > > + char cmd_name[256];
> > > > + FILE *fd = fopen(whitelist_file, "r");
> > >
> > > If you use qemu_open() here (followed by fdopen if you still prefer
> > > fscanf over read), then you can support "/dev/fdset/NNN" to
> > > auto-magically support someone passing in the whitelist via an inherited
> > > file descriptor, rather than having to be somewhere on disk that qemu
> > > can directly open().
> > >
> > > > +
> > > > + if (fd == NULL) {
> > > > + error_report("Could not open whitelist file: %s", strerror(errno));
> > > > + exit(1);
> > > > + }
> > > > +
> > > > + mon->whitelist = g_hash_table_new_full(g_str_hash,
> > > > + g_str_equal,
> > > > + g_free,
> > > > + NULL);
> > > > +
> > > > + g_hash_table_add(mon->whitelist, g_strdup("qmp_capabilities"));
> > > > + g_hash_table_add(mon->whitelist, g_strdup("query-commands"));
> > > > +
> > > > + while (fscanf(fd, "%255s", cmd_name) == 1) {
> > >
> > > %255s fits your cmd_name array declaration and stops consuming at either
> > > 255 bytes or at the first whitespace encountered, but where do you check
> > > for overflow from a file that passes more than 255 non-whitespace bytes
> > > without a newline? Also, this is a bit sloppy in that it skips all
> > > leading whitespace, rather than ensuring that the user actually passed
> > > newline-separated command names. Does glib provide any interfaces for
> > > more easily reading in an array of lines from a file?
> >
> > With glib, normally you'd use:
> >
> > char *content;
> > gsize len;
> > GError *err = NULL;
> > char **lines;
> >
> > g_file_get_contents(filename, &contnet, &len, &err)
>
> With g_file_get_contents() I won't be able to do qemu_open() and support
> "/dev/fdset/NNN". The workaround seems to me unnecessarily complex.
Yes, its a question of whether the /dev/fdset/NNN feature is needed or
not. At the very least though you should be using getline() rather than
fscanf so that we don't have a large hardcoded buffer on the stack.
>
> > lines = g_str_split(content, "\n", 0);
> >
> > g_free(content);
> >
> > ...do something with lines
> >
> > g_strfreev(lines);
> >
> >
> > The GIO library provides higher level functions for I/O but we don't
> > use that in QEMU
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.