Re: [PATCH V2] security: Ensure file exists before attempting to restore label

On 4/2/24 00:14, 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 > > Add a check for file existence to the virSecurity*RestoreFileLabel functions, > and avoid relabeling if the file is no longer available. Skipping the restore > caused failures in qemusecuritytest, which mocks stat, chown, etc as part of > ensuring the security drivers properly restore labels. virFileExists is now > mocked in qemusecuritymock.c to return true when passed a file previously > seen by the mocked stat, chown, etc functions. > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > src/security/security_dac.c | 3 +++ > src/security/security_selinux.c | 2 ++ > tests/qemusecuritymock.c | 18 ++++++++++++++++++ > 3 files changed, 23 insertions(+) > Looks better now, thanks! > 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); > > diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c > index 80d59536b1..bb0a85544b 100644 > --- a/tests/qemusecuritymock.c > +++ b/tests/qemusecuritymock.c > @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED, > } > > > +bool virFileExists(const char *path G_GNUC_UNUSED) This argument is used, so please drop the unused annotation. > +{ > + VIR_LOCK_GUARD lock = virLockGuardLock(&m); > + > + if (getenv(ENVVAR) == NULL) > + return access(path, F_OK) == 0; I wonder whether we should call real virFileExists() here. That way, if a test would chain mocks they can work (though both would probably be mocking access()). I'll leave it up to you whether you want to squash in the following: