Trying to use virlockd to lock metadata turns out to be too big
gun. Since we will always spawn a separate process for relabeling
we are safe to use thread unsafe POSIX locks and take out
virtlockd completely out of the picture.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

When metadata locking is enabled that means the security commit
processing will be run in a fork similar to how namespaces use fork()'s
for processing. This is done to ensure libvirt can properly and
synchronously modify the metadata to store the original owner data.

Since fork()'s (e.g. virFork) have been seen as a performance bottleneck
being able to disable them allows the admin to choose whether the
performance 'hit' is worth the extra 'security' of being able to
remember the original owner of a lock.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Return -1 and report an error message if no transaction is set and
virSecuritySELinuxTransactionCommit is called.

The function description of virSecuritySELinuxTransactionCommit says:

  "Also it is considered as error if there's no transaction set and this
   function is called."
Signed-off-by: NMarc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: NBoris Fiuczynski <fiuczy@linux.ibm.com>

In 4674fc6a I've implemented transactions for selinux driver.
Well, now that I am working in this area I've noticed a subtle
bug: @ret is initialized to 0 instead of -1. Facepalm.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NMarc Hartmayer <mhartmay@linux.ibm.com>

This shouldn't be needed per-se. Security manager shouldn't
disappear during transactions - it's immutable. However, it
doesn't hurt to grab a reference either - transaction code uses
it after all.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NMarc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

This array is allocated in virSecuritySELinuxContextListAppend()
but never freed. This commit is essentially the same as ca250269.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Lock all the paths we want to relabel to mutually exclude other
libvirt daemons.

The only hitch here is that directories can't be locked.
Therefore, when relabeling a directory do not lock it (this
happens only when setting up some domain private paths anyway,
e.g. huge pages directory).
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

So far the whole transaction handling is done
virSecuritySELinuxSetFileconHelper(). This needs to change for
the sake of security label remembering and locking. Otherwise we
would be locking a path when only appending it to transaction
list and not when actually relabelling it.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

Firstly, the following code pattern is harder to follow:

  if (func() < 0) {
      error();
  } else {
      /* success */
  }

We should put 'goto cleanup' into the error branch and move the
else branch one level up.
Secondly, 'rc' should really be named 'ret' because it holds
return value of the function. Not some intermediate value.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

This label is used in both successful and error paths. Therefore
it should be named 'cleanup' and not 'err'.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

It will be desirable to run transactions more often than we
currently do. Even if the domain we're relabeling the paths for
does not run in a namespace. If that's the case, there is no need
to fork() as we are already running in the right namespace. To
differentiate whether transaction code should fork() or not the
@pid argument now accepts -1 (which means do not fork).
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

In this patch we label the swtpm process with SELinux labels. We give it the
same label as the QEMU process has. We label its state directory and files
as well. We restore the old security labels once the swtpm has terminated.

The file and process labels now look as follows:

Directory: /var/lib/libvirt/swtpm

[root@localhost swtpm]# ls -lZ
total 4
rwx------. 2 tss  tss  system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr  5 16:46 testvm

[root@localhost testvm]# ls -lZ
total 8
-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr  5 16:46 tpm-00.permall

The log in /var/log/swtpm/libvirt/qemu is labeled as follows:

-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr  5 16:46 vtpm.log

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0  0.0 28172  3892 ?        Ss   16:57   0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0  0.0 3096704 48500 ?    Sl   16:57   3:28 /bin/qemu-system-x86_64 [..]
Signed-off-by: NStefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Extend the DAC and SELinux modules with support for the tpm-emulator.
We label the Unix socket that QEMU connects to after starting swtmp
with DAC and SELinux labels. We do not have to restore the labels in
this case since the tpm-emulator will remove the Unix socket when it
terminates.
Signed-off-by: NStefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

This patch adds support for an external swtpm TPM emulator. The XML for
this type of TPM looks as follows:

 <tpm model='tpm-tis'>
   <backend type='emulator'/>
 </tpm>

The XML will currently only define a TPM 1.2.

Extend the documentation.

Add a test case testing the XML parser and formatter.
Signed-off-by: NStefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>
Reviewed-by: NJán Tomko <jtomko@redhat.com>

virSecurityManagerDomainSetPathLabel is used to make a path known
to the security modules, but today is used interchangably for
 - paths to files/dirs to be accessed directly
 - paths to a dir, but the access will actually be to files therein

Depending on the security module it is important to know which of
these types it will be.

The argument allowSubtree augments the call to the implementations of
DomainSetPathLabel that can - per security module - decide if extra
actions shall be taken.

For now dac/selinux handle this as before, but apparmor will make
use of it to add a wildcard to the path that was passed.
Signed-off-by: NChristian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

SELinux and DAC drivers already have both functions but they were not
exported as public API of security manager.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Export the existing DAC and SELinux for separate use and introduce
functions for stack, nop and the security manager.

