12:39 p.m.
The need to have something like this in the first place is driven by
KubeVirt (see [1] and [2]). A draft version of this series has been
integrated into KubeVirt and it has been confirmed that it was
effective in removing the need to use LD_PRELOAD hacks in the storage
provider.
Changes from [v2]:
* added canonicalization for user-provided paths;
* fixed compilation issues when AppArmor support is enabled.
Changes from [v1]:
* documented more explicitly that the newly introduced option is
intended for very specific scenarios and not general usage; as
part of this, the NEWS update has been dropped too;
* made a few tweaks and addressed a few oversight based on review
feedback;
* several preparatory cleanup patches have been pushed.
Changes from [v0]:
* reworked approach.
[v2]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XP...
[v1]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XE...
[v0]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MM...
[1] https://issues.redhat.com/browse/CNV-34322
[2] https://issues.redhat.com/browse/CNV-39370
Andrea Bolognani (5):
security: Fix alignment
qemu: Introduce shared_filesystems configuration option
qemu: Propagate shared_filesystems
utils: Use overrides in virFileIsSharedFS()
qemu: Always set labels for TPM state
src/lxc/lxc_controller.c | 3 +-
src/lxc/lxc_driver.c | 2 +-
src/lxc/lxc_process.c | 4 +-
src/qemu/libvirtd_qemu.aug | 3 ++
src/qemu/qemu.conf.in | 23 ++++++++
src/qemu/qemu_conf.c | 31 +++++++++++
src/qemu/qemu_conf.h | 2 +
src/qemu/qemu_domain.c | 7 ++-
src/qemu/qemu_extdevice.c | 2 +-
src/qemu/qemu_migration.c | 23 ++++----
src/qemu/qemu_security.c | 85 +++++++++++++++++++++++-------
src/qemu/qemu_tpm.c | 38 +++++++------
src/qemu/qemu_tpm.h | 10 ++--
src/qemu/test_libvirtd_qemu.aug.in | 5 ++
src/security/security_apparmor.c | 8 ++-
src/security/security_dac.c | 47 +++++++++++++----
src/security/security_driver.h | 8 ++-
src/security/security_manager.c | 33 +++++++++---
src/security/security_manager.h | 9 +++-
src/security/security_nop.c | 5 ++
src/security/security_selinux.c | 56 +++++++++++++++-----
src/security/security_stack.c | 32 ++++++++---
src/util/virfile.c | 53 +++++++++++++++++--
src/util/virfile.h | 3 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/virfiletest.c | 2 +-
26 files changed, 389 insertions(+), 107 deletions(-)
--
2.44.0
12:39 p.m.
New subject: [PATCH v3 1/5] security: Fix alignment
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/security/security_selinux.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index aaec34ff8b..a4915dbc89 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1983,9 +1983,9 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
static int
virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
- virDomainDef *def,
- virStorageSource *src,
- virSecurityDomainImageLabelFlags flags)
+ virDomainDef *def,
+ virStorageSource *src,
+ virSecurityDomainImageLabelFlags flags)
{
virStorageSource *parent = src;
virStorageSource *n;
--
2.44.0
6:15 a.m.
New subject: [PATCH v3 1/5] security: Fix alignment
On Thu, May 02, 2024 at 19:39:38 +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/security/security_selinux.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
12:39 p.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
As explained in the comment, this can help in scenarios where
a shared filesystem can't be detected as such by libvirt, by
giving the admin the opportunity to provide this information
manually.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 3 +++
src/qemu/qemu.conf.in | 23 ++++++++++++++++++++++
src/qemu/qemu_conf.c | 31 ++++++++++++++++++++++++++++++
src/qemu/qemu_conf.h | 2 ++
src/qemu/test_libvirtd_qemu.aug.in | 5 +++++
5 files changed, 64 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 2b6526538f..1377fd89cc 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -143,6 +143,8 @@ module Libvirtd_qemu =
let storage_entry = bool_entry "storage_use_nbdkit"
+ let filesystem_entry = str_array_entry "shared_filesystems"
+
(* Entries that used to exist in the config which are now
* deleted. We keep on parsing them so we don't break
* ability to parse old configs after upgrade
@@ -173,6 +175,7 @@ module Libvirtd_qemu =
| swtpm_entry
| capability_filters_entry
| storage_entry
+ | filesystem_entry
| obsolete_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store
/([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index f406df8749..c543a0a55b 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -986,3 +986,26 @@
# note that the default might change in future releases.
#
#storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
+
+# libvirt will normally prevent migration if the storage backing the VM is not
+# on a shared filesystems. Sometimes, however, the storage *is* shared despite
+# not being detected as such: for example, this is the case when one of the
+# hosts involved in the migration is exporting its local storage to the other
+# one via NFS.
+#
+# Any directory listed here will be assumed to live on a shared filesystem,
+# making migration possible in scenarios such as the one described above.
+#
+# Due to how label remembering is implemented, it will generally be necessary
+# to set remember_owner=0 *on both sides of the migration* as well.
+#
+# NOTE: this option is intended to help in very specific scenarios that are
+# rarely encountered. If you find yourself reaching for this option, consider
+# reworking your environment so that it follows a more common architecture
+# rather than using it.
+#
+#shared_filesystems = [
+# "/path/to/images",
+# "/path/to/nvram",
+# "/path/to/swtpm"
+#]
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4050a82341..1daa97a359 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -374,6 +374,8 @@ static void virQEMUDriverConfigDispose(void *obj)
g_strfreev(cfg->capabilityfilters);
+ g_strfreev(cfg->sharedFilesystems);
+
g_free(cfg->deprecationBehavior);
}
@@ -1084,6 +1086,32 @@ virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
}
+static int
+virQEMUDriverConfigLoadFilesystemEntry(virQEMUDriverConfig *cfg,
+ virConf *conf)
+{
+ char **iter;
+
+ if (virConfGetValueStringList(conf, "shared_filesystems", false,
+ &cfg->sharedFilesystems) < 0)
+ return -1;
+
+ if (!cfg->sharedFilesystems)
+ return 0;
+
+ /* The paths provided by the user might contain trailing slashes
+ * and other fun diversions, which would break the naive string
+ * comparisons that we're later going to use them for */
+ for (iter = cfg->sharedFilesystems; *iter; iter++) {
+ char *canon = virFileCanonicalizePath(*iter);
+ g_free(*iter);
+ *iter = canon;
+ }
+
+ return 0;
+}
+
+
int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
const char *filename,
bool privileged)
@@ -1158,6 +1186,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
return -1;
+ if (virQEMUDriverConfigLoadFilesystemEntry(cfg, conf) < 0)
+ return -1;
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 36049b4bfa..b53d56be02 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -233,6 +233,8 @@ struct _virQEMUDriverConfig {
bool storageUseNbdkit;
virQEMUSchedCore schedCore;
+
+ char **sharedFilesystems;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index b97e6de11e..69fdae215a 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -119,3 +119,8 @@ module Test_libvirtd_qemu =
{ "deprecation_behavior" = "none" }
{ "sched_core" = "none" }
{ "storage_use_nbdkit" = "@USE_NBDKIT_DEFAULT@" }
+{ "shared_filesystems"
+ { "1" = "/path/to/images" }
+ { "2" = "/path/to/nvram" }
+ { "3" = "/path/to/swtpm" }
+}
--
2.44.0
6:58 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration
option
On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote:
As explained in the comment, this can help in scenarios where
a shared filesystem can't be detected as such by libvirt, by
giving the admin the opportunity to provide this information
manually.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 3 +++
src/qemu/qemu.conf.in | 23 ++++++++++++++++++++++
src/qemu/qemu_conf.c | 31 ++++++++++++++++++++++++++++++
src/qemu/qemu_conf.h | 2 ++
src/qemu/test_libvirtd_qemu.aug.in | 5 +++++
5 files changed, 64 insertions(+)
[...]
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index f406df8749..c543a0a55b 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -986,3 +986,26 @@
# note that the default might change in future releases.
#
#storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
+
+# libvirt will normally prevent migration if the storage backing the VM is not
+# on a shared filesystems. Sometimes, however, the storage *is* shared despite
+# not being detected as such: for example, this is the case when one of the
+# hosts involved in the migration is exporting its local storage to the other
+# one via NFS.
I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check,
but doing so without disabling dynamic ownership on the image itself
would still make the security driver cut off access to this file.
This means that users would have to use VIR_MIGRATE_UNSAFE and disabling
relabelling to all images they wish to use for such migration.
+# Any directory listed here will be assumed to live on a shared
filesystem,
+# making migration possible in scenarios such as the one described above.
+#
+# Due to how label remembering is implemented, it will generally be necessary
+# to set remember_owner=0 *on both sides of the migration* as well.
So IIUC the problem here is that on the box where the storage is local
this will create the XATTR entries used for remembering the lablels
(which also includes a refcount), which given that older NFS doesn't
support security labelling would then be leaked?
Looking at the code we seem to be calling 'qemuSecurityRestoreAllLabel'
with the 'migrated' flag set when migrating at all times. This should
make it theoretically possible to simply clear out the XATTRs for any
file which resides on the paths in question if we're migrating away
rather than ignoring them, which would solve the issue.
This would go together with the 'syntax-sugar'-ish flavor to this
feature. Additionally it would help in case, when the user configures
these paths after the VM was started and thus the XATTRs are already
applied. Those would not then be cleared on migration.
I also wanted to suggest to modify this paragraph to suggest a lesser
hammer to disable ownersip remembering, as that will disable it also for
the non-shared paths, but IIRC we don't have flags to disable it
individually (in contrast with the 'dynamic_ownership' flag which can be
disabled per-image via the 'relabel' attribute).
+# NOTE: this option is intended to help in very specific scenarios
that are
+# rarely encountered. If you find yourself reaching for this option, consider
+# reworking your environment so that it follows a more common architecture
+# rather than using it.
+#
+#shared_filesystems = [
+# "/path/to/images",
+# "/path/to/nvram",
+# "/path/to/swtpm"
+#]
[...]
+static int
+virQEMUDriverConfigLoadFilesystemEntry(virQEMUDriverConfig *cfg,
+ virConf *conf)
+{
+ char **iter;
+
+ if (virConfGetValueStringList(conf, "shared_filesystems", false,
+ &cfg->sharedFilesystems) < 0)
+ return -1;
+
+ if (!cfg->sharedFilesystems)
+ return 0;
+
+ /* The paths provided by the user might contain trailing slashes
+ * and other fun diversions, which would break the naive string
+ * comparisons that we're later going to use them for */
+ for (iter = cfg->sharedFilesystems; *iter; iter++) {
+ char *canon = virFileCanonicalizePath(*iter);
'virFileCanonicalizePath' is using 'realpath()' inside which will return
'NULL' in case the path does not exist.
As the output is cached/remembered this would cause that:
- any path which doesn't exist at time the daemon started would not
work if created after startup of the daemon until the daemon is
restarted
- any valid config following this entry will not work either as this is
a string list and is accessed exclusively via NULL-termination.
If you want to canonicalize paths you must do so at the time this
information will be used.
+ g_free(*iter);
+ *iter = canon;
+ }
+
+ return 0;
+}
8:40 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
On Thu, May 09, 2024 at 01:58:21PM GMT, Peter Krempa wrote:
On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote:
> +# libvirt will normally prevent migration if the storage backing the VM is not
> +# on a shared filesystems. Sometimes, however, the storage *is* shared despite
> +# not being detected as such: for example, this is the case when one of the
> +# hosts involved in the migration is exporting its local storage to the other
> +# one via NFS.
I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check,
but doing so without disabling dynamic ownership on the image itself
would still make the security driver cut off access to this file.
This was suggested in the past, but the argument against it was that
it's too vague: we want to skip the check on whether the storage is
shared, and that one only, while "unsafe migration" could potentially
cover a lot of unrelated scenarios.
The other argument against using a migration flag was that it's an
all-or-nothing proposition: either you perform the check for all
paths, or you skip it for all paths. A configuration option allows us
more granular control, where we can designate exactly which paths
should not be subject to the check, retaining the default behavior
for all the other ones.
> +# Any directory listed here will be assumed to live on a shared
filesystem,
> +# making migration possible in scenarios such as the one described above.
> +#
> +# Due to how label remembering is implemented, it will generally be necessary
> +# to set remember_owner=0 *on both sides of the migration* as well.
So IIUC the problem here is that on the box where the storage is local
this will create the XATTR entries used for remembering the lablels
(which also includes a refcount), which given that older NFS doesn't
support security labelling would then be leaked?
Yeah, the expectation when remembering is enabled is that both sides
of the migration will keep the record updated.
This works when XATTRs/SELinux labels are supported by the shared
filesystem or are *not* supported and both sides access it through
it, in which case it all ends up being no-op and works by omission :)
In our scenario, migrating from local to NFS will work but then
migrating back won't, because the existing information will lead
libvirt to believe that the image is already in use - which it is,
but not in the way that it expects :)
In general, all the bookkeeping and relabeling is pretty much broken
by design when migration is involved, and things only appear to work
because requests are silently ignored.
Looking at the code we seem to be calling
'qemuSecurityRestoreAllLabel'
with the 'migrated' flag set when migrating at all times. This should
make it theoretically possible to simply clear out the XATTRs for any
file which resides on the paths in question if we're migrating away
rather than ignoring them, which would solve the issue.
This would involve touching the file's attributes on the source side
after the VM is already running on the destination side, which is
something that the existing code understandably steers very clear of.
I've tried fairly hard to make things work "just so" and by now I'm
convinced that there is no solution which gets it right without
adding a lot of additional complexity/fragility and introducing its
own trade-offs.
Asking users to disable owner remembering is not too unreasonable
IMO. Especially when we take into account the fact that I'm targeting
KubeVirt's needs specifically with this feature, and they *already*
do that anyway.
This would go together with the 'syntax-sugar'-ish flavor to
this
feature. Additionally it would help in case, when the user configures
these paths after the VM was started and thus the XATTRs are already
applied. Those would not then be cleared on migration.
That sounds like a corner case of a corner case, and a direct result
of the admin failing to configure the host correctly in the first
place. I wouldn't be too concerned about it.
I also wanted to suggest to modify this paragraph to suggest a
lesser
hammer to disable ownersip remembering, as that will disable it also for
the non-shared paths, but IIRC we don't have flags to disable it
individually (in contrast with the 'dynamic_ownership' flag which can be
disabled per-image via the 'relabel' attribute).
I'm not aware of anything of the sort either.
--
Andrea Bolognani / Red Hat / Virtualization
10:10 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration
option
On Thu, May 09, 2024 at 06:40:01 -0700, Andrea Bolognani wrote:
On Thu, May 09, 2024 at 01:58:21PM GMT, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote:
> > +# libvirt will normally prevent migration if the storage backing the VM is
not
> > +# on a shared filesystems. Sometimes, however, the storage *is* shared
despite
> > +# not being detected as such: for example, this is the case when one of the
> > +# hosts involved in the migration is exporting its local storage to the other
> > +# one via NFS.
>
> I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check,
> but doing so without disabling dynamic ownership on the image itself
> would still make the security driver cut off access to this file.
This was suggested in the past, but the argument against it was that
it's too vague: we want to skip the check on whether the storage is
shared, and that one only, while "unsafe migration" could potentially
cover a lot of unrelated scenarios.
The other argument against using a migration flag was that it's an
all-or-nothing proposition: either you perform the check for all
paths, or you skip it for all paths. A configuration option allows us
more granular control, where we can designate exactly which paths
should not be subject to the check, retaining the default behavior
for all the other ones.
Fair enough.
> > +# Any directory listed here will be assumed to live on a
shared filesystem,
> > +# making migration possible in scenarios such as the one described above.
> > +#
> > +# Due to how label remembering is implemented, it will generally be necessary
> > +# to set remember_owner=0 *on both sides of the migration* as well.
>
> So IIUC the problem here is that on the box where the storage is local
> this will create the XATTR entries used for remembering the lablels
> (which also includes a refcount), which given that older NFS doesn't
> support security labelling would then be leaked?
Yeah, the expectation when remembering is enabled is that both sides
of the migration will keep the record updated.
This works when XATTRs/SELinux labels are supported by the shared
filesystem or are *not* supported and both sides access it through
it, in which case it all ends up being no-op and works by omission :)
In our scenario, migrating from local to NFS will work but then
migrating back won't, because the existing information will lead
libvirt to believe that the image is already in use - which it is,
but not in the way that it expects :)
There are two distinct things in play here when migrating in scenario
that this patch is dealing with:
1) Actual security/selinux labels
- these must properly allow access to the image during the whole
time and partially also from two hosts at once
2) The additional XATTR props libvirt uses to remember security labels
- these need to be refcounted properly
In addition to that there's two directions of the migration
o) Migration out from the host that uses this feature
i) Migration in to the host using this feature
Also note that for migration in you need to consider also cases when the
VM was never started at the host using this feature yet, and is freshly
migrated in.
Now things I see as problem in case when NFS not supporting xattr is
used. This means that the remote VM can set XATTRs and must use
'virt_use_nfs' sebool.
1.o) The selinux label will be leaked/kept on the image as the target of
the migration isn't able to remove the label
1.i) A new selinux label must be applied to the image
- this is what also patch 5/5 does for TPM
- applying security label might actually prevent the NFS server from
exporting the image if configured poorly
2.o) The libvirt XATTR label will be leaked with a refcount (preventing
further local start)
2.i) This should work IIUC as the label will simply be added
I'll definitely will need to check the behaviour of selinux labels
first myself.
While just removing 2 out of existence by disabling it may seem
lucrative I think we even might need the feature if we'll need to solve
1.o).
IMO the only proper option to do this across the XATTR boundary will be
to have an additional step in the finalizing phase of migration that
will unref the libvirt labels. In case when the last reference is gone
it'd need to also restore the label, same as it does now. During
migration there'll need to be a period while two refs are on the libvirt
xattrs.
As said I'll need to actually check what's really happening in regards
of the selinux labels.
In general, all the bookkeeping and relabeling is pretty much broken
by design when migration is involved, and things only appear to work
because requests are silently ignored.
As shown above the libvirt XATTRs are only a part of this.
> Looking at the code we seem to be calling
'qemuSecurityRestoreAllLabel'
> with the 'migrated' flag set when migrating at all times. This should
> make it theoretically possible to simply clear out the XATTRs for any
> file which resides on the paths in question if we're migrating away
> rather than ignoring them, which would solve the issue.
This would involve touching the file's attributes on the source side
after the VM is already running on the destination side, which is
something that the existing code understandably steers very clear of.
I've tried fairly hard to make things work "just so" and by now I'm
convinced that there is no solution which gets it right without
adding a lot of additional complexity/fragility and introducing its
own trade-offs.
Asking users to disable owner remembering is not too unreasonable
IMO. Especially when we take into account the fact that I'm targeting
KubeVirt's needs specifically with this feature, and they *already*
do that anyway.
Well, doing it like this makes it not very attractive for any other use.
Doing this just replaces the ugly hack in kubevirt with a rather
unattractive implementation of this in libvirt.
For anyone wanting to use this outside of Kubevirt they'd need to give
up the label remembering.
Also as noted above the libvirt xattr stuff migt be needed to actually
prevent leaking selinux labels.
> This would go together with the 'syntax-sugar'-ish
flavor to this
> feature. Additionally it would help in case, when the user configures
> these paths after the VM was started and thus the XATTRs are already
> applied. Those would not then be cleared on migration.
That sounds like a corner case of a corner case, and a direct result
of the admin failing to configure the host correctly in the first
place. I wouldn't be too concerned about it.
Sure, but proper support for using it together with security label
remembering would fix it anyways.
11:47 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
On Thu, May 09, 2024 at 05:10:50PM GMT, Peter Krempa wrote:
Now things I see as problem in case when NFS not supporting xattr is
used. This means that the remote VM can set XATTRs and must use
'virt_use_nfs' sebool.
I must be confused about the purpose of the virt_use_nfs sebool, and
I can't seem to find decent documentation about it. Do you have any
handy?
Have you actually been able to use either SELinux or (trusted)
XATTRs on an NFS-mounted filesystem? If so, how?
IMO the only proper option to do this across the XATTR boundary will
be
to have an additional step in the finalizing phase of migration that
will unref the libvirt labels. In case when the last reference is gone
it'd need to also restore the label, same as it does now. During
migration there'll need to be a period while two refs are on the libvirt
xattrs.
This sounds fairly attractive from a high-level point of view, though
I'll admit that I'm concerned about things going out of sync and
unintentionally cutting off file access to the target host as a
consequence of that.
As said I'll need to actually check what's really happening
in regards
of the selinux labels.
Please do. Hopefully you'll get further than I was able to :)
--
Andrea Bolognani / Red Hat / Virtualization
11:51 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration
option
On Thu, May 09, 2024 at 04:47:48PM +0000, Andrea Bolognani wrote:
On Thu, May 09, 2024 at 05:10:50PM GMT, Peter Krempa wrote:
> Now things I see as problem in case when NFS not supporting xattr is
> used. This means that the remote VM can set XATTRs and must use
> 'virt_use_nfs' sebool.
I must be confused about the purpose of the virt_use_nfs sebool, and
I can't seem to find decent documentation about it. Do you have any
handy?
Out of the box, there usually is no ability for QEMU to access
files stored on NFS whatsoever, because NFS lacks support for
storing (svirt_image_t:MCS) labels in xattr.
Setting virt_use_nfs, toggles the policy such that QEMU can now
access *any* nfs_t file. This lets QEMU works on NFS lacking
label support, but at the cost of killing MAC protection against
any other non-VM related files that might be stored on NFS. DAC
protection still applies though, since we're not running QEMU
as root.
If an NFS deployment *does* support SELinux labels, there is
no reason to use virt_use_nfs, and it should not be used due
to reduced MAC protection.
If an NFS deployment does *not* support SELinux labels, then
virt_use_nfs must be turned on
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:15 a.m.
New subject: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration
option
On Thu, May 09, 2024 at 17:51:59 +0100, Daniel P. Berrangé wrote:
On Thu, May 09, 2024 at 04:47:48PM +0000, Andrea Bolognani wrote:
> On Thu, May 09, 2024 at 05:10:50PM GMT, Peter Krempa wrote:
> > Now things I see as problem in case when NFS not supporting xattr is
> > used. This means that the remote VM can set XATTRs and must use
> > 'virt_use_nfs' sebool.
>
> I must be confused about the purpose of the virt_use_nfs sebool, and
> I can't seem to find decent documentation about it. Do you have any
> handy?
Out of the box, there usually is no ability for QEMU to access
files stored on NFS whatsoever, because NFS lacks support for
storing (svirt_image_t:MCS) labels in xattr.
Setting virt_use_nfs, toggles the policy such that QEMU can now
access *any* nfs_t file. This lets QEMU works on NFS lacking
label support, but at the cost of killing MAC protection against
any other non-VM related files that might be stored on NFS. DAC
protection still applies though, since we're not running QEMU
as root.
If an NFS deployment *does* support SELinux labels, there is
no reason to use virt_use_nfs, and it should not be used due
to reduced MAC protection.
So I've finally got around to test this with NFS-4.2 with "XATTR"
support. I used xattr in quotes, as per spec it seems that xattr support
over NFS is limited (per spec IIUC, and observably) to only handle the
'user' namespace of xattr labels. Any other namespace is _NOT_
available over NFS at least in default config. (The main point in the
docs seem to be that OSes may interpret the security labels in such
namespaces differently and NFS doesn't want to deal with attempting to
translate them in any shape or form (e.g. on linux we use the 'trusted'
namespace while on FreeBSD we use the 'system' namespace to remember
lables))
This means that neither 'selinux' labels nor the XATTRs libvirt uses to
remember the previous security labels are available on the other side of
migration.
This works (partially) correctly though, as either side of the migration
is handing the full lifecycle of the security label (even despite that
the storage doesn't transport them).
This means that on outgoing migration the source will remove the
reference on the xattr remembering the seclabels (without actually
restoring them). Destination would obviously attempt to take a reference
but can't as it's not supported. This means that xattrs are not leaked.
On incoming migration though there's an issue though. Libvirt tries to
apply (the same) seclabel, which is correct. This though involves also
an attempt to remember the owner, whcih succeeds, but remembers the
*active* seclabel. When the VM is then terminated that seclabel is
restored.
The above is _not_ a good reason to disable seclabel remembering though,
as suggested by docs added by this patch, even when it does in fact work
around that issue. We should rather fix the seclabel remembering code:
- for now by not trying to create the xattrs to remember the seclabel
when we're migrating in and the labels on the image are already
correct.
- in the future we perhaps could transfer the original seclabels in the
migration cookie
I'll be posting patches to address this before this series goes in, so
that the suggestion to disable 'remember_owner' globally can be dropped.
If an NFS deployment does *not* support SELinux labels, then
virt_use_nfs must be turned on
Based on the above, this is in fact needed even with NFS-4.2
12:39 p.m.
New subject: [PATCH v3 3/5] qemu: Propagate shared_filesystems
virFileIsSharedFS() is the function that ultimately decides
whether a filesystem should be considered shared, but the list
of manually configured shared filesystems is part of the QEMU
driver's configuration, so we need to pass the information
through several layers in order to make use of it.
Note that with this change the list is propagated all the way
through, but its contents are still ignored, so the behavior
remains the same for now.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/lxc/lxc_controller.c | 3 +-
src/lxc/lxc_driver.c | 2 +-
src/lxc/lxc_process.c | 4 +-
src/qemu/qemu_domain.c | 7 ++-
src/qemu/qemu_extdevice.c | 2 +-
src/qemu/qemu_migration.c | 23 ++++-----
src/qemu/qemu_security.c | 85 ++++++++++++++++++++++++--------
src/qemu/qemu_tpm.c | 29 +++++++----
src/qemu/qemu_tpm.h | 10 ++--
src/security/security_apparmor.c | 8 ++-
src/security/security_dac.c | 47 ++++++++++++++----
src/security/security_driver.h | 8 ++-
src/security/security_manager.c | 33 ++++++++++---
src/security/security_manager.h | 9 +++-
src/security/security_nop.c | 5 ++
src/security/security_selinux.c | 50 ++++++++++++++-----
src/security/security_stack.c | 32 +++++++++---
src/util/virfile.c | 13 +++--
src/util/virfile.h | 3 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/virfiletest.c | 2 +-
21 files changed, 281 insertions(+), 96 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 505b71d05e..7b432a1160 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1919,7 +1919,8 @@ static int virLXCControllerSetupDisk(virLXCController *ctrl,
/* Labelling normally operates on src, but we need
* to actually label the dst here, so hack the config */
def->src->path = dst;
- if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, def->src,
+ if (virSecurityManagerSetImageLabel(securityDriver,
+ NULL, ctrl->def, def->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)
< 0)
goto cleanup;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 1842ae8f0e..1fda848b0c 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3261,7 +3261,7 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid G_GNUC_UNUSED,
char *tmpsrc = def->src->path;
def->src->path = data->file;
if (virSecurityManagerSetImageLabel(data->driver->securityManager,
- data->vm->def, def->src,
+ NULL, data->vm->def, def->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0) {
def->src->path = tmpsrc;
goto cleanup;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 30ff4eb3d0..a7e8e2c28e 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -170,7 +170,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver,
}
if (flags & VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL) {
- virSecurityManagerRestoreAllLabel(driver->securityManager,
+ virSecurityManagerRestoreAllLabel(driver->securityManager, NULL,
vm->def, false, false);
}
@@ -1320,7 +1320,7 @@ int virLXCProcessStart(virLXCDriver * driver,
stopFlags |= VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL;
VIR_DEBUG("Setting domain security labels");
- if (virSecurityManagerSetAllLabel(driver->securityManager,
+ if (virSecurityManagerSetAllLabel(driver->securityManager, NULL,
vm->def, NULL, false, false) < 0)
goto cleanup;
stopFlags |= VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3469f0d40c..736486f9ac 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11912,7 +11912,12 @@ virQEMUFileOpenAs(uid_t fallback_uid,
bool need_unlink = false;
unsigned int vfoflags = 0;
int fd = -1;
- int path_shared = virFileIsSharedFS(path);
+ /* Note that it would be pointless to pass
+ * virQEMUDriverConfig.sharedFilesystems here, since those
+ * listed there are by definition paths that can be accessed
+ * as local from the current host. Thus, a second attempt at
+ * opening the file would not make a difference */
+ int path_shared = virFileIsSharedFS(path, NULL);
uid_t uid = geteuid();
gid_t gid = getegid();
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index ed5976d1f7..dc1bb56237 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -165,7 +165,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver,
virDomainTPMDef *tpm = def->tpms[i];
if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)
- qemuExtTPMCleanupHost(tpm, flags, outgoingMigration);
+ qemuExtTPMCleanupHost(driver, tpm, flags, outgoingMigration);
}
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1faab5dd23..0d496e19fe 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1429,6 +1429,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
unsigned int flags)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ virQEMUDriver *driver = priv->driver;
int nsnapshots;
int pauseReason;
size_t i;
@@ -1599,7 +1600,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
}
}
- if (qemuTPMHasSharedStorage(vm->def)&&
+ if (qemuTPMHasSharedStorage(driver, vm->def) &&
!qemuTPMCanMigrateSharedStorage(vm->def)) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("the running swtpm does not support migration with
shared storage"));
@@ -1611,20 +1612,23 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
}
static bool
-qemuMigrationSrcIsSafe(virDomainDef *def,
- virQEMUCaps *qemuCaps,
+qemuMigrationSrcIsSafe(virDomainObj *vm,
size_t nmigrate_disks,
const char **migrate_disks,
unsigned int flags)
{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ virQEMUCaps *qemuCaps = priv->qemuCaps;
+ virQEMUDriver *driver = priv->driver;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
bool storagemigration = flags & (VIR_MIGRATE_NON_SHARED_DISK |
VIR_MIGRATE_NON_SHARED_INC);
size_t i;
int rc;
- for (i = 0; i < def->ndisks; i++) {
- virDomainDiskDef *disk = def->disks[i];
+ for (i = 0; i < vm->def->ndisks; i++) {
+ virDomainDiskDef *disk = vm->def->disks[i];
const char *src = virDomainDiskGetSource(disk);
virStorageType actualType = virStorageSourceGetActualType(disk->src);
bool unsafe = false;
@@ -1643,7 +1647,7 @@ qemuMigrationSrcIsSafe(virDomainDef *def,
/* However, disks on local FS (e.g. ext4) are not safe. */
switch (actualType) {
case VIR_STORAGE_TYPE_FILE:
- if ((rc = virFileIsSharedFS(src)) < 0) {
+ if ((rc = virFileIsSharedFS(src, cfg->sharedFilesystems)) < 0) {
return false;
} else if (rc == 0) {
unsafe = true;
@@ -2604,8 +2608,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
return NULL;
if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) &&
- !qemuMigrationSrcIsSafe(vm->def, priv->qemuCaps,
- nmigrate_disks, migrate_disks, flags))
+ !qemuMigrationSrcIsSafe(vm, nmigrate_disks, migrate_disks, flags))
return NULL;
if (flags & VIR_MIGRATE_POSTCOPY &&
@@ -6066,7 +6069,6 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver,
int ret = -1;
virErrorPtr orig_err = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- qemuDomainObjPrivate *priv = vm->privateData;
qemuDomainJobPrivate *jobPriv = vm->job->privateData;
if (flags & VIR_MIGRATE_POSTCOPY_RESUME) {
@@ -6091,8 +6093,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver,
goto endjob;
if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) &&
- !qemuMigrationSrcIsSafe(vm->def, priv->qemuCaps,
- nmigrate_disks, migrate_disks, flags))
+ !qemuMigrationSrcIsSafe(vm, nmigrate_disks, migrate_disks, flags))
goto endjob;
qemuMigrationSrcStoreDomainState(vm);
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 4aaa863ae9..996c95acc0 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -38,15 +38,18 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
{
int ret = -1;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetAllLabel(driver->securityManager,
+ cfg->sharedFilesystems,
vm->def,
incomingPath,
priv->chardevStdioLogd,
@@ -70,6 +73,7 @@ qemuSecurityRestoreAllLabel(virQEMUDriver *driver,
bool migrated)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
bool transactionStarted = false;
/* In contrast to qemuSecuritySetAllLabel, do not use vm->pid
@@ -78,10 +82,12 @@ qemuSecurityRestoreAllLabel(virQEMUDriver *driver,
* domain's namespace is gone as qemu was the only process
* running there. We would not succeed in entering the
* namespace then. */
- if (virSecurityManagerTransactionStart(driver->securityManager) >= 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) >= 0)
transactionStarted = true;
virSecurityManagerRestoreAllLabel(driver->securityManager,
+ cfg->sharedFilesystems,
vm->def,
migrated,
priv->chardevStdioLogd);
@@ -103,6 +109,7 @@ qemuSecuritySetImageLabel(virQEMUDriver *driver,
bool chainTop)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
virSecurityDomainImageLabelFlags labelFlags = 0;
@@ -116,10 +123,12 @@ qemuSecuritySetImageLabel(virQEMUDriver *driver,
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetImageLabel(driver->securityManager,
+ cfg->sharedFilesystems,
vm->def, src, labelFlags) < 0)
goto cleanup;
@@ -141,6 +150,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriver *driver,
bool backingChain)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
virSecurityDomainImageLabelFlags labelFlags = 0;
@@ -151,10 +161,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriver *driver,
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreImageLabel(driver->securityManager,
+ cfg->sharedFilesystems,
vm->def, src, labelFlags) < 0)
goto cleanup;
@@ -176,6 +188,7 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver,
virStorageSource *dst)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (!priv->rememberOwner)
@@ -184,7 +197,9 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver,
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- return virSecurityManagerMoveImageMetadata(driver->securityManager, pid, src,
dst);
+ return virSecurityManagerMoveImageMetadata(driver->securityManager,
+ cfg->sharedFilesystems,
+ pid, src, dst);
}
@@ -194,13 +209,15 @@ qemuSecuritySetHostdevLabel(virQEMUDriver *driver,
virDomainHostdevDef *hostdev)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetHostdevLabel(driver->securityManager,
@@ -226,13 +243,15 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriver *driver,
virDomainHostdevDef *hostdev)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
@@ -258,13 +277,15 @@ qemuSecuritySetMemoryLabel(virQEMUDriver *driver,
virDomainMemoryDef *mem)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetMemoryLabel(driver->securityManager,
@@ -289,13 +310,15 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriver *driver,
virDomainMemoryDef *mem)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreMemoryLabel(driver->securityManager,
@@ -320,13 +343,15 @@ qemuSecuritySetInputLabel(virDomainObj *vm,
{
qemuDomainObjPrivate *priv = vm->privateData;
virQEMUDriver *driver = priv->driver;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetInputLabel(driver->securityManager,
@@ -351,13 +376,15 @@ qemuSecurityRestoreInputLabel(virDomainObj *vm,
{
qemuDomainObjPrivate *priv = vm->privateData;
virQEMUDriver *driver = priv->driver;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreInputLabel(driver->securityManager,
@@ -383,12 +410,14 @@ qemuSecuritySetChardevLabel(virQEMUDriver *driver,
{
int ret = -1;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetChardevLabel(driver->securityManager,
@@ -415,12 +444,14 @@ qemuSecurityRestoreChardevLabel(virQEMUDriver *driver,
{
int ret = -1;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreChardevLabel(driver->securityManager,
@@ -446,12 +477,14 @@ qemuSecuritySetNetdevLabel(virQEMUDriver *driver,
{
int ret = -1;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetNetdevLabel(driver->securityManager,
@@ -476,12 +509,14 @@ qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver,
{
int ret = -1;
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreNetdevLabel(driver->securityManager,
@@ -505,9 +540,11 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver,
bool setTPMStateLabel)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
int ret = -1;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerSetTPMLabels(driver->securityManager,
@@ -531,9 +568,11 @@ qemuSecurityRestoreTPMLabels(virQEMUDriver *driver,
bool restoreTPMStateLabel)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
int ret = -1;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerRestoreTPMLabels(driver->securityManager,
@@ -558,13 +597,15 @@ qemuSecurityDomainSetPathLabel(virQEMUDriver *driver,
bool allowSubtree)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
@@ -590,13 +631,15 @@ qemuSecurityDomainRestorePathLabel(virQEMUDriver *driver,
const char *path)
{
qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
pid_t pid = -1;
int ret = -1;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerDomainRestorePathLabel(driver->securityManager,
@@ -634,6 +677,7 @@ qemuSecurityDomainSetMountNSPathLabel(virQEMUDriver *driver,
const char *path)
{
int ret = -1;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
VIR_DEBUG("Not labeling '%s': mount namespace disabled for domain
'%s'",
@@ -641,7 +685,8 @@ qemuSecurityDomainSetMountNSPathLabel(virQEMUDriver *driver,
return 1;
}
- if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(driver->securityManager,
+ cfg->sharedFilesystems) < 0)
goto cleanup;
if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index bf0c6bcb0d..cdf4bfbad2 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -538,6 +538,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
* @privileged: whether we are running in privileged mode
* @swtpm_user: The uid for the swtpm to run as (drop privileges to from root)
* @swtpm_group: The gid for the swtpm to run as
+ * @sharedFilesystems: list of filesystem to consider shared
* @incomingMigration: whether we have an incoming migration
*
* Create the virCommand use for starting the emulator
@@ -551,6 +552,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
bool privileged,
uid_t swtpm_user,
gid_t swtpm_group,
+ char *const *sharedFilesystems,
bool incomingMigration)
{
g_autoptr(virCommand) cmd = NULL;
@@ -568,7 +570,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
/* Do not create storage and run swtpm_setup on incoming migration over
* shared storage
*/
- on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath) == 1;
+ on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath,
sharedFilesystems) == 1;
if (incomingMigration && on_shared_storage)
create_storage = false;
@@ -734,6 +736,7 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm,
/**
* qemuTPMEmulatorCleanupHost:
+ * @driver: QEMU driver
* @tpm: TPM definition
* @flags: flags indicating whether to keep or remove TPM persistent state
* @outgoingMigration: whether cleanup is due to an outgoing migration
@@ -741,15 +744,18 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm,
* Clean up persistent storage for the swtpm.
*/
static void
-qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm,
+qemuTPMEmulatorCleanupHost(virQEMUDriver *driver,
+ virDomainTPMDef *tpm,
virDomainUndefineFlagsValues flags,
bool outgoingMigration)
{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
/* Never remove the state in case of outgoing migration with shared
* storage.
*/
if (outgoingMigration &&
- virFileIsSharedFS(tpm->data.emulator.storagepath) == 1)
+ virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems)
== 1)
return;
/*
@@ -935,6 +941,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
driver->privileged,
cfg->swtpm_user,
cfg->swtpm_group,
+ cfg->sharedFilesystems,
incomingMigration)))
return -1;
@@ -950,7 +957,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virCommandSetErrorFD(cmd, &errfd);
if (incomingMigration &&
- virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) {
+ virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems)
== 1) {
/* security labels must have been set up on source already */
setTPMStateLabel = false;
}
@@ -1010,8 +1017,10 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
bool
-qemuTPMHasSharedStorage(virDomainDef *def)
+qemuTPMHasSharedStorage(virQEMUDriver *driver,
+ virDomainDef *def)
{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
size_t i;
for (i = 0; i < def->ntpms; i++) {
@@ -1019,7 +1028,8 @@ qemuTPMHasSharedStorage(virDomainDef *def)
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_EMULATOR:
- return virFileIsSharedFS(tpm->data.emulator.storagepath) == 1;
+ return virFileIsSharedFS(tpm->data.emulator.storagepath,
+ cfg->sharedFilesystems) == 1;
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
case VIR_DOMAIN_TPM_TYPE_LAST:
@@ -1097,11 +1107,12 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver,
void
-qemuExtTPMCleanupHost(virDomainTPMDef *tpm,
+qemuExtTPMCleanupHost(virQEMUDriver *driver,
+ virDomainTPMDef *tpm,
virDomainUndefineFlagsValues flags,
bool outgoingMigration)
{
- qemuTPMEmulatorCleanupHost(tpm, flags, outgoingMigration);
+ qemuTPMEmulatorCleanupHost(driver, tpm, flags, outgoingMigration);
}
@@ -1133,7 +1144,7 @@ qemuExtTPMStop(virQEMUDriver *driver,
return;
qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName);
- if (outgoingMigration && qemuTPMHasSharedStorage(vm->def))
+ if (outgoingMigration && qemuTPMHasSharedStorage(driver, vm->def))
restoreTPMStateLabel = false;
if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel) < 0)
diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h
index 33ba5d2268..3071dc3f71 100644
--- a/src/qemu/qemu_tpm.h
+++ b/src/qemu/qemu_tpm.h
@@ -35,10 +35,11 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver,
ATTRIBUTE_NONNULL(3)
G_GNUC_WARN_UNUSED_RESULT;
-void qemuExtTPMCleanupHost(virDomainTPMDef *tpm,
+void qemuExtTPMCleanupHost(virQEMUDriver *driver,
+ virDomainTPMDef *tpm,
virDomainUndefineFlagsValues flags,
bool outgoingMigration)
- ATTRIBUTE_NONNULL(1);
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuExtTPMStart(virQEMUDriver *driver,
virDomainObj *vm,
@@ -59,8 +60,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
G_GNUC_WARN_UNUSED_RESULT;
-bool qemuTPMHasSharedStorage(virDomainDef *def)
- ATTRIBUTE_NONNULL(1)
+bool qemuTPMHasSharedStorage(virQEMUDriver *driver,
+ virDomainDef *def)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
G_GNUC_WARN_UNUSED_RESULT;
bool qemuTPMCanMigrateSharedStorage(virDomainDef *def)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index c1dc859751..269dcb85e8 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -433,6 +433,7 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED,
static int
AppArmorSetSecurityAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
const char *incomingPath,
bool chardevStdioLogd G_GNUC_UNUSED,
@@ -507,6 +508,7 @@ AppArmorReleaseSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED,
static int
AppArmorRestoreSecurityAllLabel(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
bool migrated G_GNUC_UNUSED,
bool chardevStdioLogd G_GNUC_UNUSED)
@@ -625,6 +627,7 @@ AppArmorClearSecuritySocketLabel(virSecurityManager *mgr
G_GNUC_UNUSED,
/* Called when hotplugging */
static int
AppArmorRestoreSecurityImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
@@ -729,6 +732,7 @@ AppArmorRestoreInputLabel(virSecurityManager *mgr,
/* Called when hotplugging */
static int
AppArmorSetSecurityImageLabelInternal(virSecurityManager *mgr,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
virStorageSource *src)
{
@@ -762,6 +766,7 @@ AppArmorSetSecurityImageLabelInternal(virSecurityManager *mgr,
static int
AppArmorSetSecurityImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
@@ -777,7 +782,8 @@ AppArmorSetSecurityImageLabel(virSecurityManager *mgr,
return 0;
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
- if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
+ if (AppArmorSetSecurityImageLabelInternal(mgr, sharedFilesystems,
+ def, n) < 0)
return -1;
}
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4e850e219e..9957e2663e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -79,6 +79,7 @@ struct _virSecurityDACChownItem {
typedef struct _virSecurityDACChownList virSecurityDACChownList;
struct _virSecurityDACChownList {
virSecurityManager *manager;
+ char **sharedFilesystems;
virSecurityDACChownItem **items;
size_t nItems;
bool lock;
@@ -137,6 +138,7 @@ virSecurityDACChownListFree(void *opaque)
virSecurityDACChownItemFree(list->items[i]);
g_free(list->items);
virObjectUnref(list->manager);
+ g_strfreev(list->sharedFilesystems);
g_free(list);
}
@@ -228,7 +230,9 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED,
VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
}
- if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
+ if (!(state = virSecurityManagerMetadataLock(list->manager,
+ list->sharedFilesystems,
+ paths, npaths)))
return -1;
for (i = 0; i < list->nItems; i++) {
@@ -538,6 +542,7 @@ virSecurityDACPreFork(virSecurityManager *mgr)
/**
* virSecurityDACTransactionStart:
* @mgr: security manager
+ * @sharedFilesystems: list of filesystem to consider shared
*
* Starts a new transaction. In transaction nothing is chown()-ed until
* TransactionCommit() is called. This is implemented as a list that is
@@ -549,7 +554,8 @@ virSecurityDACPreFork(virSecurityManager *mgr)
* -1 otherwise.
*/
static int
-virSecurityDACTransactionStart(virSecurityManager *mgr)
+virSecurityDACTransactionStart(virSecurityManager *mgr,
+ char *const *sharedFilesystems)
{
g_autoptr(virSecurityDACChownList) list = NULL;
@@ -562,6 +568,7 @@ virSecurityDACTransactionStart(virSecurityManager *mgr)
list = g_new0(virSecurityDACChownList, 1);
list->manager = virObjectRef(mgr);
+ list->sharedFilesystems = g_strdupv((char **) sharedFilesystems);
if (virThreadLocalSet(&chownList, list) < 0) {
virReportSystemError(errno, "%s",
@@ -864,6 +871,7 @@ virSecurityDACRestoreFileLabel(virSecurityManager *mgr,
static int
virSecurityDACSetImageLabelInternal(virSecurityManager *mgr,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
virStorageSource *src,
virStorageSource *parent,
@@ -943,6 +951,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManager *mgr,
static int
virSecurityDACSetImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -953,7 +962,8 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr,
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;
- if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0)
+ if (virSecurityDACSetImageLabelInternal(mgr, sharedFilesystems,
+ def, n, parent, isChainTop) < 0)
return -1;
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
@@ -967,6 +977,7 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr,
static int
virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
bool migrated)
@@ -1009,7 +1020,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr,
if (!src->path)
return 0;
- if ((rc = virFileIsSharedFS(src->path)) < 0)
+ if ((rc = virFileIsSharedFS(src->path, sharedFilesystems)) < 0)
return -1;
}
@@ -1043,16 +1054,19 @@ virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr,
static int
virSecurityDACRestoreImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
{
- return virSecurityDACRestoreImageLabelInt(mgr, def, src, false);
+ return virSecurityDACRestoreImageLabelInt(mgr, sharedFilesystems,
+ def, src, false);
}
struct virSecurityDACMoveImageMetadataData {
virSecurityManager *mgr;
+ char **sharedFilesystems;
const char *src;
const char *dst;
};
@@ -1067,7 +1081,9 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
virSecurityManagerMetadataLockState *state;
int ret;
- if (!(state = virSecurityManagerMetadataLock(data->mgr, paths,
G_N_ELEMENTS(paths))))
+ if (!(state = virSecurityManagerMetadataLock(data->mgr,
+ data->sharedFilesystems,
+ paths, G_N_ELEMENTS(paths))))
return -1;
ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst);
@@ -1084,12 +1100,17 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
static int
virSecurityDACMoveImageMetadata(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst)
{
virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr);
- struct virSecurityDACMoveImageMetadataData data = { .mgr = mgr, 0 };
+ struct virSecurityDACMoveImageMetadataData data = {
+ .mgr = mgr,
+ .sharedFilesystems = (char **) sharedFilesystems,
+ 0
+ };
int rc;
/* If dynamicOwnership is turned off, or owner remembering is
@@ -1888,6 +1909,7 @@ virSecurityDACRestoreSysinfoLabel(virSecurityManager *mgr,
static int
virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
bool migrated,
bool chardevStdioLogd)
@@ -1912,6 +1934,7 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
for (i = 0; i < def->ndisks; i++) {
if (virSecurityDACRestoreImageLabelInt(mgr,
+ sharedFilesystems,
def,
def->disks[i]->src,
migrated) < 0)
@@ -1969,7 +1992,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
}
if (def->os.loader && def->os.loader->nvram) {
- if (virSecurityDACRestoreImageLabelInt(mgr, def, def->os.loader->nvram,
+ if (virSecurityDACRestoreImageLabelInt(mgr, sharedFilesystems,
+ def, def->os.loader->nvram,
migrated) < 0)
rc = -1;
}
@@ -2111,6 +2135,7 @@ virSecurityDACSetSysinfoLabel(virSecurityManager *mgr,
static int
virSecurityDACSetAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
const char *incomingPath G_GNUC_UNUSED,
bool chardevStdioLogd,
@@ -2136,7 +2161,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
/* XXX fixme - we need to recursively label the entire tree :-( */
if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR)
continue;
- if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src,
+ if (virSecurityDACSetImageLabel(mgr, sharedFilesystems,
+ def, def->disks[i]->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |
VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) <
0)
return -1;
@@ -2195,7 +2221,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
}
if (def->os.loader && def->os.loader->nvram) {
- if (virSecurityDACSetImageLabel(mgr, def, def->os.loader->nvram,
+ if (virSecurityDACSetImageLabel(mgr, sharedFilesystems,
+ def, def->os.loader->nvram,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |
VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) <
0)
return -1;
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index aa1fb2125d..2956e002ff 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -46,7 +46,8 @@ typedef const char *(*virSecurityDriverGetBaseLabel) (virSecurityManager
*mgr,
typedef int (*virSecurityDriverPreFork) (virSecurityManager *mgr);
-typedef int (*virSecurityDriverTransactionStart) (virSecurityManager *mgr);
+typedef int (*virSecurityDriverTransactionStart) (virSecurityManager *mgr,
+ char *const *sharedFilesystems);
typedef int (*virSecurityDriverTransactionCommit) (virSecurityManager *mgr,
pid_t pid,
bool lock);
@@ -80,11 +81,13 @@ typedef int (*virSecurityDomainReserveLabel) (virSecurityManager
*mgr,
typedef int (*virSecurityDomainReleaseLabel) (virSecurityManager *mgr,
virDomainDef *sec);
typedef int (*virSecurityDomainSetAllLabel) (virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *sec,
const char *incomingPath,
bool chardevStdioLogd,
bool migrated);
typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
bool migrated,
bool chardevStdioLogd);
@@ -113,14 +116,17 @@ typedef int (*virSecurityDomainSetHugepages) (virSecurityManager
*mgr,
const char *path);
typedef int (*virSecurityDomainSetImageLabel) (virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags);
typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags
flags);
typedef int (*virSecurityDomainMoveImageMetadata) (virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst);
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 24f2f3d3dc..3e341be007 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -244,6 +244,7 @@ virSecurityManagerPostFork(virSecurityManager *mgr)
/**
* virSecurityManagerTransactionStart:
* @mgr: security manager
+ * @sharedFilesystems: list of filesystem to consider shared
*
* Starts a new transaction. In transaction nothing is changed security
* label until virSecurityManagerTransactionCommit() is called.
@@ -252,14 +253,15 @@ virSecurityManagerPostFork(virSecurityManager *mgr)
* -1 otherwise.
*/
int
-virSecurityManagerTransactionStart(virSecurityManager *mgr)
+virSecurityManagerTransactionStart(virSecurityManager *mgr,
+ char *const *sharedFilesystems)
{
VIR_LOCK_GUARD lock = virObjectLockGuard(mgr);
if (!mgr->drv->transactionStart)
return 0;
- return mgr->drv->transactionStart(mgr);
+ return mgr->drv->transactionStart(mgr, sharedFilesystems);
}
@@ -402,6 +404,7 @@ virSecurityManagerGetPrivileged(virSecurityManager *mgr)
/**
* virSecurityManagerRestoreImageLabel:
* @mgr: security manager object
+ * @sharedFilesystems: list of filesystem to consider shared
* @vm: domain definition object
* @src: disk source definition to operate on
* @flags: bitwise or of 'virSecurityDomainImageLabelFlags'
@@ -412,6 +415,7 @@ virSecurityManagerGetPrivileged(virSecurityManager *mgr)
*/
int
virSecurityManagerRestoreImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -423,13 +427,15 @@ virSecurityManagerRestoreImageLabel(virSecurityManager *mgr,
return -1;
}
- return mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, flags);
+ return mgr->drv->domainRestoreSecurityImageLabel(mgr, sharedFilesystems,
+ vm, src, flags);
}
/**
* virSecurityManagerMoveImageMetadata:
* @mgr: security manager
+ * @sharedFilesystems: list of filesystem to consider shared
* @pid: domain's PID
* @src: source of metadata
* @dst: destination to move metadata to
@@ -449,6 +455,7 @@ virSecurityManagerRestoreImageLabel(virSecurityManager *mgr,
*/
int
virSecurityManagerMoveImageMetadata(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst)
@@ -458,7 +465,8 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr,
if (!mgr->drv->domainMoveImageMetadata)
return 0;
- return mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst);
+ return mgr->drv->domainMoveImageMetadata(mgr, sharedFilesystems,
+ pid, src, dst);
}
@@ -510,6 +518,7 @@ virSecurityManagerClearSocketLabel(virSecurityManager *mgr,
/**
* virSecurityManagerSetImageLabel:
* @mgr: security manager object
+ * @sharedFilesystems: list of filesystem to consider shared
* @vm: domain definition object
* @src: disk source definition to operate on
* @flags: bitwise or of 'virSecurityDomainImageLabelFlags'
@@ -520,6 +529,7 @@ virSecurityManagerClearSocketLabel(virSecurityManager *mgr,
*/
int
virSecurityManagerSetImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -531,7 +541,8 @@ virSecurityManagerSetImageLabel(virSecurityManager *mgr,
return -1;
}
- return mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, flags);
+ return mgr->drv->domainSetSecurityImageLabel(mgr, sharedFilesystems,
+ vm, src, flags);
}
@@ -816,6 +827,7 @@ int virSecurityManagerCheckAllLabel(virSecurityManager *mgr,
int
virSecurityManagerSetAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
const char *incomingPath,
bool chardevStdioLogd,
@@ -828,13 +840,15 @@ virSecurityManagerSetAllLabel(virSecurityManager *mgr,
return -1;
}
- return mgr->drv->domainSetSecurityAllLabel(mgr, vm, incomingPath,
+ return mgr->drv->domainSetSecurityAllLabel(mgr, sharedFilesystems,
+ vm, incomingPath,
chardevStdioLogd, migrated);
}
int
virSecurityManagerRestoreAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
bool migrated,
bool chardevStdioLogd)
@@ -846,7 +860,8 @@ virSecurityManagerRestoreAllLabel(virSecurityManager *mgr,
return -1;
}
- return mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated,
+ return mgr->drv->domainRestoreSecurityAllLabel(mgr, sharedFilesystems,
+ vm, migrated,
chardevStdioLogd);
}
@@ -1270,6 +1285,7 @@ cmpstringp(const void *p1,
/**
* virSecurityManagerMetadataLock:
* @mgr: security manager object
+ * @sharedFilesystems: list of filesystem to consider shared
* @paths: paths to lock
* @npaths: number of items in @paths array
*
@@ -1285,6 +1301,7 @@ cmpstringp(const void *p1,
*/
virSecurityManagerMetadataLockState *
virSecurityManagerMetadataLock(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems,
const char **paths,
size_t npaths)
{
@@ -1355,7 +1372,7 @@ virSecurityManagerMetadataLock(virSecurityManager *mgr
G_GNUC_UNUSED,
}
#endif /* !WIN32 */
- if (virFileIsSharedFS(p)) {
+ if (virFileIsSharedFS(p, sharedFilesystems)) {
/* Probably a root squashed NFS. */
continue;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index a416af3215..c622fe5882 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -81,7 +81,8 @@ virSecurityManager *virSecurityManagerNewDAC(const char *virtDriver,
int virSecurityManagerPreFork(virSecurityManager *mgr);
void virSecurityManagerPostFork(virSecurityManager *mgr);
-int virSecurityManagerTransactionStart(virSecurityManager *mgr);
+int virSecurityManagerTransactionStart(virSecurityManager *mgr,
+ char *const *sharedFilesystems);
int virSecurityManagerTransactionCommit(virSecurityManager *mgr,
pid_t pid,
bool lock);
@@ -129,11 +130,13 @@ int virSecurityManagerReleaseLabel(virSecurityManager *mgr,
int virSecurityManagerCheckAllLabel(virSecurityManager *mgr,
virDomainDef *sec);
int virSecurityManagerSetAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *sec,
const char *incomingPath,
bool chardevStdioLogd,
bool migrated);
int virSecurityManagerRestoreAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
bool migrated,
bool chardevStdioLogd);
@@ -168,14 +171,17 @@ typedef enum {
} virSecurityDomainImageLabelFlags;
int virSecurityManagerSetImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags);
int virSecurityManagerRestoreImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags);
int virSecurityManagerMoveImageMetadata(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst);
@@ -244,6 +250,7 @@ struct _virSecurityManagerMetadataLockState {
virSecurityManagerMetadataLockState *
virSecurityManagerMetadataLock(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
const char **paths,
size_t npaths);
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index 1413f43d57..e6e337a49d 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -116,6 +116,7 @@ virSecurityDomainReleaseLabelNop(virSecurityManager *mgr
G_GNUC_UNUSED,
static int
virSecurityDomainSetAllLabelNop(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *sec G_GNUC_UNUSED,
const char *incomingPath G_GNUC_UNUSED,
bool chardevStdioLogd G_GNUC_UNUSED,
@@ -126,6 +127,7 @@ virSecurityDomainSetAllLabelNop(virSecurityManager *mgr
G_GNUC_UNUSED,
static int
virSecurityDomainRestoreAllLabelNop(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *vm G_GNUC_UNUSED,
bool migrated G_GNUC_UNUSED,
bool chardevStdioLogd G_GNUC_UNUSED)
@@ -189,6 +191,7 @@ virSecurityGetBaseLabel(virSecurityManager *mgr G_GNUC_UNUSED,
static int
virSecurityDomainRestoreImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def G_GNUC_UNUSED,
virStorageSource *src G_GNUC_UNUSED,
virSecurityDomainImageLabelFlags flags
G_GNUC_UNUSED)
@@ -198,6 +201,7 @@ virSecurityDomainRestoreImageLabelNop(virSecurityManager *mgr
G_GNUC_UNUSED,
static int
virSecurityDomainSetImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def G_GNUC_UNUSED,
virStorageSource *src G_GNUC_UNUSED,
virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
@@ -207,6 +211,7 @@ virSecurityDomainSetImageLabelNop(virSecurityManager *mgr
G_GNUC_UNUSED,
static int
virSecurityDomainMoveImageMetadataNop(virSecurityManager *mgr G_GNUC_UNUSED,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
pid_t pid G_GNUC_UNUSED,
virStorageSource *src G_GNUC_UNUSED,
virStorageSource *dst G_GNUC_UNUSED)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index a4915dbc89..4f1874644f 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -77,6 +77,7 @@ struct _virSecuritySELinuxContextItem {
typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList;
struct _virSecuritySELinuxContextList {
virSecurityManager *manager;
+ char **sharedFilesystems;
virSecuritySELinuxContextItem **items;
size_t nItems;
bool lock;
@@ -141,6 +142,7 @@ virSecuritySELinuxContextListFree(void *opaque)
g_free(list->items);
virObjectUnref(list->manager);
+ g_strfreev(list->sharedFilesystems);
g_free(list);
}
@@ -254,7 +256,9 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED,
VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
}
- if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths)))
+ if (!(state = virSecurityManagerMetadataLock(list->manager,
+ list->sharedFilesystems,
+ paths, npaths)))
goto cleanup;
for (i = 0; i < list->nItems; i++) {
@@ -1102,6 +1106,7 @@ virSecuritySELinuxGetDOI(virSecurityManager *mgr G_GNUC_UNUSED)
/**
* virSecuritySELinuxTransactionStart:
* @mgr: security manager
+ * @sharedFilesystems: list of filesystem to consider shared
*
* Starts a new transaction. In transaction nothing is changed context
* until TransactionCommit() is called. This is implemented as a list
@@ -1114,7 +1119,8 @@ virSecuritySELinuxGetDOI(virSecurityManager *mgr G_GNUC_UNUSED)
* -1 otherwise.
*/
static int
-virSecuritySELinuxTransactionStart(virSecurityManager *mgr)
+virSecuritySELinuxTransactionStart(virSecurityManager *mgr,
+ char *const *sharedFilesystems)
{
virSecuritySELinuxContextList *list;
@@ -1128,6 +1134,7 @@ virSecuritySELinuxTransactionStart(virSecurityManager *mgr)
list = g_new0(virSecuritySELinuxContextList, 1);
list->manager = virObjectRef(mgr);
+ list->sharedFilesystems = g_strdupv((char **) sharedFilesystems);
if (virThreadLocalSet(&contextList, list) < 0) {
virReportSystemError(errno, "%s",
@@ -1777,6 +1784,7 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManager *mgr,
static int
virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
bool migrated)
@@ -1835,7 +1843,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr,
if (!src->path)
return 0;
- if ((rc = virFileIsSharedFS(src->path)) < 0)
+ if ((rc = virFileIsSharedFS(src->path, sharedFilesystems)) < 0)
return -1;
}
@@ -1867,16 +1875,19 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr,
static int
virSecuritySELinuxRestoreImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags
G_GNUC_UNUSED)
{
- return virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false);
+ return virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems,
+ def, src, false);
}
static int
virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
+ char *const *sharedFilesystems G_GNUC_UNUSED,
virDomainDef *def,
virStorageSource *src,
virStorageSource *parent,
@@ -1983,6 +1994,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
static int
virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -1993,7 +2005,9 @@ virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;
- if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, isChainTop) <
0)
+ if (virSecuritySELinuxSetImageLabelInternal(mgr, sharedFilesystems,
+ def, n, parent,
+ isChainTop) < 0)
return -1;
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
@@ -2008,6 +2022,7 @@ virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
struct virSecuritySELinuxMoveImageMetadataData {
virSecurityManager *mgr;
+ char **sharedFilesystems;
const char *src;
const char *dst;
};
@@ -2022,7 +2037,9 @@ virSecuritySELinuxMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
virSecurityManagerMetadataLockState *state;
int ret;
- if (!(state = virSecurityManagerMetadataLock(data->mgr, paths,
G_N_ELEMENTS(paths))))
+ if (!(state = virSecurityManagerMetadataLock(data->mgr,
+ data->sharedFilesystems,
+ paths, G_N_ELEMENTS(paths))))
return -1;
ret = virSecurityMoveRememberedLabel(SECURITY_SELINUX_NAME, data->src,
data->dst);
@@ -2039,11 +2056,16 @@ virSecuritySELinuxMoveImageMetadataHelper(pid_t pid
G_GNUC_UNUSED,
static int
virSecuritySELinuxMoveImageMetadata(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst)
{
- struct virSecuritySELinuxMoveImageMetadataData data = { .mgr = mgr, 0 };
+ struct virSecuritySELinuxMoveImageMetadataData data = {
+ .mgr = mgr,
+ .sharedFilesystems = (char **) sharedFilesystems,
+ 0
+ };
int rc;
if (src && virStorageSourceIsLocalStorage(src))
@@ -2820,6 +2842,7 @@ virSecuritySELinuxRestoreSysinfoLabel(virSecurityManager *mgr,
static int
virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
bool migrated,
bool chardevStdioLogd)
@@ -2844,7 +2867,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr,
for (i = 0; i < def->ndisks; i++) {
virDomainDiskDef *disk = def->disks[i];
- if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src,
+ if (virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems,
+ def, disk->src,
migrated) < 0)
rc = -1;
}
@@ -2890,7 +2914,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr,
}
if (def->os.loader && def->os.loader->nvram) {
- if (virSecuritySELinuxRestoreImageLabelInt(mgr, def,
def->os.loader->nvram,
+ if (virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems,
+ def, def->os.loader->nvram,
migrated) < 0)
rc = -1;
}
@@ -3232,6 +3257,7 @@ virSecuritySELinuxSetSysinfoLabel(virSecurityManager *mgr,
static int
virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *def,
const char *incomingPath G_GNUC_UNUSED,
bool chardevStdioLogd,
@@ -3259,7 +3285,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
def->disks[i]->dst);
continue;
}
- if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src,
+ if (virSecuritySELinuxSetImageLabel(mgr, sharedFilesystems,
+ def, def->disks[i]->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN
|
VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP)
< 0)
return -1;
@@ -3309,7 +3336,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
}
if (def->os.loader && def->os.loader->nvram) {
- if (virSecuritySELinuxSetImageLabel(mgr, def, def->os.loader->nvram,
+ if (virSecuritySELinuxSetImageLabel(mgr, sharedFilesystems,
+ def, def->os.loader->nvram,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN
|
VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP)
< 0)
return -1;
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 369b5dd3a6..11800535b9 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -140,13 +140,15 @@ virSecurityStackPreFork(virSecurityManager *mgr)
static int
-virSecurityStackTransactionStart(virSecurityManager *mgr)
+virSecurityStackTransactionStart(virSecurityManager *mgr,
+ char *const *sharedFilesystems)
{
virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItem *item = priv->itemsHead;
for (; item; item = item->next) {
- if (virSecurityManagerTransactionStart(item->securityManager) < 0)
+ if (virSecurityManagerTransactionStart(item->securityManager,
+ sharedFilesystems) < 0)
goto rollback;
}
@@ -337,6 +339,7 @@ virSecurityStackRestoreHostdevLabel(virSecurityManager *mgr,
static int
virSecurityStackSetAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
const char *incomingPath,
bool chardevStdioLogd,
@@ -346,8 +349,9 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr,
virSecurityStackItem *item = priv->itemsHead;
for (; item; item = item->next) {
- if (virSecurityManagerSetAllLabel(item->securityManager, vm,
- incomingPath, chardevStdioLogd,
+ if (virSecurityManagerSetAllLabel(item->securityManager,
+ sharedFilesystems,
+ vm, incomingPath, chardevStdioLogd,
migrated) < 0)
goto rollback;
}
@@ -357,6 +361,7 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr,
rollback:
for (item = item->prev; item; item = item->prev) {
if (virSecurityManagerRestoreAllLabel(item->securityManager,
+ sharedFilesystems,
vm,
migrated,
chardevStdioLogd) < 0) {
@@ -373,6 +378,7 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr,
static int
virSecurityStackRestoreAllLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
bool migrated,
bool chardevStdioLogd)
@@ -382,8 +388,11 @@ virSecurityStackRestoreAllLabel(virSecurityManager *mgr,
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerRestoreAllLabel(item->securityManager, vm,
- migrated, chardevStdioLogd) < 0)
+ if (virSecurityManagerRestoreAllLabel(item->securityManager,
+ sharedFilesystems,
+ vm,
+ migrated,
+ chardevStdioLogd) < 0)
rc = -1;
}
@@ -638,6 +647,7 @@ virSecurityStackGetBaseLabel(virSecurityManager *mgr, int virtType)
static int
virSecurityStackSetImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -646,8 +656,9 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr,
virSecurityStackItem *item = priv->itemsHead;
for (; item; item = item->next) {
- if (virSecurityManagerSetImageLabel(item->securityManager, vm, src,
- flags) < 0)
+ if (virSecurityManagerSetImageLabel(item->securityManager,
+ sharedFilesystems,
+ vm, src, flags) < 0)
goto rollback;
}
@@ -656,6 +667,7 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr,
rollback:
for (item = item->prev; item; item = item->prev) {
if (virSecurityManagerRestoreImageLabel(item->securityManager,
+ sharedFilesystems,
vm,
src,
flags) < 0) {
@@ -672,6 +684,7 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr,
static int
virSecurityStackRestoreImageLabel(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
virDomainDef *vm,
virStorageSource *src,
virSecurityDomainImageLabelFlags flags)
@@ -682,6 +695,7 @@ virSecurityStackRestoreImageLabel(virSecurityManager *mgr,
for (; item; item = item->next) {
if (virSecurityManagerRestoreImageLabel(item->securityManager,
+ sharedFilesystems,
vm, src, flags) < 0)
rc = -1;
}
@@ -691,6 +705,7 @@ virSecurityStackRestoreImageLabel(virSecurityManager *mgr,
static int
virSecurityStackMoveImageMetadata(virSecurityManager *mgr,
+ char *const *sharedFilesystems,
pid_t pid,
virStorageSource *src,
virStorageSource *dst)
@@ -701,6 +716,7 @@ virSecurityStackMoveImageMetadata(virSecurityManager *mgr,
for (; item; item = item->next) {
if (virSecurityManagerMoveImageMetadata(item->securityManager,
+ sharedFilesystems,
pid, src, dst) < 0)
rc = -1;
}
diff --git a/src/util/virfile.c b/src/util/virfile.c
index deaf4555fd..3268866f8b 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2597,8 +2597,14 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
goto error;
/* On Linux we can also verify the FS-type of the
- * directory. (this is a NOP on other platforms). */
- if (virFileIsSharedFS(path) <= 0)
+ * directory. (this is a NOP on other platforms).
+ *
+ * Note that it would be pointless to pass
+ * virQEMUDriverConfig.sharedFilesystems here, since those
+ * listed there are by definition paths that can be accessed
+ * as local from the current host. Thus, a second attempt at
+ * opening the file would not make a difference */
+ if (virFileIsSharedFS(path, NULL) <= 0)
goto error;
}
@@ -3795,7 +3801,8 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
return NULL;
}
-int virFileIsSharedFS(const char *path)
+int virFileIsSharedFS(const char *path,
+ char *const *overrides G_GNUC_UNUSED)
{
return virFileIsSharedFSType(path,
VIR_FILE_SHFS_NFS |
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 56fe309bce..3fdd7f526c 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -235,7 +235,8 @@ enum {
};
int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1);
-int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1);
+int virFileIsSharedFS(const char *path,
+ char *const *overrides) ATTRIBUTE_NONNULL(1);
int virFileIsClusterFS(const char *path) ATTRIBUTE_NONNULL(1);
int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
int virFileIsCDROM(const char *path)
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 7b7cf53569..43db128b3a 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -270,7 +270,7 @@ testSELinuxLabeling(const void *opaque)
if (!(def = testSELinuxLoadDef(testname)))
goto cleanup;
- if (virSecurityManagerSetAllLabel(mgr, def, NULL, false, false) < 0)
+ if (virSecurityManagerSetAllLabel(mgr, NULL, def, NULL, false, false) < 0)
goto cleanup;
if (testSELinuxCheckLabels(files, nfiles) < 0)
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index 9fbfc37e56..e05925a321 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -313,7 +313,7 @@ testFileIsSharedFSType(const void *opaque G_GNUC_UNUSED)
goto cleanup;
}
- actual = virFileIsSharedFS(data->filename);
+ actual = virFileIsSharedFS(data->filename, NULL);
if (actual != data->expected) {
fprintf(stderr, "Unexpected FS type. Expected %d got %d\n",
--
2.44.0
7:17 a.m.
New subject: [PATCH v3 3/5] qemu: Propagate shared_filesystems
On Thu, May 02, 2024 at 19:39:40 +0200, Andrea Bolognani wrote:
virFileIsSharedFS() is the function that ultimately decides
whether a filesystem should be considered shared, but the list
of manually configured shared filesystems is part of the QEMU
driver's configuration, so we need to pass the information
through several layers in order to make use of it.
Note that with this change the list is propagated all the way
through, but its contents are still ignored, so the behavior
remains the same for now.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/lxc/lxc_controller.c | 3 +-
src/lxc/lxc_driver.c | 2 +-
src/lxc/lxc_process.c | 4 +-
src/qemu/qemu_domain.c | 7 ++-
src/qemu/qemu_extdevice.c | 2 +-
src/qemu/qemu_migration.c | 23 ++++-----
src/qemu/qemu_security.c | 85 ++++++++++++++++++++++++--------
src/qemu/qemu_tpm.c | 29 +++++++----
src/qemu/qemu_tpm.h | 10 ++--
src/security/security_apparmor.c | 8 ++-
src/security/security_dac.c | 47 ++++++++++++++----
src/security/security_driver.h | 8 ++-
src/security/security_manager.c | 33 ++++++++++---
src/security/security_manager.h | 9 +++-
src/security/security_nop.c | 5 ++
src/security/security_selinux.c | 50 ++++++++++++++-----
src/security/security_stack.c | 32 +++++++++---
src/util/virfile.c | 13 +++--
src/util/virfile.h | 3 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/virfiletest.c | 2 +-
21 files changed, 281 insertions(+), 96 deletions(-)
A general note/nit for this patch:
I'd go with 'exportedFilesystems' instead of 'sharedFilesystems'
throughout this patch(set) for any case when the list contains only the
paths configured by user (from previous commit).
That way it'd be IMO more clear that the list contains only filesystems
which are local on this host, rather than having also anything that's
consiedered shared.
It IMO would make sense to consider that e.g. also for the name of the
config option.
[...]
@@ -1611,20 +1612,23 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
}
static bool
-qemuMigrationSrcIsSafe(virDomainDef *def,
- virQEMUCaps *qemuCaps,
+qemuMigrationSrcIsSafe(virDomainObj *vm,
size_t nmigrate_disks,
const char **migrate_disks,
unsigned int flags)
Sneaky refactor :)
{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ virQEMUCaps *qemuCaps = priv->qemuCaps;
+ virQEMUDriver *driver = priv->driver;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
bool storagemigration = flags & (VIR_MIGRATE_NON_SHARED_DISK |
VIR_MIGRATE_NON_SHARED_INC);
[...]
With the rename very strongly considered:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
8:48 a.m.
New subject: [PATCH v3 3/5] qemu: Propagate shared_filesystems
On Thu, May 09, 2024 at 02:17:21PM GMT, Peter Krempa wrote:
I'd go with 'exportedFilesystems' instead of
'sharedFilesystems'
throughout this patch(set) for any case when the list contains only the
paths configured by user (from previous commit).
Isn't that all cases? We never build a list where we add the
user-provided paths to those that have been detected as shared via
other means AFAICT.
That way it'd be IMO more clear that the list contains only
filesystems
which are local on this host, rather than having also anything that's
consiedered shared.
It IMO would make sense to consider that e.g. also for the name of the
config option.
I can certainly change the name, but if the goal is to make things
clearer to someone looking at a function in isolation I'm not
entirely sure "exportedFilesystems" will help much.
What about "userSharedFilesystems"? As in, filesystems that are to be
considered as shared specifically because the user told us to.
> @@ -1611,20 +1612,23 @@ qemuMigrationSrcIsAllowed(virDomainObj
*vm,
> }
>
> static bool
> -qemuMigrationSrcIsSafe(virDomainDef *def,
> - virQEMUCaps *qemuCaps,
> +qemuMigrationSrcIsSafe(virDomainObj *vm,
> size_t nmigrate_disks,
> const char **migrate_disks,
> unsigned int flags)
Sneaky refactor :)
Do you want me to split it off to a separate patch? I can do that.
--
Andrea Bolognani / Red Hat / Virtualization
9 a.m.
New subject: [PATCH v3 3/5] qemu: Propagate shared_filesystems
On Thu, May 09, 2024 at 06:48:36 -0700, Andrea Bolognani wrote:
On Thu, May 09, 2024 at 02:17:21PM GMT, Peter Krempa wrote:
> I'd go with 'exportedFilesystems' instead of
'sharedFilesystems'
> throughout this patch(set) for any case when the list contains only the
> paths configured by user (from previous commit).
Isn't that all cases? We never build a list where we add the
user-provided paths to those that have been detected as shared via
other means AFAICT.
> That way it'd be IMO more clear that the list contains only filesystems
> which are local on this host, rather than having also anything that's
> consiedered shared.
>
> It IMO would make sense to consider that e.g. also for the name of the
> config option.
I can certainly change the name, but if the goal is to make things
clearer to someone looking at a function in isolation I'm not
entirely sure "exportedFilesystems" will help much.
So I've considered 'exported' filesystem to be a local filesystem made
available to other hosts. Basically based on /etc/exports and how NFS
works.
What about "userSharedFilesystems"? As in, filesystems that
are to be
considered as shared specifically because the user told us to.
While 'shared' can be confused with something that is mounted from other
host where the existing function "IsSharedFs" works correctly.
Since this doesn't work only for exported filesystems, thus the name.
> > @@ -1611,20 +1612,23 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm,
> > }
> >
> > static bool
> > -qemuMigrationSrcIsSafe(virDomainDef *def,
> > - virQEMUCaps *qemuCaps,
> > +qemuMigrationSrcIsSafe(virDomainObj *vm,
> > size_t nmigrate_disks,
> > const char **migrate_disks,
> > unsigned int flags)
>
> Sneaky refactor :)
Do you want me to split it off to a separate patch? I can do that.
No, not needed :)
12:39 p.m.
New subject: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
If the local admin has explicitly declared that a certain
filesystem is to be considered shared, we should treat it as
such.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/util/virfile.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3268866f8b..45815919d6 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3801,9 +3801,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
return NULL;
}
+static bool
+virFileIsSharedFSOverride(const char *path,
+ char *const *overrides)
+{
+ g_autofree char *dirpath = NULL;
+ char *p = NULL;
+
+ if (!path || path[0] != '/' || !overrides)
+ return false;
+
+ if (g_strv_contains((const char *const *) overrides, path))
+ return true;
+
+ dirpath = g_strdup(path);
+
+ /* Continue until we've scanned the entire path */
+ while (p != dirpath) {
+
+ /* Find the last slash */
+ if ((p = strrchr(dirpath, '/')) == NULL)
+ break;
+
+ /* Truncate the path by overwriting the slash that we've just
+ * found with a null byte. If it is the very first slash in
+ * the path, we need to handle things slightly differently */
+ if (p == dirpath)
+ *(p+1) = '\0';
+ else
+ *p = '\0';
+
+ if (g_strv_contains((const char *const *) overrides, dirpath))
+ return true;
+ }
+
+ return false;
+}
+
int virFileIsSharedFS(const char *path,
- char *const *overrides G_GNUC_UNUSED)
+ char *const *overrides)
{
+ if (virFileIsSharedFSOverride(path, overrides))
+ return 1;
+
return virFileIsSharedFSType(path,
VIR_FILE_SHFS_NFS |
VIR_FILE_SHFS_GFS2 |
--
2.44.0
7:28 a.m.
New subject: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
If the local admin has explicitly declared that a certain
filesystem is to be considered shared, we should treat it as
such.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/util/virfile.c | 42 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3268866f8b..45815919d6 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3801,9 +3801,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
return NULL;
}
+static bool
+virFileIsSharedFSOverride(const char *path,
+ char *const *overrides)
+{
+ g_autofree char *dirpath = NULL;
+ char *p = NULL;
+
+ if (!path || path[0] != '/' || !overrides)
+ return false;
Per my comment on canonicalizing paths only when they're about to be
used.
I think you can also modify the algorithm to avoid the truncate&compare
operations to:
foreach override in overrides:
pc = canonicalize(path);
po = canonicalize(override);
if (STRPREFIX(pc, po))
return true;
Checking the full prefix on canonicalized paths is IIUC equivalent to
what you do below. (Okay perhaps except the case when user declares a
full to a single file as an exported override).
7:54 a.m.
New subject: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
On Thu, May 09, 2024 at 14:28:15 +0200, Peter Krempa wrote:
On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
[...]
foreach override in overrides:
pc = canonicalize(path);
po = canonicalize(override);
if (STRPREFIX(pc, po))
return true;
Checking the full prefix on canonicalized paths is IIUC equivalent to
what you do below. (Okay perhaps except the case when user declares a
full to a single file as an exported override).
To take the above into account we can perhaps do STRSKIP:
if ((tmp = STRSKIP(pc, po))) {
if (*tmp == '/' || *tmp == '\0)
return true;
}
8:54 a.m.
New subject: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
On Thu, May 09, 2024 at 02:28:15PM GMT, Peter Krempa wrote:
On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> +static bool
> +virFileIsSharedFSOverride(const char *path,
> + char *const *overrides)
> +{
> + g_autofree char *dirpath = NULL;
> + char *p = NULL;
> +
> + if (!path || path[0] != '/' || !overrides)
> + return false;
Per my comment on canonicalizing paths only when they're about to be
used.
Gotcha.
I think you can also modify the algorithm to avoid the
truncate&compare
operations to:
foreach override in overrides:
pc = canonicalize(path);
po = canonicalize(override);
if (STRPREFIX(pc, po))
return true;
I'll give it a try.
Checking the full prefix on canonicalized paths is IIUC equivalent
to
what you do below. (Okay perhaps except the case when user declares a
full to a single file as an exported override).
That isn't supposed to work anyway... If the current code allows it
then it will need to be fixed.
--
Andrea Bolognani / Red Hat / Virtualization
9:07 a.m.
New subject: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
On Thu, May 09, 2024 at 06:54:11 -0700, Andrea Bolognani wrote:
On Thu, May 09, 2024 at 02:28:15PM GMT, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> > +static bool
> > +virFileIsSharedFSOverride(const char *path,
> > + char *const *overrides)
> > +{
> > + g_autofree char *dirpath = NULL;
> > + char *p = NULL;
> > +
> > + if (!path || path[0] != '/' || !overrides)
> > + return false;
>
> Per my comment on canonicalizing paths only when they're about to be
> used.
Gotcha.
> I think you can also modify the algorithm to avoid the truncate&compare
> operations to:
>
>
> foreach override in overrides:
>
> pc = canonicalize(path);
> po = canonicalize(override);
>
> if (STRPREFIX(pc, po))
> return true;
I'll give it a try.
> Checking the full prefix on canonicalized paths is IIUC equivalent to
> what you do below. (Okay perhaps except the case when user declares a
> full to a single file as an exported override).
Okay, so firstly I wrote something else than I've thought.
The algorithm above has a bug if you declare an exported file such
as:
/path/to/ble
which is a file not a directory, and have VM with disk pointing to
/path/to/blesomething
the above code would mark it as shared based on the override. You need
to use the fixed version I've suggested in a reply.
That isn't supposed to work anyway... If the current code allows
it
then it will need to be fixed.
By going with the meaning that users can mark individual files as
exported, the original impl you've posted does support that:
+ if (g_strv_contains((const char *const *) overrides, path))
+ return true;
+
12:39 p.m.
New subject: [PATCH v3 5/5] qemu: Always set labels for TPM state
Up until this point, we have avoided setting labels for
incoming migration when the TPM state is stored on a shared
filesystem. This seems to make sense, because since the
underlying storage is shared surely the labels will be as
well.
There's one problem, though: when a guest is migrated, the
SELinux context for the destination process is different from
the one of the source process.
We haven't hit any issues with the current approach so far
because NFS doesn't support SELinux, so effectively it doesn't
matter whether relabeling happens or not: even if the SELinux
contexts of the source and target processes are different,
both will be able to access the storage.
Now that it's possible for the local admin to manually mark
exported directories as shared filesystems, however, things
can get problematic.
Consider the case in which one host (mig-one) exports its
local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the
same time bind-mounts it to /var/lib/libvirt/swtpm; another
host (mig-two) mounts the same filesystem to the same
location, this time via NFS. Additionally, in order to
allow migration in both directions, on mig-one the
/var/lib/libvirt/swtpm directory is listed in the
shared_filesystems qemu.conf option.
When migrating from mig-one to mig-two, things work just fine;
going in the opposite direction, however, results in an error:
# virsh migrate cirros qemu+ssh://mig-one/system
error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'):
qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with a TPM error
0x1f
qemu-system-x86_64: error while loading state for instance 0x0 of device
'tpm-emulator'
qemu-system-x86_64: load of migration failed: Input/output error
This is because the directory on mig-one is considered a
shared filesystem and thus labeling is skipped, resulting in
a SELinux denial.
The solution is quite simple: remove the check and always
relabel. We know that it's okay to do so not just because it
makes the error seen above go away, but also because no such
check currently exists for disks and other types of persistent
storage such as NVRAM files, which always get relabeled.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/qemu/qemu_tpm.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index cdf4bfbad2..e5a9fb8b16 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -929,7 +929,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
g_autofree char *pidfile = NULL;
virTimeBackOffVar timebackoff;
const unsigned long long timeout = 1000; /* ms */
- bool setTPMStateLabel = true;
pid_t pid = -1;
cfg = virQEMUDriverGetConfig(driver);
@@ -956,13 +955,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virCommandSetPidFile(cmd, pidfile);
virCommandSetErrorFD(cmd, &errfd);
- if (incomingMigration &&
- virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems)
== 1) {
- /* security labels must have been set up on source already */
- setTPMStateLabel = false;
- }
-
- if (qemuSecuritySetTPMLabels(driver, vm, setTPMStateLabel) < 0)
+ if (qemuSecuritySetTPMLabels(driver, vm, true) < 0)
return -1;
if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user,
@@ -1011,7 +1004,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virProcessKillPainfully(pid, true);
if (pidfile)
unlink(pidfile);
- qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel);
+ qemuSecurityRestoreTPMLabels(driver, vm, true);
return -1;
}
--
2.44.0
7:41 a.m.
New subject: [PATCH v3 5/5] qemu: Always set labels for TPM state
On Thu, May 02, 2024 at 19:39:42 +0200, Andrea Bolognani wrote:
Up until this point, we have avoided setting labels for
incoming migration when the TPM state is stored on a shared
filesystem. This seems to make sense, because since the
underlying storage is shared surely the labels will be as
well.
There's one problem, though: when a guest is migrated, the
SELinux context for the destination process is different from
the one of the source process.
We haven't hit any issues with the current approach so far
because NFS doesn't support SELinux, so effectively it doesn't
matter whether relabeling happens or not: even if the SELinux
contexts of the source and target processes are different,
both will be able to access the storage.
Now that it's possible for the local admin to manually mark
exported directories as shared filesystems, however, things
can get problematic.
Consider the case in which one host (mig-one) exports its
local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the
same time bind-mounts it to /var/lib/libvirt/swtpm; another
host (mig-two) mounts the same filesystem to the same
location, this time via NFS. Additionally, in order to
allow migration in both directions, on mig-one the
/var/lib/libvirt/swtpm directory is listed in the
shared_filesystems qemu.conf option.
When migrating from mig-one to mig-two, things work just fine;
going in the opposite direction, however, results in an error:
# virsh migrate cirros qemu+ssh://mig-one/system
error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'):
qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with a TPM
error 0x1f
qemu-system-x86_64: error while loading state for instance 0x0 of device
'tpm-emulator'
qemu-system-x86_64: load of migration failed: Input/output error
This is because the directory on mig-one is considered a
shared filesystem and thus labeling is skipped, resulting in
a SELinux denial.
The solution is quite simple: remove the check and always
relabel. We know that it's okay to do so not just because it
makes the error seen above go away, but also because no such
check currently exists for disks and other types of persistent
storage such as NVRAM files, which always get relabeled.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com>
---
src/qemu/qemu_tpm.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
176
days inactive
203
days old
20 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Peter Krempa