From: Simon Gaiser <simon@invisiblethingslab.com>

By setting the permissive flag the guest access to the PCI config space
is not filtered. This might be a security risk, but it's required for
some devices and the IOMMU and interrupt remapping should (mostly?)
contain it.

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 docs/formatdomain.html.in     |  3 +++
 docs/schemas/domaincommon.rng |  5 +++++
 src/conf/domain_conf.c        | 12 ++++++++++++
 src/conf/domain_conf.h        |  1 +
 src/libxl/libxl_conf.c        |  1 +
 5 files changed, 22 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6f43976815..79a5176ccd 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5051,6 +5051,9 @@
             or hot-plugging the device and <code>virNodeDeviceReAttach</code>
             (or <code>virsh nodedev-reattach</code>) after hot-unplug or
             stopping the guest.
+            When <code>permissive</code> is "yes" the pci config space access
+            will not be filtered. This might be a security issue. The default
+            is "no".
           </dd>
           <dt><code>scsi</code></dt>
           <dd>For SCSI devices, user is responsible to make sure the device
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 65d6580434..9389eec3d8 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3064,6 +3064,11 @@
               <ref name="virYesNo"/>
             </attribute>
           </optional>
+          <optional>
+            <attribute name="permissive">
+              <ref name="virYesNo"/>
+            </attribute>
+          </optional>
           <interleave>
             <element name="source">
               <optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8e8146374c..607cae61d4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8419,6 +8419,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
     virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host;
     virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev;
     g_autofree char *managed = NULL;
+    g_autofree char *permissive = NULL;
     g_autofree char *sgio = NULL;
     g_autofree char *rawio = NULL;
     g_autofree char *backendStr = NULL;
@@ -8434,6 +8435,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
     if ((managed = virXMLPropString(node, "managed")) != NULL)
         ignore_value(virStringParseYesNo(managed, &def->managed));
 
+    if ((permissive = virXMLPropString(node, "permissive")) != NULL) {
+        if (STREQ(permissive, "yes"))
+            def->permissive = true;
+    }
+
     sgio = virXMLPropString(node, "sgio");
     rawio = virXMLPropString(node, "rawio");
     model = virXMLPropString(node, "model");
@@ -25942,6 +25948,8 @@ virDomainActualNetDefFormat(virBufferPtr buf,
         virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def);
         if  (hostdef && hostdef->managed)
             virBufferAddLit(buf, " managed='yes'");
+        if  (hostdef && hostdef->permissive)
+            virBufferAddLit(buf, " permissive='yes'");
     }
     if (def->trustGuestRxFilters)
         virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
@@ -26130,6 +26138,8 @@ virDomainNetDefFormat(virBufferPtr buf,
     virBufferAsprintf(buf, "<interface type='%s'", typeStr);
     if (hostdef && hostdef->managed)
         virBufferAddLit(buf, " managed='yes'");
+    if (hostdef && hostdef->permissive)
+        virBufferAddLit(buf, " permissive='yes'");
     if (def->trustGuestRxFilters)
         virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
                           virTristateBoolTypeToString(def->trustGuestRxFilters));
@@ -27914,6 +27924,8 @@ virDomainHostdevDefFormat(virBufferPtr buf,
     if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
         virBufferAsprintf(buf, " managed='%s'",
                           def->managed ? "yes" : "no");
+        if (def->permissive)
+            virBufferAddLit(buf, " permissive='yes'");
 
         if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
             scsisrc->sgio)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aad3f82db7..b81a3ce901 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -345,6 +345,7 @@ struct _virDomainHostdevDef {
     bool missing;
     bool readonly;
     bool shareable;
+    bool permissive;
     union {
         virDomainHostdevSubsys subsys;
         virDomainHostdevCaps caps;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b3f67f817a..55f2a09e3e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2249,6 +2249,7 @@ libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev)
     pcidev->bus = pcisrc->addr.bus;
     pcidev->dev = pcisrc->addr.slot;
     pcidev->func = pcisrc->addr.function;
+    pcidev->permissive = hostdev->permissive;
 
     return 0;
 }
