| Message ID | 1509988303.22094.8.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 06, 2017 at 06:11:43PM +0100, Patrick Ohly wrote: > Hello! > > When Amarnath proposed the initial patch set for interacting with swtpm > without relying on CUSE, letting QEMU invoke swtpm was part of the > patch series. When using some virtual machine management framework like > virt-manager that might not be necessary. But our goal was to use > QEMU+swtpm to test TPM support in Yocto, and there QEMU is started by a > simple wrapper script where handling additional processes would be > awkward. > > To achieve that goal without having to make that script more complex, > I'd like to propose that with some minor (?) changes, a chardev could > also be configured to use an external command. As a proof of concept > I'm attaching a patch which modifies char-socket.c accordingly. Personally I'm not a huge fan of adding more ways for QEMU to spawn external commands. QEMU is already a very large & complex beast which is hard to confine, and when QEMU spawns a command it is hard to lock it down to any useful extent without us adding alot of extra features to QEMU. Most stuff we've been doing in QEMU has tended to go in the opposite direction of facilitating integration with helpers spawned by a 3rd party, whether it be the mgmt app that launches QEMU or something else. cf vhost-user where we just expose a normal chardev unix socket and assume someone/thing else spawned the service on the other end, or the new approach to host FS passthrough where we expose vsock and run NFS server externally. I can see the argument about it making QEMU easier to use, and those who care about security aren't forced to use this new feature. It none the less has a cost on maintainers and existance of these features does reflect on QEMU's security reputation even if many don't use it. > +static void chardev_open_socket_cmd(Chardev *chr, > + const char *cmd, > + Error **errp) > +{ > + int fds[2] = { -1, -1 }; > + QIOChannelSocket *sioc = NULL; > + pid_t pid = -1; > + const char *argv[] = { "/bin/sh", "-c", cmd, NULL }; > + > + if (socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, fds)) { > + error_setg_errno(errp, errno, "Error creating socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC)"); > + goto error; > + } > + > + pid = qemu_fork(errp); > + if (pid < 0) { > + goto error; > + } > + > + if (!pid) { > + /* child */ > + dup2(fds[1], STDIN_FILENO); > + execv(argv[0], (char * const *)argv); > + _exit(1); > + } > + > + /* > + * Hand over our end of the socket pair to the qio channel. > + * TODO: We don't reap the child at the moment. > + */ > + sioc = qio_channel_socket_new_fd(fds[0], errp); NB, you've mostly re-invented qio_channel_command_new_spawn() here, though you're using a socketpair instead of pipe, which has the slight benefit of meaning we can do FD passing across the external command. If the latter is why you chose socketpair, then it would be nice to improve QIOChannelCommand Regards, Daniel
On Mon, 2017-11-06 at 17:26 +0000, Daniel P. Berrange wrote: > I can see the argument about it making QEMU easier to use, and those > who care about security aren't forced to use this new feature. It > none the less has a cost on maintainers and existance of these > features does reflect on QEMU's security reputation even if many > don't use it. With Yocto we really don't have much choice: we need a patch like this because the alternative (introducing support for spawning and stopping swtpm and then passing the right parameters to QEMU) is way more complex. So if this patch isn't acceptable to QEMU upstream, then I will keep it as simple as possible and propose it as a local patch in Yocto. But that then won't help others who might be using QEMU in a similar situation. > > +static void chardev_open_socket_cmd(Chardev *chr, > > + const char *cmd, > > + Error **errp) > > +{ > > + int fds[2] = { -1, -1 }; > > + QIOChannelSocket *sioc = NULL; > > + pid_t pid = -1; > > + const char *argv[] = { "/bin/sh", "-c", cmd, NULL }; > > + > > + if (socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, fds)) { > > + error_setg_errno(errp, errno, "Error creating > > socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC)"); > > + goto error; > > + } > > + > > + pid = qemu_fork(errp); > > + if (pid < 0) { > > + goto error; > > + } > > + > > + if (!pid) { > > + /* child */ > > + dup2(fds[1], STDIN_FILENO); > > + execv(argv[0], (char * const *)argv); > > + _exit(1); > > + } > > + > > + /* > > + * Hand over our end of the socket pair to the qio channel. > > + * TODO: We don't reap the child at the moment. > > + */ > > + sioc = qio_channel_socket_new_fd(fds[0], errp); > > NB, you've mostly re-invented qio_channel_command_new_spawn() here, > though you're using a socketpair instead of pipe, which has the > slight benefit of meaning we can do FD passing across the external > command. Yes, that's exactly the reason. swtpm depends on a bidirectional stream with support for FD passing. I should have added that an option for a new "-charset cmd" might be to choose two uni-direction pipes connected to stdin/stdout of the command. That would be useful for some other commands. > If the latter is why you chose socketpair, then it would > be nice to improve QIOChannelCommand Yes, that would be better. But before touching too many different files and thus increasing the risk of merge conflicts (which is an issue when maintaining the patch downstream) I'd like to get a feeling for whether it's worth enhancing the patch to make it suitable for upstream or whether there are strong reasons to never include anything like it.
On Mon, Nov 06, 2017 at 10:02:05PM +0100, Patrick Ohly wrote: > On Mon, 2017-11-06 at 17:26 +0000, Daniel P. Berrange wrote: > > I can see the argument about it making QEMU easier to use, and those > > who care about security aren't forced to use this new feature. It > > none the less has a cost on maintainers and existance of these > > features does reflect on QEMU's security reputation even if many > > don't use it. > > With Yocto we really don't have much choice: we need a patch like this > because the alternative (introducing support for spawning and stopping > swtpm and then passing the right parameters to QEMU) is way more > complex. So if this patch isn't acceptable to QEMU upstream, then I > will keep it as simple as possible and propose it as a local patch in > Yocto. I don't really buy this argument. Any distro's core job is the ability to start/stop/manage processes. Saying yocto is unable to manage runing of swtpm is really dubious - it is simply a choice to declare that it is QEMU's job. Regards, Daniel
On Tue, 2017-11-07 at 09:23 +0000, Daniel P. Berrange wrote: > On Mon, Nov 06, 2017 at 10:02:05PM +0100, Patrick Ohly wrote: > > On Mon, 2017-11-06 at 17:26 +0000, Daniel P. Berrange wrote: > > > I can see the argument about it making QEMU easier to use, and > > > those > > > who care about security aren't forced to use this new feature. It > > > none the less has a cost on maintainers and existance of these > > > features does reflect on QEMU's security reputation even if many > > > don't use it. > > > > With Yocto we really don't have much choice: we need a patch like > > this > > because the alternative (introducing support for spawning and > > stopping > > swtpm and then passing the right parameters to QEMU) is way more > > complex. So if this patch isn't acceptable to QEMU upstream, then I > > will keep it as simple as possible and propose it as a local patch > > in > > Yocto. > > I don't really buy this argument. Any distro's core job is the > ability to start/stop/manage processes. Saying yocto is unable to > manage runing of swtpm is really dubious - it is simply a choice to > declare that it is QEMU's job. Yocto isn't really a distro. It's mostly a build system and meta data that can be used to build custom distros. The use of qemu+swtpm is during testing on whatever distro the developer has chosen for his build machine. Yocto could build libvirt for that host to manage QEMU, but given the choice between that or enhancing the Yocto QEMU support code and patching QEMU, patching IMHO is the easier and cleaner solution - for Yocto, at least. I'll proceed with proposing the patch as a Yocto-specific modification for QEMU.
From 85e2456983e8c284d304b8fcd7e9af8d37820cff Mon Sep 17 00:00:00 2001 From: Patrick Ohly <patrick.ohly@intel.com> Date: Fri, 27 Oct 2017 15:23:35 +0200 Subject: [PATCH 1/1] chardev: connect socket to a spawned command The command is started in a shell (sh -c) with stdin connect to qemu via a Unix domain stream socket. Qemu then exchanges data via its own end of the socket, just like it normally does. "-chardev socket" supports some ways of connecting via protocols like telnet, but that is only a subset of the functionality supported by tools socat. To use socat instead, for example to connect via a socks proxy, use: -chardev 'socket,cmd=exec socat FD:0 SOCKS4A:socks-proxy.localdomain:example.com:9999,,socksuser=nobody' Beware that commas in the command must be escaped as double commas. Another usage is starting swtpm from inside qemu. swtpm will automatically shut down once it looses the connection to the parent qemu, so there is no risk of lingering processes: -chardev 'socket,id=chrtpm0,cmd=exec swtpm socket --terminate --ctrl type=unixio,,clientfd=0 --tpmstate dir=... --log file=swtpm.log' \ -tpmdev emulator,id=tpm0,chardev=chrtpm0 \ -device tpm-tis,tpmdev=tpm0 Signed-off-by: Patrick Ohly <patrick.ohly@intel.com> %% original patch: chardev-connect-socket-to-a-spawned-command.patch --- chardev/char-socket.c | 88 ++++++++++++++++++++++++++++++++++++++++++++------- chardev/char.c | 3 ++ qapi-schema.json | 5 +++ 3 files changed, 84 insertions(+), 12 deletions(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 1ae730a4..42a67f88 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -854,6 +854,58 @@ static gboolean socket_reconnect_timeout(gpointer opaque) return false; } +static void chardev_open_socket_cmd(Chardev *chr, + const char *cmd, + Error **errp) +{ + int fds[2] = { -1, -1 }; + QIOChannelSocket *sioc = NULL; + pid_t pid = -1; + const char *argv[] = { "/bin/sh", "-c", cmd, NULL }; + + if (socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, fds)) { + error_setg_errno(errp, errno, "Error creating socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC)"); + goto error; + } + + pid = qemu_fork(errp); + if (pid < 0) { + goto error; + } + + if (!pid) { + /* child */ + dup2(fds[1], STDIN_FILENO); + execv(argv[0], (char * const *)argv); + _exit(1); + } + + /* + * Hand over our end of the socket pair to the qio channel. + * TODO: We don't reap the child at the moment. + */ + sioc = qio_channel_socket_new_fd(fds[0], errp); + if (!sioc) { + goto error; + } + fds[0] = -1; + + g_free(chr->filename); + chr->filename = g_strdup_printf("cmd:%s", cmd); + tcp_chr_new_client(chr, sioc); + + error: + if (fds[0] >= 0) { + close(fds[0]); + } + if (fds[1] >= 0) { + close(fds[1]); + } + if (sioc) { + object_unref(OBJECT(sioc)); + } +} + static void qmp_chardev_open_socket(Chardev *chr, ChardevBackend *backend, bool *be_opened, @@ -861,12 +913,13 @@ static void qmp_chardev_open_socket(Chardev *chr, { SocketChardev *s = SOCKET_CHARDEV(chr); ChardevSocket *sock = backend->u.socket.data; - bool do_nodelay = sock->has_nodelay ? sock->nodelay : false; - bool is_listen = sock->has_server ? sock->server : true; - bool is_telnet = sock->has_telnet ? sock->telnet : false; - bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false; - bool is_waitconnect = sock->has_wait ? sock->wait : false; - int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0; + const char *cmd = sock->cmd; + bool do_nodelay = cmd ? false : sock->has_nodelay ? sock->nodelay : false; + bool is_listen = cmd ? false : sock->has_server ? sock->server : true; + bool is_telnet = cmd ? false : sock->has_telnet ? sock->telnet : false; + bool is_tn3270 = cmd ? false : sock->has_tn3270 ? sock->tn3270 : false; + bool is_waitconnect = cmd ? false : sock->has_wait ? sock->wait : false; + int64_t reconnect = cmd ? 0 : sock->has_reconnect ? sock->reconnect : 0; QIOChannelSocket *sioc = NULL; SocketAddress *addr; @@ -911,11 +964,11 @@ static void qmp_chardev_open_socket(Chardev *chr, qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE); /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */ - if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) { + if (cmd || addr->type == SOCKET_ADDRESS_TYPE_UNIX) { qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS); } - /* be isn't opened until we get a connection */ + /* be isn't opened until we get a connection or fork a command */ *be_opened = false; update_disconnected_filename(s); @@ -928,7 +981,12 @@ static void qmp_chardev_open_socket(Chardev *chr, s->reconnect_time = reconnect; } - if (s->reconnect_time) { + if (cmd) { + chardev_open_socket_cmd(chr, cmd, errp); + + /* everything ready (or failed permanently) before we return */ + *be_opened = true; + } else if (s->reconnect_time) { sioc = qio_channel_socket_new(); tcp_chr_set_client_ioc_name(chr, sioc); qio_channel_socket_connect_async(sioc, s->addr, @@ -987,11 +1045,16 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, const char *host = qemu_opt_get(opts, "host"); const char *port = qemu_opt_get(opts, "port"); const char *tls_creds = qemu_opt_get(opts, "tls-creds"); + const char *cmd = qemu_opt_get(opts, "cmd"); SocketAddressLegacy *addr; ChardevSocket *sock; backend->type = CHARDEV_BACKEND_KIND_SOCKET; - if (!path) { + if (cmd && path) { + error_setg(errp, "chardev: socket: cmd and path are mutually exclusive"); + return; + } + if (!path && !cmd) { if (!host) { error_setg(errp, "chardev: socket: no host given"); return; @@ -1023,13 +1086,14 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, sock->has_reconnect = true; sock->reconnect = reconnect; sock->tls_creds = g_strdup(tls_creds); + sock->cmd = g_strdup(cmd); addr = g_new0(SocketAddressLegacy, 1); - if (path) { + if (path || cmd) { UnixSocketAddress *q_unix; addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX; q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1); - q_unix->path = g_strdup(path); + q_unix->path = cmd ? g_strdup_printf("cmd:%s", cmd) : g_strdup(path); } else { addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET; addr->u.inet.data = g_new(InetSocketAddress, 1); diff --git a/chardev/char.c b/chardev/char.c index 5d283b90..ccb329d4 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -782,6 +782,9 @@ QemuOptsList qemu_chardev_opts = { .name = "path", .type = QEMU_OPT_STRING, },{ + .name = "cmd", + .type = QEMU_OPT_STRING, + },{ .name = "host", .type = QEMU_OPT_STRING, },{ diff --git a/qapi-schema.json b/qapi-schema.json index 78a00bc8..790b026d 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -5004,6 +5004,10 @@ # # @addr: socket address to listen on (server=true) # or connect to (server=false) +# @cmd: command to run via "sh -c" with stdin as one end of +# a AF_UNIX SOCK_DSTREAM socket pair. The other end +# is used by the chardev. Either an addr or a cmd can +# be specified, but not both. # @tls-creds: the ID of the TLS credentials object (since 2.6) # @server: create server socket (default: true) # @wait: wait for incoming connection on server @@ -5021,6 +5025,7 @@ # Since: 1.4 ## { 'struct': 'ChardevSocket', 'data': { 'addr' : 'SocketAddressLegacy', + '*cmd' : 'str', '*tls-creds' : 'str', '*server' : 'bool', '*wait' : 'bool', -- 2.11.0