12:01 p.m.
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>
>
> 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>
> Signed-off-by: Marek Marczykowski-Górecki
> <marmarek(a)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
>