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

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(a)invisiblethingslab.com&gt; >> >> 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(a)invisiblethingslab.com&gt; >> Signed-off-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com&gt; >> --- >> 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 >> > >