Re: [Qemu-devel] [PATCH] monitor: Add whitelist support for QMP commands

[Stefan Hajnoczi]

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 <address@hidden>
> ---

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
> address@hidden -mon [chardev=]name[,mode=readline|control][,pretty[=on|off]]
> address@hidden -mon 
> [chardev=]name[,mode=readline|control][,pretty[=on|off]][,address@hidden

Your change reminded me that "[chardev=]name" should be
"address@hidden" 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
> address@hidden The file must contain one command name per line. This option 
> is only
> +avaliable in 'control' mode.

s/avaliable/available/

signature.asc
Description: PGP signature