Re: [libvirt] [PATCHv2] security: properly chown/label bidirectional and unidirectional fifos

On Tue, Sep 27, 2011 at 09:00:15PM -0400, Laine Stump wrote: > (v2: change to only relabel the uni-direction fifo pair if they're > found, otherwise only relabel the bidirectional fifo, per comment in BZ > by Dan Berrange) > > This patch fixes the regression with using named pipes for qemu serial > devices noted in: > > https://bugzilla.redhat.com/show_bug.cgi?id=740478 > > The problem was that, while new code in libvirt looks for a single > bidirectional fifo of the name given in the config, then relabels that > and continues without looking for / relabelling the two unidirectional > fifos named ${name}.in and ${name}.out, qemu looks in the opposite > order. So if the user had naively created all three fifos, libvirt > would relabel the bidirectional fifo to allow qemu access, but qemu > would attempt to use the two unidirectional fifos and fail (because it > didn't have proper permissions/rights). > > This patch changes the order that libvirt looks for the fifos to match > what qemu does - first it looks for the dual fifos, then it looks for > the single bidirectional fifo. If it finds the dual unidirectional > fifos first, it labels/chowns them and ignores any possible > bidirectional fifo. > > (Note commit d37c6a3a (which first appeared in libvirt-0.9.2) added > the code that checked for a bidirectional fifo. Prior to that commit, > bidirectional fifos fdor serial devices didn't work unless the fifo > was pre-owned/labelled such that qemu could access it.) > --- > src/security/security_dac.c | 30 ++++++++++++++++++------------ > src/security/security_selinux.c | 29 +++++++++++++++++------------ > 2 files changed, 35 insertions(+), 24 deletions(-) > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index af02236..ac79e70 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -406,18 +406,19 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, > break; > > case VIR_DOMAIN_CHR_TYPE_PIPE: > - if (virFileExists(dev->data.file.path)) { > - if (virSecurityDACSetOwnership(dev->data.file.path, priv->user, priv->group)< 0) > - goto done; > - } else { > - if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || > - (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { > - virReportOOMError(); > - goto done; > - } > + if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || > + (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { > + virReportOOMError(); > + goto done; > + } > + if (virFileExists(in) || virFileExists(out)) { Actually the qemu code of qemu_chr_open_pipe() does if (fd_in< 0 || fd_out< 0) { close either if they opened and try to open the main file so the logic in libvirt should be if (virFileExists(in)&& virFileExists(out)) { > if ((virSecurityDACSetOwnership(in, priv->user, priv->group)< 0) || > - (virSecurityDACSetOwnership(out, priv->user, priv->group)< 0)) > + (virSecurityDACSetOwnership(out, priv->user, priv->group)< 0)) { > goto done; > + } > + } else if (virSecurityDACSetOwnership(dev->data.file.path, > + priv->user, priv->group)< 0) { > + goto done; > } > ret = 0; > break; > @@ -452,9 +453,14 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, > virReportOOMError(); > goto done; > } > - if ((virSecurityDACRestoreSecurityFileLabel(out)< 0) || > - (virSecurityDACRestoreSecurityFileLabel(in)< 0)) > + if (virFileExists(in) || virFileExists(out)) { Same > + if ((virSecurityDACRestoreSecurityFileLabel(out)< 0) || > + (virSecurityDACRestoreSecurityFileLabel(in)< 0)) { > goto done; > + } > + } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path)< 0) { > + goto done; > + } > ret = 0; > break; > > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c > index 0807a34..9e5b668 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -806,18 +806,18 @@ SELinuxSetSecurityChardevLabel(virDomainObjPtr vm, > break; > > case VIR_DOMAIN_CHR_TYPE_PIPE: > - if (virFileExists(dev->data.file.path)) { > - if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel)< 0) > - goto done; > - } else { > - if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || > - (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { > - virReportOOMError(); > - goto done; > - } > + if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || > + (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { > + virReportOOMError(); > + goto done; > + } > + if (virFileExists(in) || virFileExists(out)) { Same > if ((SELinuxSetFilecon(in, secdef->imagelabel)< 0) || > - (SELinuxSetFilecon(out, secdef->imagelabel)< 0)) > + (SELinuxSetFilecon(out, secdef->imagelabel)< 0)) { > goto done; > + } > + } else if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel)< 0) { > + goto done; > } > ret = 0; > break; > @@ -858,9 +858,14 @@ SELinuxRestoreSecurityChardevLabel(virDomainObjPtr vm, > virReportOOMError(); > goto done; > } > - if ((SELinuxRestoreSecurityFileLabel(out)< 0) || > - (SELinuxRestoreSecurityFileLabel(in)< 0)) > + if (virFileExists(in) || virFileExists(out)) { And same, > + if ((SELinuxRestoreSecurityFileLabel(out)< 0) || > + (SELinuxRestoreSecurityFileLabel(in)< 0)) { > + goto done; > + } > + } else if (SELinuxRestoreSecurityFileLabel(dev->data.file.path)< 0) { > goto done; > + } > ret = 0; > break; > That bug got me a bit crazy, I would like to get it fixed :) ACK conditionned on the above four || replaced by&& Daniel