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,
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list