Just like for SPICE, we need to change the permissions on the DRI device
used as the @rendernode for egl-headless graphics type.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@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>
We switched to opening mode='bind' sockets ourselves:
commit 30fb2276d8
qemu: support passing pre-opened UNIX socket listen FD
in v4.5.0-rc1~251
Then fixed qemuBuildChrChardevStr to change libvirtd's label
while creating the socket:
commit b0c6300fc4
qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
v4.5.0-rc1~52
Also add labeling of these sockets to the DAC driver.
Instead of duplicating the logic which decides whether libvirt should
pre-create the socket, assume an existing path meaning that it was created
by libvirt.
https://bugzilla.redhat.com/show_bug.cgi?id=1633389
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
It may happen that in the list of paths/disk sources to relabel
there is a disk source. If that is the case, the path is NULL. In
that case, we shouldn't try to lock the path. It's likely a
network disk anyway and therefore there is nothing to lock.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.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>
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>
Firstly, the message that says we're setting uid:gid shouldn't be
called from virSecurityDACSetOwnershipInternal() because
virSecurityDACRestoreFileLabelInternal() is calling it too.
Secondly, there are places between us reporting label restore and
us actually doing it where we can quit. Don't say we're doing
something until we are actually about to do it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far the whole transaction handling is done
virSecurityDACSetOwnershipInternal(). 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 relabeling it.
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>
These two functions (virSecurityDACSetOwnership and
virSecurityDACRestoreFileLabelInternal) do not really change
@src. Make it const.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This function is going call security manager APIs and therefore
it needs pointer to it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
This function is going call security manager APIs and therefore
it needs pointer to it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@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>
We're freeing individual items in it but not the array itself.
==19200== 40 bytes in 1 blocks are definitely lost in loss record 847 of 1,059
==19200== at 0x4C2D12F: realloc (vg_replace_malloc.c:785)
==19200== by 0x52C5532: virReallocN (viralloc.c:245)
==19200== by 0x52C5628: virExpandN (viralloc.c:294)
==19200== by 0x52C58FC: virInsertElementsN (viralloc.c:436)
==19200== by 0x542856B: virSecurityDACChownListAppend (security_dac.c:115)
==19200== by 0x54286B4: virSecurityDACTransactionAppend (security_dac.c:167)
==19200== by 0x542902F: virSecurityDACSetOwnershipInternal (security_dac.c:560)
==19200== by 0x54295D6: virSecurityDACSetOwnership (security_dac.c:650)
==19200== by 0x542AEE0: virSecurityDACSetInputLabel (security_dac.c:1472)
==19200== by 0x542B61D: virSecurityDACSetAllLabel (security_dac.c:1693)
==19200== by 0x542DD67: virSecurityManagerSetAllLabel (security_manager.c:869)
==19200== by 0x54279C2: virSecurityStackSetAllLabel (security_stack.c:361)
Signed-off-by: Michal Privoznik <mprivozn@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.
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.
For a logged in user this a path like /dev/dri/renderD128 will have
default ownership root:video which won't work for the qemu:qemu user,
so we need to chown it.
We only do this when mount namespaces are enabled in the qemu driver,
so the chown'ing doesn't interfere with other users of the shared
render node path
https://bugzilla.redhat.com/show_bug.cgi?id=1460804
The VIR_SECURITY_MANAGER_MOUNT_NAMESPACE flag informs the DAC driver
if mount namespaces are in use for the VM. Will be used for future
changes.
Wire it up in the qemu driver
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>
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 attribute 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>
The code at the very bottom of the DAC secdriver that calls
chown() should be fine with read-only data. If something needs to
be prepared it should have been done beforehand.
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>
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>
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.
Fixes several style issues and removes "DEF" (what is it supposed to
mean anyway?) from debug messages.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Even though the APIs are not implemented yet, they create a
skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This function should really be called only when we want to change
ownership of a file (or disk source). Lets switch to calling a
wrapper function which will eventually record the current owner
of the file and call virSecurityDACSetOwnershipInternal
subsequently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This is pure code adjustment. The structure is going to be needed
later as it will hold a reference that will be used to talk to
virtlockd. However, so far this is no functional change just code
preparation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
It's better if we stat() file that we are about to chown() at
first and check if there's something we need to change. Not that
it would make much difference, but for the upcoming patches we
need to be doing stat() anyway. Moreover, if we do things this
way, we can drop @chown_errno variable which will become
redundant.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Correctly mark the places where we need to remember and recall
file ownership. We don't want to mislead any potential developer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
SELinux security driver already does that, but DAC driver somehow missed
the memo. Let's fix it so it works the same way.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
When using qemuProcessAttach to attach a qemu process,
the DAC label is not filled correctly.
Introduce a new function to get the uid:gid from the system
and fill the label.
This fixes the daemon crash when 'virsh screenshot' is called:
https://bugzilla.redhat.com/show_bug.cgi?id=1161831
It also fixes qemu-attach after the prerequisite of this patch
(commit f8c1fb3) was pushed out of order.
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Signed-off-by: Ján Tomko <jtomko@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
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>
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
To integrate the security driver with the storage driver we need to
pass a callback for a function that will chown storage volumes.
Introduce and document the callback prototype.
When restoring security labels in the dac driver the code would resolve
the file path and use the resolved one to be chown-ed. The setting code
doesn't do that. Remove the unnecessary code.
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.
I'm going to add functions that will deal with individual image files
rather than whole disks. Rename the security function to make room for
the new one.
I'm going to add functions that will deal with individual image files
rather than whole disks. Rename the security function to make room for
the new one.
The image labels are stored in the virStorageSource struct. Convert the
virDomainDiskDefGetSecurityLabelDef helper not to use the full disk def
and move it appropriately.
A network disk might actually be backed by local storage. Also the path
iterator actually handles networked disks well now so remove the code
that skips the labelling in dac and selinux security driver.
In "src/conf/domain_conf.h" there are many enum declarations. The
cleanup in this header filer was started, but it wasn't enough and
there are many other files that has enum variables declared. So, the
commit was starting to be big. This commit finish the cleanup in this
header file and in other files that has enum variables, parameters,
or functions declared.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
In "src/conf/domain_conf.h" there are many enumerations (enum)
declarations to be converted as a typedef too. As mentioned before,
it's better to use a typedef for variable types, function types and
other usages. I think this file has most of those enum declarations
at "src/conf/". So, me and Eric Blake plan to keep the cleanups all
over the source code. This time, most of the files changed in this
commit are related to part of one file: "src/conf/domain_conf.h".
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
With dynamic_ownership = 1 but no seclabels, RestoreChardevLabel
dereferences the NULL seclabel when checking if norelabel is set.
Remove this check, since it is already done in RestoreSecurityAllLabel
and if norelabel is set, RestoreChardevLabel is never called.
The domain definition is clearly used a few lines
below so there's no need to mark @def as unused.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The DAC driver ignores the relabel='no' attribute in chardev config
<serial type='file'>
<source path='/tmp/jim/test.file'>
<seclabel model='dac' relabel='no'/>
</source>
<target port='0'/>
</serial>
This patch avoids labeling chardevs when relabel='no' is specified.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
When relabel='no' at the domain level, there is no need to call
the hostdev relabeling functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
https://bugzilla.redhat.com/show_bug.cgi?id=999301
The DAC driver ignores the relabel='no' attribute in disk config
<disk type='file' device='floppy'>
<driver name='qemu' type='raw'/>
<source file='/some/path/floppy.img'>
<seclabel model='dac' relabel='no'/>
</source>
<target dev='fda' bus='fdc'/>
<readonly/>
</disk>
This patch avoid labeling disks when relabel='no' is specified.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
If relabel='no' at the domain level, no need to attempt relabeling
in virSecurityDAC{Set,Restore}SecurityAllLabel().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Currently, the DAC security driver passes callback data as
void params[2];
params[0] = mgr;
params[1] = def;
Clean this up by defining a structure for passing the callback
data. Moreover, there's no need to pass the whole virDomainDef
in the callback as the only thing needed in the callbacks is
virSecurityLabelDefPtr.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
In switch statements, use enum types since it is safer when
adding new items to the enum.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Annotate some static function parameters with ATTRIBUTE_NONNULL
and remove checks for NULL inputs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
The code in virstoragefile.c is getting more complex as I
consolidate backing chain handling code. But for the setuid
virt-login-shell, we don't need to crawl backing chains. It's
easier to audit things for setuid security if there are fewer
files involved, so this patch moves the one function that
virFileOpen() was actually relying on to also live in virfile.c.
* src/util/virstoragefile.c (virStorageFileIsSharedFS)
(virStorageFileIsSharedFSType): Move...
* src/util/virfile.c (virFileIsSharedFS, virFileIsSharedFSType):
...to here, and rename.
(virFileOpenAs): Update caller.
* src/security/security_selinux.c
(virSecuritySELinuxSetFileconHelper)
(virSecuritySELinuxSetSecurityAllLabel)
(virSecuritySELinuxRestoreSecurityImageLabelInt): Likewise.
* src/security/security_dac.c
(virSecurityDACRestoreSecurityImageLabelInt): Likewise.
* src/qemu/qemu_driver.c (qemuOpenFileAs): Likewise.
* src/qemu/qemu_migration.c (qemuMigrationIsSafe): Likewise.
* src/util/virstoragefile.h: Adjust declarations.
* src/util/virfile.h: Likewise.
* src/libvirt_private.syms (virfile.h, virstoragefile.h): Move
symbols as appropriate.
Signed-off-by: Eric Blake <eblake@redhat.com>
Part of a series of cleanups to use new accessor methods.
* src/security/security_dac.c (virSecurityDACSetSecurityImageLabel)
(virSecurityDACRestoreSecurityImageLabelInt)
(virSecurityDACSetSecurityAllLabel): Use accessors.
* src/security/security_selinux.c
(virSecuritySELinuxRestoreSecurityImageLabelInt)
(virSecuritySELinuxSetSecurityImageLabel)
(virSecuritySELinuxSetSecurityAllLabel): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
Any source file which calls the logging APIs now needs
to have a VIR_LOG_INIT("source.name") declaration at
the start of the file. This provides a static variable
of the virLogSource type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
To support passing the path of the test data to the utils, one
more argument is added to virSCSIDeviceGetSgName,
virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related
code is changed accordingly.
Later tests for the scsi utils will be based on this patch.
Signed-off-by: Osier Yang <jyang@redhat.com>
Unlike the host devices of other types, SCSI host device XML supports
"shareable" tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
To ensure proper processing by virGetUserID() and virGetGroupID()
of a uid/gid add a "+" prior to the uid/gid to denote it's really
a uid/gid for the label.
