[libvirt] [PATCHv2] security: properly chown/label bidirectional and unidirectional fifos
Hong Xiang
hxiang at linux.vnet.ibm.com
Thu Sep 29 03:59:01 UTC 2011
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
More information about the libvir-list
mailing list