Re: [Qemu-devel] [RFC v2 04/12] Add vhost-user-backend

[Daniel P . Berrangé]

On Tue, Jun 12, 2018 at 04:53:54PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jun 8, 2018 at 10:43 AM, Daniel P. Berrangé <address@hidden> wrote:
> > On Fri, Jun 08, 2018 at 12:34:15AM +0200, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Mon, Jun 4, 2018 at 11:36 AM, Daniel P. Berrangé <address@hidden> wrote:
> >> > On Fri, Jun 01, 2018 at 06:27:41PM +0200, Marc-André Lureau wrote:
> >> >> Create a vhost-user-backend object that holds a connection to a
> >> >> vhost-user backend and can be referenced from virtio devices that
> >> >> support it. See later patches for input & gpu usage.
> >> >>
> >> >> A chardev can be specified to communicate with the vhost-user backend,
> >> >> ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> >> >> vhost-user-backend,id=vuid,chardev=char0.
> >> >>
> >> >> Alternatively, an executable with its arguments may be given as 'cmd'
> >> >> property, ex: -object
> >> >> vhost-user-backend,id=vui,cmd="./vhost-user-input /dev/input..". The
> >> >> executable is then spawn and, by convention, the vhost-user socket is
> >> >> passed as fd=3. It may be considered a security breach to allow
> >> >> creating processes that may execute arbitrary executables, so this may
> >> >> be restricted to some known executables (via signature etc) or
> >> >> directory.
> >> >
> >> > Passing a binary and args as a string blob.....
> >> >
> >> >> +static int
> >> >> +vhost_user_backend_spawn_cmd(VhostUserBackend *b, int vhostfd, Error 
> >> >> **errp)
> >> >> +{
> >> >> +    int devnull = open("/dev/null", O_RDWR);
> >> >> +    pid_t pid;
> >> >> +
> >> >> +    assert(!b->child);
> >> >> +
> >> >> +    if (!b->cmd) {
> >> >> +        error_setg_errno(errp, errno, "Missing cmd property");
> >> >> +        return -1;
> >> >> +    }
> >> >> +    if (devnull < 0) {
> >> >> +        error_setg_errno(errp, errno, "Unable to open /dev/null");
> >> >> +        return -1;
> >> >> +    }
> >> >> +
> >> >> +    pid = qemu_fork(errp);
> >> >> +    if (pid < 0) {
> >> >> +        close(devnull);
> >> >> +        return -1;
> >> >> +    }
> >> >> +
> >> >> +    if (pid == 0) { /* child */
> >> >> +        int fd, maxfd = sysconf(_SC_OPEN_MAX);
> >> >> +
> >> >> +        dup2(devnull, STDIN_FILENO);
> >> >> +        dup2(devnull, STDOUT_FILENO);
> >> >> +        dup2(vhostfd, 3);
> >> >> +
> >> >> +        signal(SIGINT, SIG_IGN);
> >> >
> >> > Why ignore SIGINT ?  Surely we want this extra process to be killed
> >> > someone ctrl-c's the parent QEMU.
> >>
> >> leftover, removed
> >>
> >> >
> >> >> +
> >> >> +        for (fd = 4; fd < maxfd; fd++) {
> >> >> +            close(fd);
> >> >> +        }
> >> >> +
> >> >> +        execlp("/bin/sh", "sh", "-c", b->cmd, NULL);
> >> >
> >> > ...which is then interpreted by the shell is a recipe for security
> >> > flaws. There needs to be a way to pass the command + arguments
> >> > to QEMU as an argv[] we can directly exec without involving the
> >> > shell.
> >> >
> >>
> >> For now, I use g_shell_parse_argv(). Do you have a better idea?
> >
> > Accept individual args at the cli level is far preferrable - we don't
> > want anything to be parsing shell strings:
> >
> >  
> > vhost-user-backend,id=vui,binary=/sbin/vhost-user-input,arg=/dev/input,arg=foo,arg=bar
> 
> Object arguments are populated in a dictionary. Only the last value
> specified is used.

Sigh, yes, this goes back to -object not having a full syntax for
non-scalar properties.

The quickest short term solution to this is to get the support for
"-object json:..."  working probably, then it will be just as
expressive as QMP's object_add.

> g_shell_parse_argv() isn't that scary imho. But if there is a
> blacklist of functions, it would be worth to have them listed
> somewhere.

We don't want to be using shell at all - we are going to be directly
passing the binary to execve(), so shell won't be involved. The
g_shell_parse_argv() is not simply splitting a string based on
whitespace, it is intepreting shell quoting rules, comments and
more. IOW, it is mangling args we'll be passing to execve() based
on shell rules that we're not intending to be using. It is simply
wrong to use g_shell_parse_argv() for this.


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 :|