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

[Hong Xiang]

Hello,

I reproduced the symptoms (no output from designated pipe, guest hang 
after a while) with commit 05e2fc51, and with commit c7d1f598 they are gone.
I also noticed libvirt needs to be built with --with-qemu-user=<user> 
--with-qemu-group=<group> to reproduce the symptoms, with older code, or 
the qemu process will be running as root and can write to un-chown'ed pipe.

On 09/28/2011 03:01 PM, Daniel Veillard wrote:
> 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
>

-- 
Thanks.
Hong Xiang