[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]