-- 
2.21.1

On 4/17/20 12:33 PM, Marek Marczykowski-Górecki wrote:
> From: Simon Gaiser <simon@invisiblethingslab.com>
>
> By setting the permissive flag the guest access to the PCI config space
> is not filtered. This might be a security risk, but it's required for
> some devices and the IOMMU and interrupt remapping should (mostly?)
> contain it.
>
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>   docs/formatdomain.html.in     |  3 +++
>   docs/schemas/domaincommon.rng |  5 +++++
>   src/conf/domain_conf.c        | 12 ++++++++++++
>   src/conf/domain_conf.h        |  1 +
>   src/libxl/libxl_conf.c        |  1 +
>   5 files changed, 22 insertions(+)


I won't pretend to know anything about PCI device passthrough in Xen 
:-), so I'll just comment about the contruction of the patch rather than 
what it does (which is really just a single line in libxl_conf.c).


1) A patch that adds a new attribute to the XML should have an example 
of the use of the attribute in the commit message and in the docs. 
You've added a comment to the docs (formatdomain.html) naming the option 
an in general what it does, but not an example (which forced me to 
actually read the code! :-P)


2) The comment in the docs needs to document that it is supported "since 
6.3.0 (Xen only)". The fact that it is Xen-only should also be noted in 
the commit message (I, with my QEMU-goggles, was confused for a bit when 
I thought that you were adding support for some new qemu commandline 
knob, but had forgotten to add the bit that actually tweaked the knob). 
Usually we will start the subject line of a qemu-specific patch with 
"qemu: bobloblaw", so you could start this patch with "libxl: Add ...", 
or maybe "libxl/conf: Add" if you wanted to be pedantic about it.

3) Since this attribute is supported only for Xen, the postparse 
validation functions for other hypervisors need to check if it is set, 
and generate an error. (Although I guess the qemu driver is the only one 
that has a reasonably beefy deviceValidateCallback function (for hostdev 
devices it ends up going to qemuValidateDomainDeviceDefHostdev()), so I 
suppose you *could* counterclaim that updating that is on the people 
actively working on QEMU :-P)


4) if you consider addition of this new attribute to be noteworthy, you 
should add an entry to the news.xml file


There's also a comment inline with the patch:


>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6f43976815..79a5176ccd 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5051,6 +5051,9 @@
>               or hot-plugging the device and <code>virNodeDeviceReAttach</code>
>               (or <code>virsh nodedev-reattach</code>) after hot-unplug or
>               stopping the guest.
> +            When <code>permissive</code> is "yes" the pci config space access
> +            will not be filtered. This might be a security issue. The default
> +            is "no".
>             </dd>
>             <dt><code>scsi</code></dt>
>             <dd>For SCSI devices, user is responsible to make sure the device
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 65d6580434..9389eec3d8 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3064,6 +3064,11 @@
>                 <ref name="virYesNo"/>
>               </attribute>
>             </optional>
> +          <optional>
> +            <attribute name="permissive">
> +              <ref name="virYesNo"/>
> +            </attribute>
> +          </optional>
>             <interleave>
>               <element name="source">
>                 <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8e8146374c..607cae61d4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8419,6 +8419,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>       virDomainHostdevSubsysSCSIVHostPtr scsihostsrc = &def->source.subsys.u.scsi_host;
>       virDomainHostdevSubsysMediatedDevPtr mdevsrc = &def->source.subsys.u.mdev;
>       g_autofree char *managed = NULL;
> +    g_autofree char *permissive = NULL;
>       g_autofree char *sgio = NULL;
>       g_autofree char *rawio = NULL;
>       g_autofree char *backendStr = NULL;
> @@ -8434,6 +8435,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>       if ((managed = virXMLPropString(node, "managed")) != NULL)
>           ignore_value(virStringParseYesNo(managed, &def->managed));
>   
> +    if ((permissive = virXMLPropString(node, "permissive")) != NULL) {
> +        if (STREQ(permissive, "yes"))
> +            def->permissive = true;
> +    }
> +


