Currently, if either template is missing AppArmor support is
completely disabled. This means that uninstalling the LXC
driver from a system results in QEMU domains being started
without AppArmor confinement, which obviously doesn't make any
sense.
The problematic scenario was impossible to hit in Debian until
very recently, because all AppArmor files were shipped as part
of the same package; now that the Debian package is much closer
to the Fedora one, and specifically ships the AppArmor files
together with the corresponding driver, it becomes trivial to
trigger it.
Drop the checks entirely. virt-aa-helper, which is responsible
for creating the per-domain profiles starting from the
driver-specific template, already fails if the latter is not
present, so they were always redundant.
https://bugs.debian.org/1081396
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This commit modifies the AppArmor profile for virt-aa-helper to
accommodate an observed behavior in certain Linux distributions,
such as ArchLinux.
In these distributions, /usr/sbin symlinks to /usr/bin. To ensure
that virt-aa-helper can execute apparmor_parser when it resides
in /usr/bin, the profile has been updated accordingly.
Signed-off-by: Tom <libvirt-patch@douile.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
In its commit v9.0.0-rc0~1^2 QEMU started to read
/proc/sys/vm/max_map_count file to set up coroutine limits better
(something about VMAs, mmap(), see the commit for more info).
Allow the file in apparmor profile.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/660
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The QEMU package in Debian has recently moved the
qemu-bridge-helper binary under /usr/libexec/qemu. Update the
AppArmor profile accordingly.
https://bugs.debian.org/1077915
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
The whole point of pstore device is that the guest writes crash
dumps into it. But the way SELinux label is set on the
corresponding file warrants RO access only. This is due to a
copy-paste from code around: kernel/initrd/DTB/SLIC - these are
RO indeed, but pstore MUST be writable too. In a sense it's
closer to NVRAM/disks - hence set imagelabel on it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
The acpi-erst backend for pstore device exposes a path in the
host accessible to the guest and as such we must set seclabels on
it to grant QEMU RW access.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Kristina Hanicova <khanicov@redhat.com>
Since libvirt commit 3ef9b51b10e52886e8fe8d75e36d0714957616b7,
the pflash storage for the os loader file follows its read-only flag,
and qemu tries to open the file for writing if set so.
This patches virt-aa-helper to generate the VM's AppArmor rules
that allow this, using the same domain definition flag and default.
Signed-off-by: Miroslav Los <mirlos@cisco.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
When generating paths for a domain specific AppArmor profile each
path undergoes a validation where it's matched against an array
of well known prefixes (among other things). Now, for
OVMF/AAVMF/... images we have a list and some entries have
comments to which type of image the entry belongs to. For
instance:
"/usr/share/OVMF/", /* for OVMF images */
"/usr/share/AAVMF/", /* for AAVMF images */
But these comments are pretty useless. The path itself already
gives away the image type. Drop them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Fix libvirtd hang since fork() was called while another thread had
security manager locked.
We have the stack security driver, which internally manages other security drivers,
just call them "top" and "nested".
We call virSecurityStackPreFork() to lock the top one, and it also locks
and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
it unlocks the top one, but not the nested ones. Thus, if one of the nested
drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we always
surround nested locks with top lock, it is always secure. Because we have got top lock
before fork child libvirtd.
However, it is not always the case in the current code, We discovered this case:
the nested list obtained through the qemuSecurityGetNested() will be locked directly
for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the nested list
is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
The problem stack is as follows:
libvirtd thread1 libvirtd thread2 child libvirtd
| | |
| | |
virsh capabilities qemuProcessLanuch |
| | |
| lock top |
| | |
lock nested | |
| | |
| fork------------------->|(nested lock held by thread1)
| | |
| | |
unlock nested unlock top unlock top
|
|
qemuSecuritySetSocketLabel
|
|
lock nested (deadlock)
In this commit, we ensure that the top lock is acquired before the nested lock,
so during fork, it's not possible for another task to acquire the nested lock.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1303031
Signed-off-by: hongmianquan <hongmianquan@bytedance.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
SEV-SNP is an enhancement of SEV/SEV-ES and thus it shares some
fields with it. Nevertheless, on XML level, it's yet another type
of <launchSecurity/>.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In a few instances there is a plain if() check for
_virDomainSecDef::sectype. While this works perfectly for now,
soon there'll be another type and we can utilize compiler to
identify all the places that need adaptation. Switch those if()
statements to switch().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Change the 'include' in the AppArmor policy to use 'include if exists'
when including <uuid>.files. Note that 'if exists' is only available
after AppArmor 3.0, therefore a #ifdef check must be added.
When the <uuid>.files is not present, there are some failures in the
AppArmor tools like the following, since they expect the file to exist
when using 'include':
ERROR: Include file /etc/apparmor.d/libvirt/libvirt-8534a409-a460-4fab-a2dd-0e1dce4ff273.files not found
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use contemporary style for declarations and automatic memory clearing
for a helper string.
Since the function can't fail any more, remove any mention of returning
errno and remove error checks from all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The .probe member of virSecurityDriver struct is declared to
return virSecurityDriverStatus enum. But there are two instances
(AppArmorSecurityManagerProbe() and
virSecuritySELinuxDriverProbe()) where callbacks are defined to
return an integer. This is an undefined behavior because integer
has strictly bigger space of possible values than the enum.
Defined those aforementioned callbacks so that they return the
correct enum instead of int.
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 performing an install, it's common for tooling such as virt-install
to remove the install kernel/initrd once they are successfully booted and
the domain has been redefined to boot without them. After the installation
is complete and the domain is rebooted/shutdown, the DAC and selinux
security drivers attempt to restore labels on the now deleted files. It's
harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir>
Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo>
Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Add a check for file existence to the virSecurity*RestoreFileLabel functions,
and avoid relabeling if the file is no longer available. Skipping the restore
caused failures in qemusecuritytest, which mocks stat, chown, etc as part of
ensuring the security drivers properly restore labels. virFileExists is now
mocked in qemusecuritymock.c to return true when passed a file previously
seen by the mocked stat, chown, etc functions.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The single caller for each function passes the same value
for @src and @parent, which means that we don't really need
the additional API.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Each one only has a single, trivial caller.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
It was clearly copied over from the SELinux driver without
updating its name in the process.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Commit 7a39b04d683f ("apparmor: Enable passt support") grants
passt(1) read-write access to /{,var/}run/libvirt/qemu/passt/* if
started by the libvirt daemon. That's the path where passt creates
PID and socket files only if the guest is started by the root user.
If the guest is started by another user, though, the path is more
commonly /var/run/user/$UID/libvirt/qemu/run/passt: add it as
read-write location. Otherwise, passt won't be able to start, as
reported by Andreas.
While at it, replace /{,var/}run/ in the existing rule by its
corresponding tunable variable, @{run}.
Fixes: 7a39b04d683f ("apparmor: Enable passt support")
Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1061678
Reported-by: Andreas B. Mundt <andi@debian.org>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
A QEMU change (10218ae6d006f76410804cc4dc690085b3d008b5) introduced
some libnuma calls that require read access to
/sys/devices/system/node/*/cpumap, which currently is forbidden by the
standard apparmor profile.
This commit allows read-only access to the file specified above.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/515
Signed-off-by: Sergio Durigan Junior <sergio.durigan@canonical.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
The new struct is virDeviceHostdevPCIDriverInfo, and the "backend"
enum in the hostdevDef will be replaced with a
virDeviceHostdevPCIDriverInfo named "driver'. Since the enum value in
this new struct is called "name", it means that all references to
"backend" will become "driver.name".
This will allow easily adding other items for new attributes in the
<driver> element / C struct, which will be useful once we are using
this new struct in multiple places.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Currently this enum is defined in domain_conf.h and named
virDomainHostdevSubsysPCIDriverType. I want to use it in parts of the
network and networkport config, so am moving its definition to
device_conf.h which is / can be included by all interested parties,
and renaming it to match the name of the corresponding XML attribute
("driver name"). The name change (which includes enum values) does cause a
lot of churn, but it's all mechanical.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When splitting out the apparmor modular daemon profiles from the
libvirtd profile, the net_admin and sys_admin capabilities were
dropped from the virtxend profile. It was not known at the time
that these capabilities were needed for PCI passthrough. Without
the capabilities, the following messages are emitted from the audit
subsystem
audit: type=1400 audit(1702939277.946:63): apparmor="DENIED" \
operation="capable" class="cap" profile="virtxend" pid=3611 \
comm="rpc-virtxend" capability=21 capname="sys_admin"
audit: type=1400 audit(1702940304.818:63): apparmor="DENIED" \
operation="capable" class="cap" profile="virtxend" pid=3731 \
comm="rpc-virtxend" capability=12 capname="net_admin"
It appears sys_admin is needed to simply read from the PCI dev's
sysfs config file. The net_admin capability is needed when setting
the MAC address of an SR-IOV virtual function.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Fix incorrect log message for timestamp value.
Probably this line was copied from the check for attr.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 7cfb7aab57 ("security_util: Remove stale XATTRs")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
While glibc provides qsort(), which usually is just a mergesort,
until sorting arrays so huge that temporary array used by
mergesort would not fit into physical memory (which in our case
is never), we are not guaranteed it'll use mergesort. The
advantage of mergesort is clear - it's stable. IOW, if we have an
array of values parsed from XML, qsort() it and produce some
output based on those values, we can then compare the output with
some expected output, line by line.
But with newer glibc this is all history. After [1], qsort() is
no longer mergesort but introsort instead, which is not stable.
This is suboptimal, because in some cases we want to preserve
order of equal items. For instance, in ebiptablesApplyNewRules(),
nwfilter rules are sorted by their priority. But if two rules
have the same priority, we want to keep them in the order they
appear in the XML. Since it's hard/needless work to identify
places where stable or unstable sorting is needed, let's just
play it safe and use stable sorting everywhere.
Fortunately, glib provides g_qsort_with_data() which indeed
implement mergesort and it's a drop in replacement for qsort(),
almost. It accepts fifth argument (pointer to opaque data), that
is passed to comparator function, which then accepts three
arguments.
We have to keep one occurance of qsort() though - in NSS module
which deliberately does not link with glib.
1: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=03bf8357e8291857a435afcc3048e0b697b6cc04
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Considering that at the virSecuritySELinuxSetFilecon() function can only
return 0 or -1 and so does the virSecuritySELinuxFSetFilecon(), the check
for '1' at the end of virSecuritySELinuxSetImageLabelInternal() is
effectively a dead code. Drop it.
Co-developed-by: sdl.qemu <sdl.qemu@linuxtesting.org>
Signed-off-by: Sergey Mironov <mironov@fintech.ru>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
The 'virt-aa-helper' process gets a XML of the VM it needs to create a
profile for. For a disk type='volume' this XML contained only the
pool and volume name.
The 'virt-aa-helper' needs a local path though for anything it needs to
label. This means that we'd either need to invoke connection to the
storage driver and re-resolve the volume. Alternative which makes more
sense is to pass the proper data in the XML already passed to it via the
new XML formatter and parser flags.
This was indirectly reported upstream in
https://gitlab.com/libvirt/libvirt/-/issues/546
The configuration in the issue above was created by Cockpit on Debian.
Since Cockpit is getting more popular it's more likely that users will
be impacted by this problem.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Convert the field, and fill in missing cases to switch()
statements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Convert the field, adjust the XML parsers to use virXMLPropEnum()
and fill in missing cases to switch() statements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Convert the field, adjust the XML parser to use
virXMLPropEnumDefault() and fill in missing cases to switch()
statements.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There are few places where a virDomainHostdevDef->source.subsys
is accessed without ->mode being checked. Mind you,
virDomainHostdevDef can be also in
VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES mode.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Error messages are exempt from the 80 columns rule. Move them
onto one line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
As advertised earlier, now that the _virDomainMemoryDef struct is
cleaned up, we can shorten some names as their placement within
unions define their use.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The _virDomainMemoryDef struct is getting a bit messy. It has
various members and only some of them are valid for given model.
Worse, some are re-used for different models. We tried to make
this more bearable by putting a comment next to each member
describing what models the member is valid for, but that gets
messy too.
Therefore, do what we do elsewhere: introduce an union of structs
and move individual members into their respective groups.
This allows us to shorten some names (e.g. nvdimmPath or
sourceNodes) as their purpose is obvious due to their placement.
But to make this commit as small as possible, that'll be
addressed later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Conceptually, from host POV there's no difference between NVDIMM
and VIRTIO_PMEM. Both expose a file to the guest (which is used
as a permanent storage). Other secdriver treat NVDIMM and
VIRTIO_PMEM the same. Thus, modify virt-aa-helper so that is
appends virtio-pmem backing path into the domain profile too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Currently, inside of virt-aa-helper code the domain definition is
parsed and then all def->mems are iterated over and for NVDIMM
models corresponding nvdimmPath is set label on. Conceptually,
this code works (except the label should be set for VIRTIO_PMEM
model too, but that is addressed in the next commit), but it can
be written in more extensible way. Firstly, there's no need to
check whether def->mems[i] is not NULL because we're inside a
for() loop that's counting through def->nmems. Secondly, we can
have a helper variable ('mem') to make the code more readable
(just like we do in other loops). Then, we can use switch() to
allow compiler warn us on new memory model.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is similar to the previous commit, except this is for a
different type (vahControl) and also fixes the case where _ctl is
passed not initialized to vah_error() (via ctl pointer so that's
probably why compilers don't complain).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Do for all other profiles what we already do for the
virt-aa-helper one. In this case we limit the feature to AppArmor
3.x, as it was never implemented for 2.x.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
For AppArmor 3.x we can use 'include if exists', which frees us
from having to create a dummy override. For AppArmor 2.x we keep
things as they are to avoid introducing regressions.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Implement the standard AppArmor 3.x abstraction extension
approach.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
The subprofile can only work by including the abstraction shipped
in the passt package, which we can't assume is present, and
'include if exists' doesn't work well on 2.x.
No distro that's stuck on AppArmor 2.x is likely to be shipping
passt anyway.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Compared to profiles, we only need a single preprocessing step
here, as there is no variable substitution happening.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Perform an additional preprocessing step before the existing
variable substitution. This is the same approach that we already
use to customize systemd unit files based on whether the service
supports TCP connections.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
As it turns out, apparmor 2.x and 3.x behave differently or have differing
levels of support for local customizations of profiles and profile
abstractions. Additionally the apparmor 2.x tools do not cope well with
'include if exists'. Revert this commit until a more complete solution is
developed that works with old and new apparmor.
Reverts: 9b743ee19053db2fc3da8fba1e9cf81915c1e2f4
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Apparmor profiles in /etc/apparmor.d/ are config files that can and should
be replaced on package upgrade, which introduces the potential to overwrite
any local changes. Apparmor supports local profile customizations via
/etc/apparmor.d/local/<service> [1].
This change makes the support explicit by adding libvirtd, virtqemud, and
virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs
are conditionally included by the corresponding main profiles.
[1] https://ubuntu.com/server/docs/security-apparmor
See "Profile customization" section
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Commit dbf1f68410 ("security: do not remember/recall labels for VFIO")
rightly changed the DAC and SELinux labeling parameters to fix a problem
with "VFIO hostdevs" but really only addressed the PCI codepaths.
As a result, we can still encounter this with VFIO MDEVs such as
vfio-ccw and vfio-ap, which can fail on a hotplug:
[test@host ~]# mdevctl stop -u 11f2d2bc-4083-431d-a023-eff72715c4f0
[test@host ~]# mdevctl start -u 11f2d2bc-4083-431d-a023-eff72715c4f0
[test@host ~]# cat disk.xml
<hostdev mode='subsystem' type='mdev' model='vfio-ccw'>
<source>
<address uuid='11f2d2bc-4083-431d-a023-eff72715c4f0'/>
</source>
<address type='ccw' cssid='0xfe' ssid='0x0' devno='0x3c51'/>
</hostdev>
[test@host ~]# virsh attach-device guest ~/disk.xml
error: Failed to attach device from /home/test/disk.xml
error: Requested operation is not valid: Setting different SELinux label on /dev/vfio/3 which is already in use
Make the same changes as reported in commit dbf1f68410, for the mdev paths.
Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
