Re: [PATCH 3/3] security: Don't remember labels for TPM

[Cole Robinson <crobinso@xxxxxxxxxx>]

On 10/11/19 11:08 AM, Michal Privoznik wrote:
> On 10/10/19 10:15 PM, Cole Robinson wrote:
> > On 10/1/19 11:00 AM, Michal Privoznik wrote:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1755803
> > >
> > > The /dev/tpmN file can be opened only once, as implemented in
> > > drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
> > > other attempt to open the file fails. And since we're opening the
> > > file ourselves and passing the FD to qemu we will not succeed
> > > opening the file again when locking it for seclabel remembering.
> > >
> > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > ---
> > >   src/security/security_dac.c     | 18 +++++++++---------
> > >   src/security/security_selinux.c | 10 +++++-----
> > >   2 files changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> > > index 2733fa664f..347a7a5f63 100644
> > > --- a/src/security/security_dac.c
> > > +++ b/src/security/security_dac.c
> > > @@ -1653,14 +1653,14 @@ 
> > > virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
> > >       switch (tpm->type) {
> > >       case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> > > -        ret = virSecurityDACSetChardevLabel(mgr, def,
> > > - &tpm->data.passthrough.source,
> > > -                                            false);
> > > +        ret = virSecurityDACSetChardevLabelHelper(mgr, def,
> > > + &tpm->data.passthrough.source,
> > > +                                                  false, false);
> > >           break;
> > >       case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> > > -        ret = virSecurityDACSetChardevLabel(mgr, def,
> > > -                                            &tpm->data.emulator.source,
> > > -                                            false);
> > > +        ret = virSecurityDACSetChardevLabelHelper(mgr, def,
> > > + &tpm->data.emulator.source,
> > > +                                                  false, false);
> > >           break;
> > >       case VIR_DOMAIN_TPM_TYPE_LAST:
> > >           break;
> > > @@ -1679,9 +1679,9 @@ 
> > > virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,
> > >       switch (tpm->type) {
> > >       case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> > > -        ret = virSecurityDACRestoreChardevLabel(mgr, def,
> > > - &tpm->data.passthrough.source,
> > > -                                                false);
> > > +        ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
> > > + &tpm->data.passthrough.source,
> > > +                                                      false, false);
> > >           break;
> > >       case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> > >           /* swtpm will have removed the Unix socket upon termination */
> > > diff --git a/src/security/security_selinux.c 
> > > b/src/security/security_selinux.c
> > > index e3be724a2b..0486bdd6b6 100644
> > > --- a/src/security/security_selinux.c
> > > +++ b/src/security/security_selinux.c
> > > @@ -1682,14 +1682,14 @@ 
> > > virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,
> > >       switch (tpm->type) {
> > >       case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
> > >           tpmdev = tpm->data.passthrough.source.data.file.path;
> > > -        rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
> > > seclabel->imagelabel, true);
> > > +        rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
> > > seclabel->imagelabel, false);
> > >           if (rc < 0)
> > >               return -1;
> > >           if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
> > >               rc = virSecuritySELinuxSetFilecon(mgr,
> > >                                                 cancel_path,
> > > -                                              seclabel->imagelabel, 
> > > true);
> > > +                                              seclabel->imagelabel, 
> > > false);
> > >               VIR_FREE(cancel_path);
> > >               if (rc < 0) {
> > >                   virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, 
> > > tpm);
> > > @@ -1701,7 +1701,7 @@ 
> > > virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,
> > >           break;
> > >       case VIR_DOMAIN_TPM_TYPE_EMULATOR:
> > >           tpmdev = tpm->data.emulator.source.data.nix.path;
> > > -        rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
> > > seclabel->imagelabel, true);
> > > +        rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
> > > seclabel->imagelabel, false);
> > >           if (rc < 0)
> > >               return -1;
> > >           break;
> >
> > Are the EMULATOR changes actually required? I think it just uses a 
> > unix socket and doesn't touch the host kernel tpm subsystem
>
> Well, not per se, because the socket is created on domain startup and 
> then unlink()-ed on domain shutdown. That's why it doesn't make sense to 
> try remember anything about the socket.

Okay so it's basically a separate issue from the patch series. In that 
case use of virSecurityDACRestoreChardevLabelHelper for EMULATOR is a 
bit misleading, and instead virSecurityDACRestoreChardevLabel should 
probably grow some smarts to set remember=false for any unix socket path 
it sees. But it's a minor distinction because I don't think it matters 
in practice given that sockets are unlinked like you say

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list