Re: [PATCH] security: Ensure kernel/initrd exist before restoring label

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(a)suse.com&gt; > > --- > > 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_se... 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);