[libvirt] [PATCH v2 01/11] security: Always spawn process for transactions

[John Ferlan]

On 10/11/18 8:03 AM, Michal Privoznik wrote:
> In the next commit the virSecurityManagerMetadataLock() is going
> to be turned thread unsafe. Therefore, we have to spawn a
> separate process for it. Always.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c     | 12 ++++++------
>  src/security/security_selinux.c | 12 ++++++------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index da4a6c72fe..21db3b9684 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
^^^^

The virSecurityDACTransactionCommit description would need some
adjustment.... Well a lot of adjustment... and the caller
virSecurityManagerTransactionCommit would also require some adjustment.

>          goto cleanup;
>      }
>  
> -    if ((pid == -1 &&
> -         virSecurityDACTransactionRun(pid, list) < 0) ||
> -        (pid != -1 &&
> -         virProcessRunInMountNamespace(pid,
> -                                       virSecurityDACTransactionRun,
> -                                       list) < 0))
> +    if (pid == -1)
> +        pid = getpid();
> +
> +    if (virProcessRunInMountNamespace(pid,
> +                                      virSecurityDACTransactionRun,
> +                                      list) < 0)

As described in the v1 series cover letter, namespaces would need to be
enabled for this to work. Whether they are enabled true everywhere,
who's to say. I always assumed namespaces were used for a somewhat
narrow band of things and didn't pay that close attention to them.

I will note that will a couple of well placed VIR_WARN()'s in each of
these functions, I can see DAC and SELinux transactionCommit's getting
called. Before the guest actually starts they're called with -1, then
once it starts there's a single call with the domain PID. So I guess I'm
using NS and didn't know.

However, looking at the qemu.conf namespace setting I note things like
"privileged" and "!defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)"
So this all then assumes MountNamespace is being used. And what if it
isn't? Does everything that's security related start failing? and the
only way to back that out is revert this?

I guess, what I don't understand is why this usage pattern cannot be
limited to meta locking. That is, if we're metalocking, then add the
pid; otherwise, business as usual.  At least that limits the scope.

FWIW:
In my "limited" testing, I have a guest that failed to start with this
code running:

# virsh start f23
error: Failed to start domain f23
error: internal error: Child process (6767) unexpected fatal signal 11

...

in the code running in the debugger I get:

2018-10-11 18:38:28.491+0000: 5088: error : virProcessWait:274 :
internal error: Child process (6220) unexpected fatal signal 11
2018-10-11 18:38:28.732+0000: 6211: error : learnIPAddressThread:688 :
encountered an error on interface vnet0 index 11: Invalid argument

I narrowed the failure down to my iSCSI disks such as:

    <disk type='network' device='disk'>
      <driver name='qemu' type='raw'/>
      <source protocol='iscsi'
name='iqn.2013-12.com.example:iscsi-chap-netpool/3'>
        <host name='192.168.122.1' port='3260'/>
        <auth username='redhat'>
          <secret type='iscsi' usage='libvirtiscsi'/>
        </auth>
      </source>
      <target dev='vdc' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x0a'
function='0x0'/>
    </disk>

which doesn't make sense given the error, but who really knows... I
don't have the patience to debug much further either. Even stranger, the
same guest with <hostdev>:

    <hostdev mode='subsystem' type='scsi' managed='no'>
      <source protocol='iscsi'
name='iqn.2013-12.com.example:iscsi-chap-netpool/2'>
        <host name='192.168.122.1' port='3260'/>
        <auth username='redhat'>
          <secret type='iscsi' usage='libvirtiscsi'/>
        </auth>
      </source>
      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
    </hostdev>

would work.

My selinux is "Permissive".

John

>          goto cleanup;
>  
>      ret = 0;
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 467d1e6bfe..3c847d8dcb 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1103,12 +1103,12 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> -    if ((pid == -1 &&
> -         virSecuritySELinuxTransactionRun(pid, list) < 0) ||
> -        (pid != -1 &&
> -         virProcessRunInMountNamespace(pid,
> -                                       virSecuritySELinuxTransactionRun,
> -                                       list) < 0))
> +    if (pid == -1)
> +        pid = getpid();
> +
> +    if (virProcessRunInMountNamespace(pid,
> +                                      virSecuritySELinuxTransactionRun,
> +                                      list) < 0)
>          goto cleanup;
>  
>      ret = 0;
>