[PATCH v2 1/3] libxl: Add 'permissive' option for PCI devices
Laine Stump
laine at redhat.com
Thu May 21 17:01:48 UTC 2020
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 at invisiblethingslab.com>
>>
>> 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 at invisiblethingslab.com>
>> Signed-off-by: Marek Marczykowski-Górecki
>> <marmarek at invisiblethingslab.com>
>> ---
>> 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>
>> ...
>> <devices>
>> - <hostdev mode='subsystem' type='pci' managed='yes'>
>> + <hostdev mode='subsystem' type='pci' managed='yes'
>> permissive='no'>
>> <source>
>> <address domain='0x0000' bus='0x06' slot='0x02'
>> function='0x0'/>
>> </source>
>> @@ -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
>>
>
>
More information about the libvir-list
mailing list