Until now we ignored user-provided backing chains and while detecting
the code inherited labels of the parent device. With user provided
chains we should keep this functionality, so label of the parent image
in the backing chain will be applied if an image-specific label is not
present.

virSecuritySELinuxSetImageLabelInternal assigns different labels to
backing chain members than to the parent image. This was done via the
'first' flag. Convert it to passing in pointer to the parent
virStorageSource. This will allow us to use the parent virStorageSource
in further changes.

Add helpers that will simplify checking if a backing file is valid or
whether it has backing store. The helper virStorageSourceIsBacking
returns true if the given virStorageSource is a valid backing store
member. virStorageSourceHasBacking returns true if the virStorageSource
has a backing store child.

Adding these functions creates a central points for further refactors.

Our commit e13e8808 was way too generic. Currently, virtlogd is
used only for chardevs type of file and nothing else. True, we
must not relabel the path in this case, but we have to in all
other cases. For instance, if you want to have a physical console
attached to your guest:

    <console type='dev'>
      <source path='/dev/ttyS0'/>
      <target type='virtio' port='1'/>
    </console>

Starting such domain fails because qemu doesn't have access to
/dev/ttyS0 because we haven't relabelled the path.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

In the case that virtlogd is used as stdio handler we pass to QEMU
only FD to a PIPE connected to virtlogd instead of the file itself.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Reviewed-by: NMartin Kletzander <mkletzan@redhat.com>

Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
Reviewed-by: NJohn Ferlan <jferlan@redhat.com>

If the first console is just a copy of the first serial device we
don't need to iterate over the same device twice in order to perform
actions like security labeling, cgroup configuring, etc.

Currently only security SELinux manager was aware of this fact.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Namely, this patch is about virMediatedDeviceGetIOMMUGroup{Dev,Num}
functions. There's no compelling reason why these functions should take
an object, on the contrary, having to create an object every time one
needs to query the IOMMU group number, discarding the object afterwards,
seems odd.
Signed-off-by: NErik Skultety <eskultet@redhat.com>

This patch updates all of our security driver to start labeling the
VFIO IOMMU devices under /dev/vfio/ as well.
Signed-off-by: NErik Skultety <eskultet@redhat.com>

A mediated device will be identified by a UUID (with 'model' now being
a mandatory <hostdev> attribute to represent the mediated device API) of
the user pre-created mediated device. We also need to make sure that if
user explicitly provides a guest address for a mdev device, the address
type will be matching the device API supported on that specific mediated
device and error out with an incorrect XML message.

The resulting device XML:
<devices>
  <hostdev mode='subsystem' type='mdev' model='vfio-pci'>
    <source>
      <address uuid='c2177883-f1bb-47f0-914d-32a22e3a8804'>
    </source>
  </hostdev>
</devices>
Signed-off-by: NErik Skultety <eskultet@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

When domain is being started up, we ought to relabel the host
side of NVDIMM so qemu has access to it.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

The problem is in the way how the list item is created prior to
appending it to the transaction list - the @path argument is just a
shallow copy instead of deep copy of the hostdev device's path.
Unfortunately, the hostdev devices from which the @path is extracted, in
order to add them into the transaction list, are only temporary and
freed before the buildup of the qemu namespace, thus making the @path
attribute in the transaction list NULL, causing 'permission denied' or
'double free' or 'unknown cause' errors.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773Signed-off-by: NErik Skultety <eskultet@redhat.com>

There are still some systems out there that have broken
setfilecon*() prototypes. Instead of taking 'const char *tcon' it
is taking 'char *tcon'. The function should just set the context,
not modify it.

We had been bitten with this problem before which resulted in
292d3f2d and subsequently b109c097. However, with one my latest
commits (4674fc6a) I've changed the type of @tcon variable to
'const char *' which results in build failure on the systems from
above.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

It doesn't make much sense to have two different prefix for
functions within the same driver.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Ensure that the vhost-scsi wwpn information is passed to the
different security policies.
Signed-off-by: NEric Farman <farman@linux.vnet.ibm.com>

We already have a "scsi" hostdev subsys type, which refers to a single
LUN that is passed through to a guest.  But what of things where
multiple LUNs are passed through via a single SCSI HBA, such as with
the vhost-scsi target?  Create a new hostdev subsys type that will
carry this.
Signed-off-by: NEric Farman <farman@linux.vnet.ibm.com>

As was suggested in an earlier review comment[1], we can
catch some additional code points by cleaning up how we use the
hostdev subsystem type in some switch statements.

[1] End of https://www.redhat.com/archives/libvir-list/2016-September/msg00399.htmlSigned-off-by: NEric Farman <farman@linux.vnet.ibm.com>
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Use a pointer and the virDomainChrSourceDefNew() function in order to
allocate the structure for _virDomainSmartcardDef.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Change the virDomainChrDef to use a pointer to 'source' and allocate
that pointer during virDomainChrDefNew.

This has tremendous "fallout" in the rest of the code which mainly
has to change source.$field to source->$field.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>