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

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(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. This simple patch is not unreasonable. Might just be that test that has bad assumptions that need fixing ?