Re: [PATCH 2/2] Fix seclabels for chardevs

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

 



In subject:

s/Fix/conf: Fix/

On 05/16/14 15:23, Ján Tomko wrote:
> We allow a seclabel to be specified in the <source> element
> of a chardev:
> 
> <serial type='file'>
>   <source path='/tmp/serial.file'>
>     <seclabel model='dac' relabel='no'/>
>   </source>
> </serial>

There is one paragraph mentioning that in the XML format documentation.
I think it would be worth adding (as a separate patch) an example of the
usage too.

> 
> But we format it outside the source:
> 
> <serial type='file'>
>   <source path='/tmp/serial.file'/>
>   <target port='0'/>
>     <seclabel model='dac' relabel='no'/>
> </serial>
> 
> Move the formatting inside the source to fix this to make the
> seclabel persistent across XML format->parse.
> 
> Introduced by commit f8b08d0 'Add <seclabel> to character devices.'
> ---
>  src/conf/domain_conf.c                             | 27 +++++++--------
>  .../qemuxml2argv-chardev-label.xml                 | 40 ++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  2 ++
>  3 files changed, 54 insertions(+), 15 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 041a113..81e9436 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15862,6 +15862,7 @@ virDomainNetDefFormat(virBufferPtr buf,
>   * output at " type='type'>". */
>  static int
>  virDomainChrSourceDefFormat(virBufferPtr buf,
> +                            virDomainChrDefPtr chr_def,
>                              virDomainChrSourceDefPtr def,
>                              bool tty_compat,
>                              unsigned int flags)
> @@ -15898,8 +15899,11 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          if (def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
>              (def->data.file.path &&
>               !(flags & VIR_DOMAIN_XML_INACTIVE))) {
> -            virBufferEscapeString(buf, "<source path='%s'/>\n",
> +            virBufferEscapeString(buf, "<source path='%s'",
>                                    def->data.file.path);
> +            virDomainSourceDefFormatSeclabel(buf, chr_def->nseclabels,
> +                                             chr_def->seclabels,
> +                                             flags);
>          }
>          break;
>  

I think that "case VIR_DOMAIN_CHR_TYPE_UNIX:" should be probably handled
too.


> @@ -15957,7 +15961,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          virBufferAsprintf(buf, "<source mode='%s'",
>                            def->data.nix.listen ? "bind" : "connect");
>          virBufferEscapeString(buf, " path='%s'", def->data.nix.path);
> -        virBufferAddLit(buf, "/>\n");
> +        virDomainSourceDefFormatSeclabel(buf, chr_def->nseclabels,
> +                                         chr_def->seclabels,
> +                                         flags);
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> @@ -15979,7 +15985,6 @@ virDomainChrDefFormat(virBufferPtr buf,
>      const char *targetType = virDomainChrTargetTypeToString(def->deviceType,
>                                                              def->targetType);
>      bool tty_compat;
> -    size_t n;
>  
>      int ret = 0;
>  
> @@ -15997,7 +16002,7 @@ virDomainChrDefFormat(virBufferPtr buf,
>                    def->source.type == VIR_DOMAIN_CHR_TYPE_PTY &&
>                    !(flags & VIR_DOMAIN_XML_INACTIVE) &&
>                    def->source.data.file.path);
> -    if (virDomainChrSourceDefFormat(buf, &def->source, tty_compat, flags) < 0)
> +    if (virDomainChrSourceDefFormat(buf, def, &def->source, tty_compat, flags) < 0)
>          return -1;
>  
>      /* Format <target> block */
> @@ -16069,14 +16074,6 @@ virDomainChrDefFormat(virBufferPtr buf,
>              return -1;
>      }
>  
> -    /* Security label overrides, if any. */
> -    if (def->seclabels && def->nseclabels > 0) {
> -        virBufferAdjustIndent(buf, 2);
> -        for (n = 0; n < def->nseclabels; n++)
> -            virSecurityDeviceLabelDefFormat(buf, def->seclabels[n], flags);
> -        virBufferAdjustIndent(buf, -2);
> -    }
> -
>      virBufferAdjustIndent(buf, -2);
>      virBufferAsprintf(buf, "</%s>\n", elementName);
>  
> @@ -16119,7 +16116,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
>          break;
>  
>      case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> -        if (virDomainChrSourceDefFormat(buf, &def->data.passthru, false,
> +        if (virDomainChrSourceDefFormat(buf, NULL, &def->data.passthru, false,
>                                          flags) < 0)

Passing NULL as chr_def to virDomainChrSourceDefFormat will induce a
crash once you will try to format a RNG, smartcard or other device with
a chardev backend with type PIPE or other.

>              return -1;
>          break;
> @@ -16384,7 +16381,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
>  
>      case VIR_DOMAIN_RNG_BACKEND_EGD:
>          virBufferAdjustIndent(buf, 2);
> -        if (virDomainChrSourceDefFormat(buf, def->source.chardev,
> +        if (virDomainChrSourceDefFormat(buf, NULL, def->source.chardev,
>                                          false, flags) < 0)
>              return -1;
>          virBufferAdjustIndent(buf, -2);
> @@ -16976,7 +16973,7 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
>  
>      virBufferAsprintf(buf, "<redirdev bus='%s'", bus);
>      virBufferAdjustIndent(buf, 2);
> -    if (virDomainChrSourceDefFormat(buf, &def->source.chr, false, flags) < 0)
> +    if (virDomainChrSourceDefFormat(buf, NULL, &def->source.chr, false, flags) < 0)
>          return -1;
>      if (virDomainDeviceInfoFormat(buf, &def->info,
>                                    flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT) < 0)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml b/tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
> new file mode 100644
> index 0000000..b6df67a
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-chardev-label.xml
> @@ -0,0 +1,40 @@
> +<domain type='qemu'>
> +  <name>machine</name>
> +  <uuid>2187c512-ff97-47d7-b67c-c02d3bdc219d</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <currentMemory unit='KiB'>219100</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <serial type='file'>
> +      <source path='/tmp/serial.file'>
> +        <seclabel model='dac' relabel='no'/>
> +      </source>
> +      <target port='0'/>
> +    </serial>
> +    <serial type='unix'>
> +      <source mode='connect' path='/tmp/serial.sock'>
> +        <seclabel model='dac' relabel='no'/>
> +      </source>
> +      <target port='1'/>
> +    </serial>
> +    <console type='file'>
> +      <source path='/tmp/serial.file'>
> +        <seclabel model='dac' relabel='no'/>
> +      </source>
> +      <target type='serial' port='0'/>
> +    </console>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>

Nice test, but add a device:

<rng model='virtio'>
  <backend model='egd' type='pipe'>
    <source path='/dev/null'/>
  </backend>
</rng>

This will lead with the code in this patch to the crash described above.

> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 3ea03e6..da528da 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -362,6 +362,8 @@ mymain(void)
>  
>      DO_TEST_DIFFERENT("disk-backing-chains");
>  
> +    DO_TEST("chardev-label");
> +
>      virObjectUnref(driver.caps);
>      virObjectUnref(driver.xmlopt);
>  
> 

Peter

Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]