[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Daniel P. Berrange wrote:
> 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,
> 
> 
I have an update to the original patch which includes a test program and
 a changelog, but I guess I need to wait for this patch to be approved.

http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch
-----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-----


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]