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

[Daniel P . Berrangé]

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