This function is no longer needed because after previous commits
it's just an alias to virSecuritySELinuxSetFilecon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Now, that we don't need to remember if setting context is
'optional' (the argument only made
virSecuritySELinuxSetFileconImpl() return a different success
code), we can drop it from the _virSecuritySELinuxContextItem
structure as we don't need to remember it in transactions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There is no real difference between
virSecuritySELinuxSetFilecon() and
virSecuritySELinuxSetFileconOptional(). Drop the latter in favour
of the former.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The only thing that the @optional argument does is that it makes
the function return 1 instead of 0 if setting SELinux context
failed in a non-critical fashion. Drop the argument then and
return 1 in that case. This enables caller to learn if SELinux
context was set or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
If user has two domains, each have the same disk (configured for
RW) but each runs with different seclabel then we deny start of
the second domain because in order to do that we would need to
relabel the disk but that would cut the first domain off. Even if
we did not do that, qemu would fail to start because it would be
unable to lock the disk image for the second time. So far, this
behaviour is expected. But what is not expected is that we
increase the refcounter in XATTRs and leave it like that.
What happens is that when the second domain starts,
virSecuritySetRememberedLabel() is called, and since there are
XATTRs from the first domain it increments the refcounter and
returns it (refcounter == 2 at this point). Then callers
(virSecurityDACSetOwnership() and
virSecuritySELinuxSetFileconHelper()) realize that refcounter is
greater than 1 and desired seclabel doesn't match the one the
disk image already has and an error is produced. But the
refcounter is never decremented.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740024
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This effectively reverts d7420430ce and adds new code.
Here is the problem: Imagine a file X that is to be shared
between two domains as a disk. Let the first domain (vm1) have
seclabel remembering turned on and the other (vm2) has it turned
off. Assume that both domains will run under the same user, but
the original owner of X is different (i.e. trying to access X
without relabelling leads to EPERM).
Let's start vm1 first. This will cause X to be relabelled and to
gain new attributes:
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.dac="$originalOwner"
When vm2 is started, X will again be relabelled, but since the
new label is the same as X already has (because of vm1) nothing
changes and vm1 and vm2 can access X just fine. Note that no
XATTR is changed (especially the refcounter keeps its value of 1)
because the vm2 domain has the feature turned off.
Now, vm1 is shut off and vm2 continues running. In seclabel
restore process we would get to X and since its refcounter is 1
we would restore the $originalOwner on it. But this is unsafe to
do because vm2 is still using X (remember the assumption that
$originalOwner and vm2's seclabel are distinct?).
The problem is that refcounter stored in XATTRs doesn't reflect
the actual times a resource is in use. Since I don't see any easy
way around it let's just not store original owner on shared
resources. Shared resource in world of domain disks is:
- whole backing chain but the top layer,
- read only disk (we don't require CDROM to be explicitly
marked as shareable),
- disk marked as shareable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Just like previous commit allowed to enable or disable owner
remembering for each individual path, do the same for SELinux
driver. This is going to be needed in the next commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Both DAC and SELinux drivers support transactions. Each item on
the transaction list consists of various variables and @restore
is one of them. Document it so that as the list of variables grow
it's easier to spot which variable does what.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The way that virSecurityDACRecallLabel is currently written is
that if XATTRs are not supported for given path to the caller
this is not different than if the path is still in use. The value
of 1 is returned which makes secdrivers skip label restore.
This is clearly a bug as we are not restoring labels on say NFS
even though previously we were.
Strictly speaking, changes to virSecurityDACRememberLabel are not
needed, but they are done anyway so that getter and setter behave
in the same fashion.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Just like it's DAC counterpart is doing,
virSecuritySELinuxRestoreAllLabel() could print @migrated in the
debug message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The d_type field cannot be assumed to be filled. Some filesystems, such
as older XFS, will simply report DT_UNKNOWN.
Even if the d_type is filled in, the use of it in the SELinux functions
is dubious. If labelling all files in a directory there's no reason to
skip things which are not regular files. We merely need to skip "." and
"..", which is done by virDirRead() already.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Security labeling of disks consists of labeling of the disk image
itself and it's backing chain. Modify
virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also
simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The @con type security_context_t is actually a "char *", so the
correct check should be to dereference one more level; otherwise,
we could return/use the NULL pointer later in a subsequent
virSecuritySELinuxSetFileconImpl call (using @fcon).
Suggested-by: Michal Prívozník <mprivozn@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If virSecuritySELinuxRestoreFileLabel returns 0 or -1 too soon, then
the @newpath will be leaked.
Suggested-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It helps whe trying to match calls with virSecuritySELinuxSetAllLabel
if the order in which devices are set/restored is the same in
both functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When iterating over list of paths/disk sources to relabel it may
happen that the process fails at some point. In that case, for
the sake of keeping seclabel refcount (stored in XATTRs) in sync
with reality we have to perform rollback. However, if that fails
too the only thing we can do is warn user.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It's important to keep XATTRs untouched (well, in the same state
they were in when entering the function). Otherwise our
refcounting would be messed up.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Similarly to what I did in DAC driver, this also requires the
same SELinux label to be used for shared paths. If a path is
already in use by a domain (or domains) then and the domain we
are starting now wants to access the path it has to have the same
SELinux label. This might look too restrictive as the new label
can still guarantee access to already running domains but in
reality it is very unlikely and usually an admin mistake.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
It is going to be important to know if the current transaction we
are running is a restore operation or set label operation so that
we know whether to call virSecurityGetRememberedLabel() or
virSecuritySetRememberedLabel(). That is, whether we are in a
restore and therefore have to fetch the remembered label, or we
are in set operation and therefore have to store the original
label.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
In 4674fc6afd 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This array is allocated in virSecuritySELinuxContextListAppend()
but never freed. This commit is essentially the same as ca25026.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Já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: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Já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: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Já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: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal 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: Pavel Hrdina <phrdina@redhat.com>
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 e13e8808f9 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: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John 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=1430988
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@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: Pavel 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: Erik 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: Erik 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: Erik Skultety <eskultet@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: Michal 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=1413773
Signed-off-by: Erik 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 b109c09765. However, with one my latest
commits (4674fc6afd) I've changed the type of @tcon variable to
'const char *' which results in build failure on the systems from
above.
Signed-off-by: Michal Privoznik <mprivozn@redhat.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: Eric 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.html
Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
Use a pointer and the virDomainChrSourceDefNew() function in order to
allocate the structure for _virDomainSmartcardDef.
Signed-off-by: John 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: John Ferlan <jferlan@redhat.com>
fdstream.c: In function 'virFDStreamWrite':
fdstream.c:390:29: error: logical 'or' of equal expressions [-Werror=logical-op]
if (errno == EAGAIN || errno == EWOULDBLOCK) {
^~
Fedora rawhide now uses gcc 6.0 and there is a bug with -Wlogical-op
producing false warnings.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602
Use GCC pragma push/pop and ignore -Wlogical-op for GCC that supports
push/pop pragma and also has this bug.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Since commit 7140807917 we are generating
socket path later than before -- when starting a domain. That makes one
particular inconsistent state of a chardev, which was not possible
before, currently valid. However, SELinux security driver forgot to
guard the main restoring function by a check for NULL-paths. So make it
no-op for NULL paths, as in the DAC driver.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1300532
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
A device tree binary file specified by /domain/os/dtb element is a
read-only resource similar to kernel and initrd files. We shouldn't
restore its label when destroying a domain to avoid breaking other
domains configure with the same device tree.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Kernel/initrd files are essentially read-only shareable images and thus
should be handled in the same way. We already use the appropriate label
for kernel/initrd files when starting a domain, but when a domain gets
destroyed we would remove the labels which would make other running
domains using the same files very unhappy.
https://bugzilla.redhat.com/show_bug.cgi?id=921135
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Many of the functions follow the pattern:
virSecurity.*Security.*Label
Remove the second 'Security' from the names, it should be obvious
that the virSecurity* functions deal with security labels even
without it.
We have macros for both positive and negative string matching.
Therefore there is no need to use !STREQ or !STRNEQ. At the same
time as we are dropping this, new syntax-check rule is
introduced to make sure we won't introduce it again.
Signed-off-by: Ishmanpreet Kaur Khera <khera.ishman@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
If running in session mode it may happen that we fail to set
correct SELinux label, but the image may still be readable to
the qemu process. Take this into account.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We have plenty of callbacks in the driver. Some of these
callbacks require more than one argument to be passed. For that
we currently have a data type (struct) per each callback. Well,
so far for only one - SELinuxSCSICallbackData. But lets turn it
into more general name so it can be reused in other callbacks too
instead of each one introducing a new, duplicate data type.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Remove unused variable, tag unused parameter and adjust return type.
introduced by 3f48345f7e
CC security/libvirt_security_manager_la-security_selinux.lo
security/security_selinux.c: In function 'virSecuritySELinuxDomainSetDirLabel':
security/security_selinux.c:2520:5: error: return makes pointer from integer without a cast [-Werror]
security/security_selinux.c:2514:9: error: unused variable 'ret' [-Werror=unused-variable]
security/security_selinux.c:2509:59: error: unused parameter 'mgr' [-Werror=unused-parameter]
In virSecuritySELinuxSetSecurityChardevLabel() we are labelling unix
socket path, but accessing another structure of the union. This does
not pose a problem currently as both paths are at the same offset, but
this should be fixed for the future.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Support for shared hostdev's was added in a number of commits, initially
starting with 'f2c1d9a80' and most recently commit id 'fd243fc4' to fix
issues with the initial implementation. Missed in all those changes was
the need to mimic the virSELinux{Set|Restore}SecurityDiskLabel code to
handle the "shared" (or shareable) and readonly options when Setting
or Restoring the SELinux labels.
This patch will adjust the virSecuritySELinuxSetSecuritySCSILabel to not
use the virSecuritySELinuxSetSecurityHostdevLabelHelper in order to set
the label. Rather follow what the Disk code does by setting the label
differently based on whether shareable/readonly is set. This patch will
also modify the virSecuritySELinuxRestoreSecuritySCSILabel to follow
the same logic as virSecuritySELinuxRestoreSecurityImageLabelInt and not
restore the label if shared/readonly
https://bugzilla.redhat.com/show_bug.cgi?id=1147057
The code for relabelling the TAP FD is there due to a race. When
libvirt creates a /dev/tapN device it's labeled as
'system_u:object_r:device_t:s0' by default. Later, when
udev/systemd reacts to this device, it's relabelled to the
expected label 'system_u:object_r:tun_tap_device_t:s0'. Hence, we
have a code that relabels the device, to cut the race down. For
more info see ae368ebfcc.
But the problem is, the relabel function is called on all TUN/TAP
devices. Yes, on /dev/net/tun too. This is however a special kind
of device - other processes uses it too. We shouldn't touch it's
label then.
Ideally, there would an API in SELinux that would label just the
passed FD and not the underlying path. That way, we wouldn't need
to care as we would be not labeling /dev/net/tun but the FD
passed to the domain. Unfortunately, there's no such API so we
have to workaround until then.
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The check for ISCSI devices was missing a check of subsys type, which
meant we could skip labelling of other host devices as well. This fixes
USB hotplug on F21
https://bugzilla.redhat.com/show_bug.cgi?id=1145968
https://bugzilla.redhat.com/show_bug.cgi?id=1141879
A long time ago I've implemented support for so called multiqueue
net. The idea was to let guest network traffic be processed by
multiple host CPUs and thus increasing performance. However, this
behavior is enabled by QEMU via special ioctl() iterated over the
all tap FDs passed in by libvirt. Unfortunately, SELinux comes in
and disallows the ioctl() call because the /dev/net/tun has label
system_u:object_r:tun_tap_device_t:s0 and 'attach_queue' ioctl()
is not allowed on tun_tap_device_t type. So after discussion with
a SELinux developer we've decided that the FDs passed to the QEMU
should be labelled with svirt_t type and SELinux policy will
allow the ioctl(). Therefore I've made a patch
(cf976d9dcf) that does exactly this. The patch
was fixed then by a443193139 and
b635b7a1af. However, things are not
that easy - even though the API to label FD is called
(fsetfilecon_raw) the underlying file is labelled too! So
effectively we are mangling /dev/net/tun label. Yes, that broke
dozen of other application from openvpn, or boxes, to qemu
running other domains.
The best solution would be if SELinux provides a way to label an
FD only, which could be then labeled when passed to the qemu.
However that's a long path to go and we should fix this
regression AQAP. So I went to talk to the SELinux developer again
and we agreed on temporary solution that:
1) All the three patches are reverted
2) SELinux temporarily allows 'attach_queue' on the
tun_tap_device_t
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
I've noticed two problem with the automatically created NVRAM varstore
file. The first, even though I run qemu as root:root for some reason I
get Permission denied when trying to open the _VARS.fd file. The
problem is, the upper directory misses execute permissions, which in
combination with us dropping some capabilities result in EPERM.
The next thing is, that if I switch SELinux to enforcing mode, I get
another EPERM because the vars file is not labeled correctly. It is
passed to qemu as disk and hence should be labelled as disk. QEMU may
write to it eventually, so this is different to kernel or initrd.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
After a4431931 the TAP FDs ale labeled with image label instead
of the process label. On the other hand, the commit was
incomplete as a few lines above, there's still old check for the
process label presence while it should be check for the image
label instead.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
QEMU now supports UEFI with the following command line:
-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \
-drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \
where the first line reflects <loader> and the second one <nvram>.
Moreover, these two lines obsolete the -bios argument.
Note that UEFI is unusable without ACPI. This is handled properly now.
Among with this extension, the variable file is expected to be
writable and hence we need security drivers to label it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
For security type='none' libvirt according to the docs should not
generate seclabel be it for selinux or any model. So, skip the
reservation of labels when type is none.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
The cleanup in commit cf976d9d used secdef->label to label the tap
FDs, but that is not possible since it's process-only label (svirt_t)
and not a object label (e.g. svirt_image_t). Starting a domain failed
with EPERM, but simply using secdef->imagelabel instead of
secdef->label fixes it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1095636
When starting up the domain the domain's NICs are allocated. As of
1f24f682 (v1.0.6) we are able to use multiqueue feature on virtio
NICs. It breaks network processing into multiple queues which can be
processed in parallel by different host CPUs. The queues are, however,
created by opening /dev/net/tun several times. Unfortunately, only the
first FD in the row is labelled so when turning the multiqueue feature
on in the guest, qemu will get AVC denial. Make sure we label all the
FDs needed.
Moreover, the default label of /dev/net/tun doesn't allow
attaching a queue:
type=AVC msg=audit(1399622478.790:893): avc: denied { attach_queue }
for pid=7585 comm="qemu-kvm"
scontext=system_u:system_r:svirt_t:s0:c638,c877
tcontext=system_u:system_r:virtd_t:s0-s0:c0.c1023
tclass=tun_socket
And as suggested by SELinux maintainers, the tun FD should be labeled
as svirt_t. Therefore, we don't need to adjust any range (as done
previously by Guannan in ae368ebf) rather set the seclabel of the
domain directly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Create the structures and API's to hold and manage the iSCSI host device.
This extends the 'scsi_host' definitions added in commit id '5c811dce'.
A future patch will add the XML parsing, but that code requires some
infrastructure to be in place first in order to handle the differences
between a 'scsi_host' and an 'iSCSI host' device.
Split virDomainHostdevSubsysSCSI further. In preparation for having
either SCSI or iSCSI data, create a union in virDomainHostdevSubsysSCSI
to contain just a virDomainHostdevSubsysSCSIHost to describe the
'scsi_host' host device
This negation in names of boolean variables is driving me insane. The
code is much more readable if we drop the 'no-' prefix. Well, at least
for me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
In the future we might need to track state of individual images. Move
the readonly and shared flags to the virStorageSource struct so that we
can keep them in a per-image basis.
