On 4/17/20 12:33 PM, Marek Marczykowski-Górecki wrote:
From: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
By setting the permissive flag the 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.
Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
---
docs/formatdomain.html.in | 3 +++
docs/schemas/domaincommon.rng | 5 +++++
src/conf/domain_conf.c | 12 ++++++++++++
src/conf/domain_conf.h | 1 +
src/libxl/libxl_conf.c | 1 +
5 files changed, 22 insertions(+)
I won't pretend to know anything about PCI device passthrough in Xen
:-), so I'll just comment about the contruction of the patch rather than
what it does (which is really just a single line in libxl_conf.c).
1) A patch that adds a new attribute to the XML should have an example
of the use of the attribute in the commit message and in the docs.
You've added a comment to the docs (formatdomain.html) naming the option
an in general what it does, but not an example (which forced me to
actually read the code! :-P)
2) The comment in the docs needs to document that it is supported "since
6.3.0 (Xen only)". The fact that it is Xen-only should also be noted in
the commit message (I, with my QEMU-goggles, was confused for a bit when
I thought that you were adding support for some new qemu commandline
knob, but had forgotten to add the bit that actually tweaked the knob).
Usually we will start the subject line of a qemu-specific patch with
"qemu: bobloblaw", so you could start this patch with "libxl: Add ...",
or maybe "libxl/conf: Add" if you wanted to be pedantic about it.
3) Since this attribute is supported only for Xen, the postparse
validation functions for other hypervisors need to check if it is set,
and generate an error. (Although I guess the qemu driver is the only one
that has a reasonably beefy deviceValidateCallback function (for hostdev
devices it ends up going to qemuValidateDomainDeviceDefHostdev()), so I
suppose you *could* counterclaim that updating that is on the people
actively working on QEMU :-P)
4) if you consider addition of this new attribute to be noteworthy, you
should add an entry to the news.xml file
There's also a comment inline with the patch:
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6f43976815..79a5176ccd 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5051,6 +5051,9 @@
or hot-plugging the device and <code>virNodeDeviceReAttach</code>
(or <code>virsh nodedev-reattach</code>) after hot-unplug or
stopping the guest.
+ When <code>permissive</code> is "yes" 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/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 65d6580434..9389eec3d8 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3064,6 +3064,11 @@
<ref name="virYesNo"/>
</attribute>
</optional>
+ <optional>
+ <attribute name="permissive">
+ <ref name="virYesNo"/>
+ </attribute>
+ </optional>
<interleave>
<element name="source">
<optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8e8146374c..607cae61d4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8419,6 +8419,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;
@@ -8434,6 +8435,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
if ((managed = virXMLPropString(node, "managed")) != NULL)
ignore_value(virStringParseYesNo(managed, &def->managed));
+ if ((permissive = virXMLPropString(node, "permissive")) != NULL) {
+ if (STREQ(permissive, "yes"))
+ def->permissive = true;
+ }
+
Parsing/storing/formatting of yes/no values in the XML is currently
handled in a few different ways in libvirt, so I may be contradicting
the preferred method of someone else [*], but my preference is these
days is to add the attribute to the C struct in conf/*.h as
virTristateBool rather than plain bool, and then use
virTristateBoolTypeFromString() to parse it (into a temporary int, so
you can check for -1 return indicating an unrecognized value), and
virTristateBoolTypeToString() when formatting back into XML. The
advantage of doing it this way is that a parse/format cycle of the XML
will always be idempotent (e.g., if someone says "permissive='no'" that
will be properly reproduced, as will "permissive='yes'", and the absence
of any permissive attribute). Additionally, if someone attempts to set
anything other than "yes or "no", it will be flagged as an error.
[*] One example right next to the new code you added - there is a
function virStringParseYesNo() (added last year), that is being used to
parse the value of the "managed" attribute (and several other yes/no
values, but not *all* of them), but I don't like that function because
it requires that you either 1) use a plain bool (thus losing the
difference between "no" and [unspecified]), or 2) that you take an extra
step to convert the returned bool into a virTristateBool, which just
seems pointless. Note that the "managed" attribute was originally parsed
in exactly the same way you're parsing "permissive", but it was
converted to use virStringParseYesNo() when that function was added; the
error return value from that function is being ignored in this case in
order to preserve backward compatibility, which isn't an issue for your
new attribute - there is no "backward" to be compatible with :-) (also,
the managed attribute has been in the config since *long* before
virTristateBool ever existed, and the need to preserve backward
compatibility at least partly explains why it was never converted to use
the new type).
sgio = virXMLPropString(node, "sgio");
rawio = virXMLPropString(node, "rawio");
model = virXMLPropString(node, "model");
@@ -25942,6 +25948,8 @@ virDomainActualNetDefFormat(virBufferPtr buf,
virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def);
if (hostdef && hostdef->managed)
virBufferAddLit(buf, " managed='yes'");
+ if (hostdef && hostdef->permissive)
+ virBufferAddLit(buf, " permissive='yes'");
}
if (def->trustGuestRxFilters)
virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
@@ -26130,6 +26138,8 @@ virDomainNetDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<interface type='%s'", typeStr);
if (hostdef && hostdef->managed)
virBufferAddLit(buf, " managed='yes'");
+ if (hostdef && hostdef->permissive)
+ virBufferAddLit(buf, " permissive='yes'");
if (def->trustGuestRxFilters)
virBufferAsprintf(buf, " trustGuestRxFilters='%s'",
virTristateBoolTypeToString(def->trustGuestRxFilters));
@@ -27914,6 +27924,8 @@ virDomainHostdevDefFormat(virBufferPtr buf,
if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
virBufferAsprintf(buf, " managed='%s'",
def->managed ? "yes" : "no");
+ if (def->permissive)
+ virBufferAddLit(buf, " permissive='yes'");
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 aad3f82db7..b81a3ce901 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;
+ bool permissive;
union {
virDomainHostdevSubsys subsys;
virDomainHostdevCaps caps;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b3f67f817a..55f2a09e3e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -2249,6 +2249,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;
return 0;
}
