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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Fri, Jun 8, 2018 at 10:43 AM, Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@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

Object arguments are populated in a dictionary. Only the last value
specified is used.

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.