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

[Marc-André Lureau]

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?

>> +        _exit(1);
>> +    }
>> +
>> +    b->child = QIO_CHANNEL(qio_channel_command_new_pid(devnull, devnull, 
>> pid));
>
> Overall this method overall duplicates much of the
> qio_channel_command_new_argv(), though you do have a few differences.
>
> I'd prefer if we could make qio_channel_command_new_argv more flexible to
> handle these extra needs though.
>

Ok I added a pre-exec callback for the extra dup2() & close().

Thanks,




-- 
Marc-André Lureau