This came up in discussions around huge pages, but it will cover
more per guest paths that should be added to the guests apparmor profile:
- keys via qemuDomainWriteMasterKeyFile
- per domain dirs via qemuProcessMakeDir
- memory backing paths via qemuProcessBuildDestroyMemoryPathsImpl
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.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>
Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.
Generated using
$ git grep -El '[[:blank:]][[:blank:]]\\$' | \
grep -E '*\.([chx]|am|mk)$$' | \
while read f; do \
sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
done
Signed-off-by: Andrea Bolognani <abologna@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.
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.
Some globbing chars in the domain name could be used to break out of
apparmor rules, so lets forbid these when in virt-aa-helper.
Also adding a test to ensure all those cases were detected as bad char.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Hot-adding disks does not parse the full XML to generate apparmor rules.
Instead it uses -f <PATH> to append a generic rule for that file path.
580cdaa7: "virt-aa-helper: locking disk files for qemu 2.10" implemented
the qemu 2.10 requirement to allow locking on disks images that are part of
the domain xml.
But on attach-device a user will still trigger an apparmor deny by going
through virt-aa-helper -f, to fix that add the lock "k" permission to the
append file case of virt-aa-helper.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
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.
To avoid any issues later on if paths ever change (unlikely but
possible) and to match the style of other generated rules the paths
of the static rules have to be quoted as well.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
libvirt allows spaces in vm names, there were issues in the past but it
seems not removed so the assumption has to be that spaces are continuing
to be allowed.
Therefore virt-aa-helper should not reject spaces in vm names anymore if
it is going to be refused causing issues then the parser or xml schema
should do so.
Apparmor rules are in quotes, so a space in a path based on the name works.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If users only specified vendor&product (the common case) then parsing
the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
Bus and Device would much later be added when the devices are prepared
to be added.
Due to that a hot-add of a usb hostdev works as the device is prepared
and virt-aa-helper processes the new internal xml. But on an initial
guest start at the time virt-aa-helper renders the apparmor rules the
bus/device id's are not set yet:
p ctl->def->hostdevs[0]->source.subsys.u.usb
$12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
= 21888}
That causes rules to be wrong:
"/dev/bus/usb/000/000" rw,
The fix calls virHostdevFindUSBDevice after reading the XML from
virt-aa-helper to only add apparmor rules for devices that could be found
and now are fully known to be able to write the rule correctly.
It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
adding an apparmor rule for a device not found makes no sense no matter
what startup policy it has set.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Seeing a log message saying 'flags=93' is ambiguous & confusing unless
you happen to know that libvirt always prints flags as hex. Change our
debug messages so that they always add a '0x' prefix when printing flags,
and '0' prefix when printing mode. A few other misc places gain a '0x'
prefix in error messages too.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
instead of only unloading it. This makes sure old profiles don't pile up
in /etc/apparmor.d/libvirt and we get updates to modified templates on
VM restart.
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
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
When security drivers are active but confinement is not enabled,
there is no need to autogenerate <seclabel> elements when starting
a domain def that contains no <seclabel> elements. In fact,
autogenerating the elements can result in needless save/restore and
migration failures when the security driver is not active on the
restore/migration target.
This patch changes the virSecurityManagerGenLabel function in
src/security_manager.c to only autogenerate a <seclabel> element
if none is already defined for the domain *and* default
confinement is enabled. Otherwise the needless <seclabel>
autogeneration is skipped.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
Testing qemu-2.10-rc3 shows issues like:
qemu-system-aarch64: -drive file=/home/ubuntu/vm-start-stop/vms/
7936-0_CODE.fd,if=pflash,format=raw,unit=1: Failed to unlock byte 100
There is an apparmor deny due to qemu now locking those files:
apparmor="DENIED" operation="file_lock" [...]
name="/home/ubuntu/vm-start-stop/vms/7936-0_CODE.fd"
name="/var/lib/uvtool/libvirt/images/kvmguest-artful-normal.qcow"
[...] comm="qemu-system-aarch64" requested_mask="k" denied_mask="k"
The profile needs to allow locking for loader and nvram files via
the locking (k) rule.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Testing qemu-2.10-rc2 shows issues like:
qemu-system-x86_64: -drive file=/var/lib/uvtool/libvirt/images/kvmguest- \
artful-normal.qcow,format=qcow2,if=none,id=drive-virtio-disk0:
Failed to lock byte 100
It seems the following qemu commit changed the needs for the backing
image rules:
(qemu) commit 244a5668106297378391b768e7288eb157616f64
Author: Fam Zheng <famz@redhat.com>
file-posix: Add image locking to perm operations
The block appears as:
apparmor="DENIED" operation="file_lock" [...]
name="/var/lib/uvtool/libvirt/images/kvmguest-artful-normal.qcow"
[...] comm="qemu-system-x86" requested_mask="k" denied_mask="k"
With that qemu change in place the rules generated for the image
and backing files need the allowance to also lock (k) the files.
Disks are added via add_file_path and with this fix rules now get
that permission, but no other rules are changed, example:
- "/var/lib/uvtool/libvirt/images/kvmguest-artful-normal-a2.qcow" rw,
+ "/var/lib/uvtool/libvirt/images/kvmguest-artful-normal-a2.qcow" rwk
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
In commit 5e515b542d I've attempted to fix the inability to access
storage from the apparmor helper program by linking with the storage
driver. By linking with the .so the linker complains that it's not
portable. Fix this by loading the module dynamically as we are supposed
to do.
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
The refactor to split up storage driver into modules broke the apparmor
helper program, since that did not initialize the storage driver
properly and thus detection of the backing chain could not work.
Register the storage driver backends explicitly. Unfortunately it's now
necessary to link with the full storage driver to satisfy dependencies
of the loadable modules.
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
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>
The split firmware and variables files introduced by
https://bugs.debian.org/764918 are in a different directory for
some reason. Let the virtual machine read both.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
virDomainXMLOption gains driver specific callbacks for parsing and
formatting save cookies.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
While checking for ABI stability, drivers might pose additional
checks that are not valid for general case. For instance, qemu
driver might check some memory backing attributes because of how
qemu works. But those attributes may work well in other drivers.
Signed-off-by: Michal Privoznik <mprivozn@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>
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>
If the apparmor security driver is loaded/enabled and domain config
contains a <seclabel> element whose type attribute is not 'apparmor',
starting the domain fails when attempting to label resources such
as tap FDs.
Many of the apparmor driver entry points attempt to retrieve the
apparmor security label from the domain def, returning failure if
not found. Functions such as AppArmorSetFDLabel fail even though
domain config contains an explicit 'none' secuirty driver, e.g.
<seclabel type='none' model='none'/>
Change the entry points to succeed if the domain config <seclabel>
is not apparmor. This matches the behavior of the selinux driver.
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>
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>
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>
With our new qemu namespace code in place, the relabelling of
devices is done not as good is it could: a child process is
spawned, it enters the mount namespace of the qemu process and
then runs desired API of the security driver.
Problem with this approach is that internal state transition of
the security driver done in the child process is not reflected in
the parent process. While currently it wouldn't matter that much,
it is fairly easy to forget about that. We should take the extra
step now while this limitation is still fresh in our minds.
Three new APIs are introduced here:
virSecurityManagerTransactionStart()
virSecurityManagerTransactionCommit()
virSecurityManagerTransactionAbort()
The Start() is going to be used to let security driver know that
we are starting a new transaction. During a transaction no
security labels are actually touched, but rather recorded and
only at Commit() phase they are actually updated. Should
something go wrong Abort() aborts the transaction freeing up all
memory allocated by transaction.
Signed-off-by: Michal Privoznik <mprivozn@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>
