[PATCH v2 1/3] libxl: Add 'permissive' option for PCI devices
Jim Fehlig
jfehlig at suse.com
Fri May 8 20:54:21 UTC 2020
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 :-).
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