Re: [PATCH v2 1/3] libxl: Add 'permissive' option for PCI devices

Laine Stump <laine@xxxxxxxxxx> · Thu, 21 May 2020 13:01:48 -0400

On 5/8/20 4:54 PM, Jim Fehlig wrote:
On 4/24/20 9:07 PM, Marek Marczykowski-Górecki wrote:
From: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>

By setting the permissive flag the Xen 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. Because of it (and that the attribute is Xen only), in an
example include "permissive='no'" - to not expose users copying from
documentation to extra risk.

Example usage:

     <devices>
       ...
       <hostdev mode='subsystem' type='pci' managed='yes' 
permissive='yes'>

As you know I always struggle with XML modeling and naming, and IIRC 
the 'managed' attribute has been a source of pain in the past, so it 
would be nice to have someone else opine on the introduction of 
'permissive' attribute. I see Laine reviewed V1 and as I recall has 
commented on the managed attribute in the past, so let's see if he has 
any advice :-).


Sorry, this message got lost in the firehose, and Jim just now reminded 
me of it on IRC.


I seriously don't remember what I may have said in the past about the 
managed='yes' attribute. Could have been something about the default 
being 'yes' instead of 'no', or could have been about its placement as 
an attribute of the toplevel <hostdev> element rather than being made a 
part of the <source> subelement (since it is really associated with what 
is done on the host side to get the device setup and hooked into the 
qemu process).


For this "permissive" attribute, I don't really know enough about it to 
know where it should go. Does it affect how the device appears in the 
guest? (in which case maybe it should be a part of <target>) or only the 
trappings around it in the host? (so maybe it should be a part of 
<source>). If not totally one or the other, could the setting of this 
option be changed and guest ABI would remain unchanged (but just some 
operations would now fail / succeed when they had done the opposite in 
the past)? In that case, again maybe <source>.


I guess the answers to that would help guide me if I was making the 
decision about where to put the new setting.


As for the *name* of it? That's even more difficult for me to have any 
opinion on since, again, I really have no understanding of what it's 
doing or why it's necessary.


One thing I would ask (maybe I did before) - wherever you put it, we 
(people who use qemu) would *really* appreciate if you added a clause to 
the post-parse validation in the qemu driver to cause an error if this 
option is found in a qemu domain config :-)


One option I considered was 'filtered' and 'unfiltered' values for a 
"config space" attribute, similar to sgio for scsi hostdevs. But I 
can't think of a good name for an attribute that "filters writes to 
the PCI hostdev config space".

         <source>
           <address domain='0x0000' bus='0x06' slot='0x02' 
function='0x0'/>
         </source>
       </hostdev>
     </devices>

Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Marek Marczykowski-Górecki 
<marmarek@xxxxxxxxxxxxxxxxxxxxxx>
---
Changes in v2:
  - improve documentation (version info, example)
  - update schema
  - use virTristateBool
  - add a test
  - improve commit message
  - news entry
---
  docs/formatdomain.html.in                      |  7 +++++--
  docs/news.xml                                  | 11 +++++++++++-

Additions to news is usually done in a sparate patch.

docs/schemas/domaincommon.rng                  | 10 ++++++++++-
  src/conf/domain_conf.c                         | 19 
+++++++++++++++++++-
  src/conf/domain_conf.h                         |  1 +-
  src/libxl/libxl_conf.c                         |  1 +-
  tests/libxlxml2domconfigdata/moredevs-hvm.json |  6 ++++++-
  tests/libxlxml2domconfigdata/moredevs-hvm.xml  |  5 +++++-
  8 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c530573..0d1146e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4987,7 +4987,7 @@
  <pre>
  ...
  &lt;devices&gt;
-  &lt;hostdev mode='subsystem' type='pci' managed='yes'&gt;
+  &lt;hostdev mode='subsystem' type='pci' managed='yes' 
permissive='no'&gt;
      &lt;source&gt;
        &lt;address domain='0x0000' bus='0x06' slot='0x02' 
function='0x0'/&gt;
      &lt;/source&gt;
