[PATCH] security: Ensure kernel/initrd exist before restoring label
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ffad058d9a..b21986cb7e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,11 +2915,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1; -- 2.44.0
On 3/8/24 16:26, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
I've already s/harmles/harmless in my local branch.
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
BTW, I'm happy to explore other suggestions for squelching these errors.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ffad058d9a..b21986cb7e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,11 +2915,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1;
If this is acceptable, I can squash in a comment such as below that explains the logic. Regards, Jim diff --git a/src/security/security_dac.c b/src/security/security_dac.c index be606c6f33..3e3d7c00b6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,6 +1993,10 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } + /* In scenarios like VM installation, tooling such as virt-install may + * have already removed the install kernel/initrd. Ensure they still + * exist before relabeling. + */ if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b21986cb7e..495d5aa01c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,6 +2915,10 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; } + /* In scenarios like VM installation, tooling such as virt-install may + * have already removed the install kernel/initrd. Ensure they still + * exist before relabeling. + */ if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1;
On 3/9/24 01:06, Jim Fehlig wrote:
On 3/8/24 16:26, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
I've already s/harmles/harmless in my local branch.
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
BTW, I'm happy to explore other suggestions for squelching these errors.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ffad058d9a..b21986cb7e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,11 +2915,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1;
If this is acceptable, I can squash in a comment such as below that explains the logic.
Regards, Jim
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index be606c6f33..3e3d7c00b6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,6 +1993,10 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
+ /* In scenarios like VM installation, tooling such as virt-install may + * have already removed the install kernel/initrd. Ensure they still + * exist before relabeling. + */ if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b21986cb7e..495d5aa01c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,6 +2915,10 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
+ /* In scenarios like VM installation, tooling such as virt-install may + * have already removed the install kernel/initrd. Ensure they still + * exist before relabeling. + */ if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1;
Yes, please add these comments. Unfortunately, I don't have a better solution. Maybe, this virFileExist() check can be moved to virSecuritySELinuxRestoreFileLabel() and virSecurityDACRestoreFileLabel()? That would solve this problem for other files too. One thing to be aware though - virSecurityDACRestoreFileLabelInternal() might work with a virStorageSource* and is specifically designed to not call 'chown' but a chownCallback() in same cases (I guess it has to do with RBD? ceph? something like that). But I guess we can so something similar to what's already present there: if (path && !virFileExist(path) return 0; But if you want to go with your version, that's okay too. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Hi All! Any comments beyond my own on this patch? :-) As said in the self reply, I'm happy to explore other suggestions for squelching the alarming, yet harmless error messages. Regards, Jim On 3/8/24 16:26, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ffad058d9a..b21986cb7e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,11 +2915,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1;
On Fri, Mar 08, 2024 at 04:26:27PM -0700, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
I wonder if this scenario is conceptually relevant to other files though. eg someone created a qcow2 overlay for the disk, to capture writes, and then immediatley unlinked it as they wanted to discard them. ie manual equivalent of QEMU's -snapshot arg. Should we instead plumb something in so that the 'stat()' failure gets silently ignored when it is ENOENT, on a "restore" code path 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 :|
On 3/21/24 08:57, Daniel P. Berrangé wrote:
On Fri, Mar 08, 2024 at 04:26:27PM -0700, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; }
- if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1;
- if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
I wonder if this scenario is conceptually relevant to other files though.
eg someone created a qcow2 overlay for the disk, to capture writes, and then immediatley unlinked it as they wanted to discard them. ie manual equivalent of QEMU's -snapshot arg.
Should we instead plumb something in so that the 'stat()' failure gets silently ignored when it is ENOENT, on a "restore" code path
Something like the following works for the DAC driver diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..28afa4846b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -667,7 +667,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid) + gid_t gid, + bool ignoreNoEnt) { int rc = 0; @@ -689,6 +690,9 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, return 0; if (stat(path, &sb) < 0) { + if (errno == ENOENT && ignoreNoEnt) + return 0; + virReportSystemError(errno, _("unable to stat: %1$s"), path); return -1; } @@ -787,7 +791,7 @@ virSecurityDACSetOwnership(virSecurityManager *mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, false) < 0) goto error; return 0; @@ -847,7 +851,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, true); } The selinux driver is not as simple. I suspect the call to virFileResolveLink() would fail if the file no longer exists, well before the call to stat() https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selin... Adding an 'ignoreNoEnt' parameter to virSecuritySELinuxRestoreFileLabel, plumbing it through to virFileResolveLink, and adjusting all call sites seems a bit overkill. An FYI: while testing the above patch, I thought something simple like the below hack was a clever fix, but it causes several qemusecuritytest failures. Regards, Jim diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..4e850e219e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, virStorageSourceIsLocalStorage(src)) path = src->path; + if (!virFileExists(path)) + return 0; + /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b49af26e49..aaec34ff8b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, */ if (!path) return 0; + if (!virFileExists(path)) + return 0; VIR_INFO("Restoring SELinux context on '%s'", path);
On Mon, Mar 25, 2024 at 07:13:05PM -0600, Jim Fehlig wrote:
On 3/21/24 08:57, Daniel P. Berrangé wrote:
On Fri, Mar 08, 2024 at 04:26:27PM -0700, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
I wonder if this scenario is conceptually relevant to other files though.
eg someone created a qcow2 overlay for the disk, to capture writes, and then immediatley unlinked it as they wanted to discard them. ie manual equivalent of QEMU's -snapshot arg.
Should we instead plumb something in so that the 'stat()' failure gets silently ignored when it is ENOENT, on a "restore" code path
Something like the following works for the DAC driver
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..28afa4846b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -667,7 +667,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid) + gid_t gid, + bool ignoreNoEnt) { int rc = 0;
@@ -689,6 +690,9 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, return 0;
if (stat(path, &sb) < 0) { + if (errno == ENOENT && ignoreNoEnt) + return 0; + virReportSystemError(errno, _("unable to stat: %1$s"), path); return -1; } @@ -787,7 +791,7 @@ virSecurityDACSetOwnership(virSecurityManager *mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, false) < 0) goto error;
return 0; @@ -847,7 +851,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, true); }
The selinux driver is not as simple. I suspect the call to virFileResolveLink() would fail if the file no longer exists, well before the call to stat()
https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selin...
Adding an 'ignoreNoEnt' parameter to virSecuritySELinuxRestoreFileLabel, plumbing it through to virFileResolveLink, and adjusting all call sites seems a bit overkill.
An FYI: while testing the above patch, I thought something simple like the below hack was a clever fix, but it causes several qemusecuritytest failures.
This simple patch is not unreasonable. Might just be that test that has bad assumptions that need fixing ?
Regards, Jim
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..4e850e219e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, virStorageSourceIsLocalStorage(src)) path = src->path;
+ if (!virFileExists(path)) + return 0; + /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b49af26e49..aaec34ff8b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, */ if (!path) return 0; + if (!virFileExists(path)) + return 0;
VIR_INFO("Restoring SELinux context on '%s'", path);
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 :|
On 3/27/24 08:01, Daniel P. Berrangé wrote:
On Mon, Mar 25, 2024 at 07:13:05PM -0600, Jim Fehlig wrote:
On 3/21/24 08:57, Daniel P. Berrangé wrote:
On Fri, Mar 08, 2024 at 04:26:27PM -0700, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
I wonder if this scenario is conceptually relevant to other files though.
eg someone created a qcow2 overlay for the disk, to capture writes, and then immediatley unlinked it as they wanted to discard them. ie manual equivalent of QEMU's -snapshot arg.
Should we instead plumb something in so that the 'stat()' failure gets silently ignored when it is ENOENT, on a "restore" code path
Something like the following works for the DAC driver
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..28afa4846b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -667,7 +667,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid) + gid_t gid, + bool ignoreNoEnt) { int rc = 0;
@@ -689,6 +690,9 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, return 0;
if (stat(path, &sb) < 0) { + if (errno == ENOENT && ignoreNoEnt) + return 0; + virReportSystemError(errno, _("unable to stat: %1$s"), path); return -1; } @@ -787,7 +791,7 @@ virSecurityDACSetOwnership(virSecurityManager *mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, false) < 0) goto error;
return 0; @@ -847,7 +851,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, true); }
The selinux driver is not as simple. I suspect the call to virFileResolveLink() would fail if the file no longer exists, well before the call to stat()
https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selin...
Adding an 'ignoreNoEnt' parameter to virSecuritySELinuxRestoreFileLabel, plumbing it through to virFileResolveLink, and adjusting all call sites seems a bit overkill.
An FYI: while testing the above patch, I thought something simple like the below hack was a clever fix, but it causes several qemusecuritytest failures.
This simple patch is not unreasonable. Might just be that test that has bad assumptions that need fixing ?
Thanks for confirming the sanity of the change. With that, I went about figuring out how the test works and will write it here for future reference. qemusecuritytest instantiates virDomainObj based on config in tests/qemuxmlconfdata/*.xml. It then calls qemuSecuritySetAllLabel to label everything, followed by qemuSecurityRestoreAllLabel to restore the labels. xattr, chown, and selinux {get,set}filecon_raw are mocked in qemusecuritymock.c, where a VIR_MOCK_STAT_HOOK is also defined. Using the DAC driver as an example, when setting labels virSecurityDACSetOwnershipInternal calls stat on a path, where the mock hook adds the path to a hashtable with associated UID:GID set to DEFAULT_{UID,GID}. virSecurityDACSetOwnershipInternal() later calls chown on the path to set UID:GID. The mocked chown() updates the new values in the hashtable. On restore, the values get changed back to DEFAULT_{UID,GID}. Finally, the checkPaths() function in qemusecuritymock.c verifies everything was restored to DEFAULT_{UID,GID}. Assuming I've understood the test and described it correctly, it's clear why adding the virFileExists() in the restore path of the drivers results in the test failure. None of the files actually exist, so the restore was never executed. I've sent a V2 of the patch, which mocks virFileExists to return true when encountering a file previously seen by the mocked stat, chown, and {get,set}filecon_raw functions https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MSB53... Regards, Jim
participants (3)
-
Daniel P. Berrangé -
Jim Fehlig -
Michal Prívozník