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

On 4/8/24 6:18 AM, Michal Prívozník wrote: > 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: I considered calling the real virFileExists, but was being lazy since the impl is a one-liner :-). Agree it's better, for consistency if nothing else. I've squashed in your suggestion and pushed the patch.