@@ -5082,7 +5082,10 @@
              (or <code>virsh nodedev-detach</code> before starting 
the guest
              or hot-plugging the device and 
<code>virNodeDeviceReAttach</code>
              (or <code>virsh nodedev-reattach</code>) after 
hot-unplug or
-            stopping the guest.
+            stopping the guest. When <code>permissive</code>
+            (<span class="since">since 6.3.0, Xen only</span>) is "yes"

6.4.0

Regards,
Jim

+            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/news.xml b/docs/news.xml
index 5835013..a8e992a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -109,6 +109,17 @@
            and/or fine tuned per individual host.
          </description>
        </change>
+      <change>
+        <summary>
+          xen: Add support for 'permissive' PCI device option
+        </summary>
+        <description>
+          <code>permissive</code> is a Xen-specific PCI device 
option that
+          disables config space write filtering. It is needed for 
proper
+          functioning of some devices using non-standard config space
+          registers.
+        </description>
+      </change>
      </section>
      <section title="Improvements">
      </section>
diff --git a/docs/schemas/domaincommon.rng 
b/docs/schemas/domaincommon.rng
index 7f18e5b..5a71222 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3065,6 +3065,11 @@
                <ref name="virYesNo"/>
              </attribute>
            </optional>
+          <optional>
+            <attribute name="permissive">
+              <ref name="virYesNo"/>
+            </attribute>
+          </optional>
            <interleave>
              <element name="source">
                <optional>
@@ -4899,6 +4904,11 @@
          <ref name="virYesNo"/>
        </attribute>
      </optional>
+    <optional>
+      <attribute name="permissive">
+        <ref name="virYesNo"/>
+      </attribute>
+    </optional>
      <choice>
        <ref name="hostdevsubsyspci"/>
        <ref name="hostdevsubsysusb"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 89cd8c5..fe1a864 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8434,6 +8434,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;
@@ -8449,6 +8450,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr 
node,
      if ((managed = virXMLPropString(node, "managed")) != NULL)
          ignore_value(virStringParseYesNo(managed, &def->managed));
  +    if ((permissive = virXMLPropString(node, "permissive")) != 
NULL) {
+        if ((def->permissive = 
virTristateBoolTypeFromString(permissive)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("unknown hostdev permissive setting 
'%s'"),
+                           permissive);
+            return -1;
+        }
+    }
+
      sgio = virXMLPropString(node, "sgio");
      rawio = virXMLPropString(node, "rawio");
      model = virXMLPropString(node, "model");
@@ -26091,6 +26101,9 @@ virDomainActualNetDefFormat(virBufferPtr buf,
          virDomainHostdevDefPtr hostdef = 
virDomainNetGetActualHostdev(def);
          if  (hostdef && hostdef->managed)
              virBufferAddLit(buf, " managed='yes'");
+        if  (hostdef && hostdef->permissive)
+            virBufferAsprintf(buf, " permissive='%s'",
+ virTristateBoolTypeToString(hostdef->permissive));
      }
      if (def->trustGuestRxFilters)
          virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
@@ -26279,6 +26292,9 @@ virDomainNetDefFormat(virBufferPtr buf,
      virBufferAsprintf(buf, "<interface type='%s'", typeStr);
      if (hostdef && hostdef->managed)
          virBufferAddLit(buf, " managed='yes'");
+    if (hostdef && hostdef->permissive)
+        virBufferAsprintf(buf, " permissive='%s'",
+ virTristateBoolTypeToString(hostdef->permissive));
      if (def->trustGuestRxFilters)
          virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
virTristateBoolTypeToString(def->trustGuestRxFilters));
@@ -28063,6 +28079,9 @@ virDomainHostdevDefFormat(virBufferPtr buf,
      if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
          virBufferAsprintf(buf, " managed='%s'",
                            def->managed ? "yes" : "no");
+        if (def->permissive)
+            virBufferAsprintf(buf, " permissive='%s'",
+ virTristateBoolTypeToString(def->permissive));
            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 ecb80ef..24ec03c 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;
+    virTristateBool permissive;
      union {
          virDomainHostdevSubsys subsys;
          virDomainHostdevCaps caps;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 458dfc2..23dd0e4 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2284,6 +2284,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 == VIR_TRISTATE_BOOL_YES;
        return 0;
  }
diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json 
b/tests/libxlxml2domconfigdata/moredevs-hvm.json
index 7bfd68b..474aa2c 100644
--- a/tests/libxlxml2domconfigdata/moredevs-hvm.json
+++ b/tests/libxlxml2domconfigdata/moredevs-hvm.json
@@ -88,6 +88,12 @@
              "dev": 16,
              "bus": 10,
          "rdm_policy": "invalid"
+        },
+        {
+            "dev": 8,
+            "bus": 10,
+            "permissive": true,
+        "rdm_policy": "invalid"
          }
      ],
      "vfbs": [
diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.xml 
b/tests/libxlxml2domconfigdata/moredevs-hvm.xml
index f7eb09f..1b6b738 100644
--- a/tests/libxlxml2domconfigdata/moredevs-hvm.xml
+++ b/tests/libxlxml2domconfigdata/moredevs-hvm.xml
@@ -48,6 +48,11 @@
          <address type='pci' domain='0x0000' bus='0x0a' slot='0x10' 
function='0x0'/>
        </source>
      </interface>
+    <hostdev mode='subsystem' type='pci' managed='yes' 
permissive='yes'>
+      <source>
+        <address domain='0x0000' bus='0x0a' slot='0x08' 
function='0x0'/>
+      </source>
+    </hostdev>
      <graphics type='vnc'/>
      <video>
        <model type='cirrus' vram='8192' heads='1' primary='yes'/>

base-commit: c3ace7e234ccd43d5a008d93e122e6d47cd58e17










