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

On 3/8/24 16:26, 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 I've already s/harmles/harmless in my local branch. > > 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. BTW, I'm happy to explore other suggestions for squelching these errors. > > 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; >   diff --git a/src/security/security_selinux.c > b/src/security/security_selinux.c > index ffad058d9a..b21986cb7e 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -2915,11 +2915,11 @@ > virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, >               rc = -1; >       } >   -    if (def->os.kernel && > +    if (def->os.kernel && virFileExists(def->os.kernel) && >           virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, > true) < 0) >           rc = -1; >   -    if (def->os.initrd && > +    if (def->os.initrd && virFileExists(def->os.initrd) && >           virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, > true) < 0) >           rc = -1; >   If this is acceptable, I can squash in a comment such as below that explains the logic. Regards, Jim diff --git a/src/security/security_dac.c b/src/security/security_dac.c index be606c6f33..3e3d7c00b6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,6 +1993,10 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,              rc = -1;      } +    /* In scenarios like VM installation, tooling such as virt-install may +     * have already removed the install kernel/initrd. Ensure they still +     * exist before relabeling. +     */      if (def->os.kernel && virFileExists(def->os.kernel) &&          virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0)          rc = -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b21986cb7e..495d5aa01c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2915,6 +2915,10 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr,              rc = -1;      } +    /* In scenarios like VM installation, tooling such as virt-install may +     * have already removed the install kernel/initrd. Ensure they still +     * exist before relabeling. +     */      if (def->os.kernel && virFileExists(def->os.kernel) &&          virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0)          rc = -1;