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 <[email protected]>
> > > ---
> > > 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_selinux.c?ref_type=heads#L1494
>
> 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 :|
_______________________________________________
Devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
