Our decision whether to remember seclabel for a disk image
depends on a few factors. If the image is readonly or shared or
not top parent of a backing chain the remembering is suppressed
for the image. However, the virSecurityManagerSetImageLabel() is
too low level to determine whether passed @src is top parent or
not. Even though it has domain definition available, in some
cases (like snapshots or block copy) the @src is added to the
definition only after the operation succeeded. Therefore,
introduce a flag which callers can use to help us with the
decision.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/security/security_dac.c | 16 +++++++++++-----
src/security/security_manager.h | 1 +
src/security/security_selinux.c | 18 ++++++++++++------
3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index f412054d0e..3f8b04b307 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -889,14 +889,14 @@ static int
virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
virDomainDefPtr def,
virStorageSourcePtr src,
- virStorageSourcePtr parent)
+ virStorageSourcePtr parent,
+ bool is_topparent)
{
virSecurityLabelDefPtr secdef;
virSecurityDeviceLabelDefPtr disk_seclabel;
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
bool remember;
- bool is_toplevel = parent == src || parent->externalDataStore == src;
uid_t user;
gid_t group;
@@ -954,7 +954,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
* but the top layer, or read only image, or disk explicitly
* marked as shared.
*/
- remember = is_toplevel && !src->readonly && !src->shared;
+ remember = is_topparent && !src->readonly && !src->shared;
return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);
}
@@ -967,10 +967,13 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
virStorageSourcePtr parent,
virSecurityDomainImageLabelFlags flags)
{
+ bool is_topparent = flags & VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
virStorageSourcePtr n;
+ flags &= ~VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
+
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
- if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
+ if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, is_topparent) < 0)
return -1;
if (n->externalDataStore &&
@@ -983,6 +986,8 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
break;
+
+ is_topparent = false;
}
return 0;
@@ -2114,7 +2119,8 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR)
continue;
if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src,
- VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0)
+ VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |
+ VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT) < 0)
return -1;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index b92ea5dc87..11904fda89 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -151,6 +151,7 @@ virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr);
typedef enum {
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0,
+ VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT = 1 << 1,
} virSecurityDomainImageLabelFlags;
int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 2241a35e6e..0aa0c2bb71 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1824,7 +1824,8 @@ static int
virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
virDomainDefPtr def,
virStorageSourcePtr src,
- virStorageSourcePtr parent)
+ virStorageSourcePtr parent,
+ bool is_topparent)
{
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1832,7 +1833,6 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
char *use_label = NULL;
bool remember;
- bool is_toplevel = parent == src || parent->externalDataStore == src;
g_autofree char *vfioGroupDev = NULL;
const char *path = src->path;
int ret;
@@ -1856,7 +1856,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
* but the top layer, or read only image, or disk explicitly
* marked as shared.
*/
- remember = is_toplevel && !src->readonly && !src->shared;
+ remember = is_topparent && !src->readonly && !src->shared;
disk_seclabel = virStorageSourceGetSecurityLabelDef(src,
SECURITY_SELINUX_NAME);
@@ -1873,7 +1873,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
return 0;
use_label = parent_seclabel->label;
- } else if (is_toplevel) {
+ } else if (parent == src || parent->externalDataStore == src) {
if (src->shared) {
use_label = data->file_context;
} else if (src->readonly) {
@@ -1927,10 +1927,13 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,
virStorageSourcePtr parent,
virSecurityDomainImageLabelFlags flags)
{
+ bool is_topparent = flags & VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
virStorageSourcePtr n;
+ flags &= ~VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
+
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
- if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent) < 0)
+ if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, is_topparent) < 0)
return -1;
if (n->externalDataStore &&
@@ -1943,6 +1946,8 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
break;
+
+ is_topparent = false;
}
return 0;
@@ -3146,7 +3151,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
continue;
}
if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src,
- VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0)
+ VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |
+ VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT) < 0)
return -1;
}
/* XXX fixme process def->fss if relabel == true */
--
2.24.1
On Thu, Feb 27, 2020 at 13:07:35 +0100, Michal Privoznik wrote:
> Our decision whether to remember seclabel for a disk image
> depends on a few factors. If the image is readonly or shared or
> not top parent of a backing chain the remembering is suppressed
Note that 'top parent' is a term used in the block commit code and the
top parent image there does not necessarily refer to the top of the disk
backing chain. As of such you should refrain from using that term and
use e.g. 'chain top'. Or perhaps better 'parentChainTop' or a variation.
The secdriver code takes the 'parent' argument which is the top level
image in the chain we want to relabel, but parent does not necessarily
refer to the topmost image and you actually want to know if 'parent' is
the topmost image.
> for the image. However, the virSecurityManagerSetImageLabel() is
> too low level to determine whether passed @src is top parent or
> not. Even though it has domain definition available, in some
> cases (like snapshots or block copy) the @src is added to the
> definition only after the operation succeeded. Therefore,
> introduce a flag which callers can use to help us with the
> decision.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/security/security_dac.c | 16 +++++++++++-----
> src/security/security_manager.h | 1 +
> src/security/security_selinux.c | 18 ++++++++++++------
> 3 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index f412054d0e..3f8b04b307 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -889,14 +889,14 @@ static int
> virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> virStorageSourcePtr src,
> - virStorageSourcePtr parent)
> + virStorageSourcePtr parent,
> + bool is_topparent)
> {
> virSecurityLabelDefPtr secdef;
> virSecurityDeviceLabelDefPtr disk_seclabel;
> virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
> virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> bool remember;
> - bool is_toplevel = parent == src || parent->externalDataStore == src;
> uid_t user;
> gid_t group;
>
> @@ -954,7 +954,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
> * but the top layer, or read only image, or disk explicitly
> * marked as shared.
> */
> - remember = is_toplevel && !src->readonly && !src->shared;
> + remember = is_topparent && !src->readonly && !src->shared;
>
> return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);
> }
> @@ -967,10 +967,13 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
> virStorageSourcePtr parent,
> virSecurityDomainImageLabelFlags flags)
> {
> + bool is_topparent = flags & VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
> virStorageSourcePtr n;
>
> + flags &= ~VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
Clearing this here ...
> +
> for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
> - if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
> + if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, is_topparent) < 0)
> return -1;
>
> if (n->externalDataStore &&
... would skip the remembering for the top level image's external data
store file imagelabel. Since the data store is equivalent to the top
level image in terms of labelling we should restore the label there as
well.
> @@ -983,6 +986,8 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
[...]
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index b92ea5dc87..11904fda89 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -151,6 +151,7 @@ virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr);
>
> typedef enum {
> VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0,
> + VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT = 1 << 1,
Please add a comment stating what that flag means in addition to the
name change.
> } virSecurityDomainImageLabelFlags;
>
> int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 2241a35e6e..0aa0c2bb71 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
[...]
> @@ -1873,7 +1873,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
> return 0;
>
> use_label = parent_seclabel->label;
> - } else if (is_toplevel) {
> + } else if (parent == src || parent->externalDataStore == src) {
> if (src->shared) {
> use_label = data->file_context;
> } else if (src->readonly) {
> @@ -1927,10 +1927,13 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,
> virStorageSourcePtr parent,
> virSecurityDomainImageLabelFlags flags)
> {
> + bool is_topparent = flags & VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
> virStorageSourcePtr n;
>
> + flags &= ~VIR_SECURITY_DOMAIN_IMAGE_TOP_PARENT;
> +
> for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
> - if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent) < 0)
> + if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, is_topparent) < 0)
> return -1;
>
> if (n->externalDataStore &&
Same as in the DAC driver.
> @@ -1943,6 +1946,8 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,
>
> if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
> break;
> +
> + is_topparent = false;
> }
>
> return 0;
© 2016 - 2026 Red Hat, Inc.