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

[Daniel P . Berrangé]

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



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