[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v2 04/12] Add vhost-user-backend
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [RFC v2 04/12] Add vhost-user-backend |
Date: |
Mon, 4 Jun 2018 10:36:53 +0100 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
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.
> +
> + 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.
> + _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.
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 :|
[Qemu-devel] [RFC v2 06/12] vhost-user: add vhost_user_input_get_config(), Marc-André Lureau, 2018/06/01
[Qemu-devel] [RFC v2 07/12] libvhost-user: export vug_source_new, Marc-André Lureau, 2018/06/01