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é <berra...@redhat.com> > 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 :|