Parsing/storing/formatting of yes/no values in the XML is currently 
handled in a few different ways in libvirt, so I may be contradicting 
the preferred method of someone else [*], but my preference is these 
days is to add the attribute to the C struct in conf/*.h as 
virTristateBool rather than plain bool, and then use 
virTristateBoolTypeFromString() to parse it (into a temporary int, so 
you can check for -1 return indicating an unrecognized value), and 
virTristateBoolTypeToString() when formatting back into XML. The 
advantage of doing it this way is that a parse/format cycle of the XML 
will always be idempotent (e.g., if someone says "permissive='no'" that 
will be properly reproduced, as will "permissive='yes'", and the absence 
of any permissive attribute). Additionally, if someone attempts to set 
anything other than "yes or "no", it will be flagged as an error.


[*] One example right next to the new code you added - there is a 
function virStringParseYesNo() (added last year), that is being used to 
parse the value of the "managed" attribute (and several other yes/no 
values, but not *all* of them), but I don't like that function because 
it requires that you either 1) use a plain bool (thus losing the 
difference between "no" and [unspecified]), or 2) that you take an extra 
step to convert the returned bool into a virTristateBool, which just 
seems pointless. Note that the "managed" attribute was originally parsed 
in exactly the same way you're parsing "permissive", but it was 
converted to use virStringParseYesNo() when that function was added; the 
error return value from that function is being ignored in this case in 
order to preserve backward compatibility, which isn't an issue for your 
new attribute - there is no "backward" to be compatible with :-) (also, 
the managed attribute has been in the config since *long* before 
virTristateBool ever existed, and the need to preserve backward 
compatibility at least partly explains why it was never converted to use 
the new type).




>       sgio = virXMLPropString(node, "sgio");
>       rawio = virXMLPropString(node, "rawio");
>       model = virXMLPropString(node, "model");
> @@ -25942,6 +25948,8 @@ virDomainActualNetDefFormat(virBufferPtr buf,
>           virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def);
>           if  (hostdef && hostdef->managed)
>               virBufferAddLit(buf, " managed='yes'");
> +        if  (hostdef && hostdef->permissive)
> +            virBufferAddLit(buf, " permissive='yes'");
>       }
>       if (def->trustGuestRxFilters)
>           virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
> @@ -26130,6 +26138,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<interface type='%s'", typeStr);
>       if (hostdef && hostdef->managed)
>           virBufferAddLit(buf, " managed='yes'");
> +    if (hostdef && hostdef->permissive)
> +        virBufferAddLit(buf, " permissive='yes'");
>       if (def->trustGuestRxFilters)
>           virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
>                             virTristateBoolTypeToString(def->trustGuestRxFilters));
> @@ -27914,6 +27924,8 @@ virDomainHostdevDefFormat(virBufferPtr buf,
>       if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
>           virBufferAsprintf(buf, " managed='%s'",
>                             def->managed ? "yes" : "no");
> +        if (def->permissive)
> +            virBufferAddLit(buf, " permissive='yes'");
>   
>           if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
>               scsisrc->sgio)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index aad3f82db7..b81a3ce901 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -345,6 +345,7 @@ struct _virDomainHostdevDef {
>       bool missing;
>       bool readonly;
>       bool shareable;
> +    bool permissive;
>       union {
>           virDomainHostdevSubsys subsys;
>           virDomainHostdevCaps caps;
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b3f67f817a..55f2a09e3e 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -2249,6 +2249,7 @@ libxlMakePCI(virDomainHostdevDefPtr hostdev, libxl_device_pci *pcidev)
>       pcidev->bus = pcisrc->addr.bus;
>       pcidev->dev = pcisrc->addr.slot;
>       pcidev->func = pcisrc->addr.function;
> +    pcidev->permissive = hostdev->permissive;
>   
>       return 0;
>   }