Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

Just spotted one serious problem we need to address. The method 'qemudStartVMDaemon' quoted here is where we set the security label: On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote: > @@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect > return -1; > } > > + /* > + * Set up the security label for the domain here, before doing > + * too much else. > + */ > + if (qemudDomainSetSecurityLabel(conn, driver, vm) < 0) { > + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("Failed to set security label")); > + return -1; > + } > + > if (qemudExtractVersionInfo(emulator, > NULL, > &qemuCmdFlags) < 0) { Which ultimately calls the following method, which sets the context to use for the next exec call. > +static int > +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn, > + virSecurityDriverPtr drv, > + const virSecurityLabelDefPtr secdef) > +{ > + /* TODO: verify DOI */ > + > + if (!STREQ(drv->name, secdef->model)) { > + virSecurityReportError(conn, VIR_ERR_ERROR, > + _("%s: security label driver mismatch: " > + "\'%s\' model configured for domain, but " > + "hypervisor driver is \'%s\'."), > + __func__, secdef->model, drv->name); > + return -1; > + } > + > + if (setexeccon(secdef->label) == -1) { > + virSecurityReportError(conn, VIR_ERR_ERROR, > + _("%s: unable to set security context " > + "'\%s\': %s."), __func__, secdef->label, > + strerror(errno)); > + return -1; > + } > + return 0; > +} The problem is that between the point where we set the exec context, and the place where QEMU is finally exec'd, there is scope for several other programs to be exec'd. Also there are other threads running concurrently, and I'm under whether 'setexeccon' scope is per thread or per process. I think we need to move place where we set the exec context to after the fork() call, ideally to be the very last call made before the actual execve(). We do not currently have an easy way todo this, but I have the exact same problem in my patches to integrate with cgroups - I need to add the new PID to the appropriate cgroup immediately before exec'ing. So i suggest the following patch whichs a generic callback to the virExec() call, so we can implant the neccessary logic after the fork() and just before the real execve(), and safely in the child process. To use this, we'd make qemudStartVM() pass in a virExecHook callback which does the call to qemudDomainSetSecurityLabel() Daniel diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,7 @@ virEnumToString; virEventAddHandle; virEventRemoveHandle; virExec; +virExecWithHook; virSetNonBlock; virFormatMacAddr; virGetHostname; diff --git a/src/util.c b/src/util.c --- a/src/util.c +++ b/src/util.c @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn, const fd_set *keepfd, pid_t *retpid, int infd, int *outfd, int *errfd, - int flags) { + int flags, + virExecHook hook, + void *data) +{ pid_t pid; int null, i, openmax; int pipeout[2] = {-1,-1}; @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn, childerr != childout) close(childerr); + if (hook) + (hook)(data); + if (envp) execve(argv[0], (char **) argv, (char**)envp); else @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn, } int -virExec(virConnectPtr conn, - const char *const*argv, - const char *const*envp, - const fd_set *keepfd, - pid_t *retpid, - int infd, int *outfd, int *errfd, - int flags) { +virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags, + virExecHook hook, + void *data) +{ char *argv_str; if ((argv_str = virArgvToString(argv)) == NULL) { @@ -462,7 +471,21 @@ virExec(virConnectPtr conn, VIR_FREE(argv_str); return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd, - flags); + flags, hook, data); +} + +int +virExec(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + pid_t *retpid, + int infd, int *outfd, int *errfd, + int flags) +{ + return virExecWithHook(conn, argv, envp, keepfd, retpid, + infd, outfd, errfd, + flags, NULL, NULL); } static int @@ -580,7 +603,7 @@ virRun(virConnectPtr conn, if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE)) < 0) { + VIR_EXEC_NONE, NULL, NULL)) < 0) { ret = execret; goto error; } diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -40,6 +40,21 @@ enum { int virSetNonBlock(int fd); +/* This will execute in the context of the first child + * immediately after fork() */ +typedef int (*virExecHook)(void *data); + +int virExecWithHook(virConnectPtr conn, + const char *const*argv, + const char *const*envp, + const fd_set *keepfd, + int *retpid, + int infd, + int *outfd, + int *errfd, + int flags, + virExecHook hook, + void *data); int virExec(virConnectPtr conn, const char *const*argv, const char *const*envp,

-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkmezMkACgkQrlYvE4MpobMM1ACg1TYPE0OyLzPHAohvx0LRva4Z wXkAoLUHS+yJMx4A0C/xz7tVs2Np3NLL =uu9y -----END PGP SIGNATURE-----