[libvirt] [PATCH] rng: restrict passthrough names to known-good files
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work. [1]https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023 * src/conf/domain_conf.c (virDomainRNGDefParseXML): Only allow /dev/random and /dev/hwrng. * docs/schemas/domaincommon.rng: Flag invalid files. * docs/formatdomain.html.in (elementsRng): Document this. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args: Update test to match. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml: Likewise. --- This needs to be acked before libvirt 1.0.3; otherwise we are stuck supporting arbitrary name passthrough. docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 5 ++++- src/conf/domain_conf.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1835b39..4cafc92 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4310,7 +4310,8 @@ qemu-kvm -net nic,model=? /dev/null <code>model</code> attribute. Supported source models are: </p> <ul> - <li>'random' — /dev/random (default) or similar device as source</li> + <li>'random' — /dev/random (default) or /dev/hwrng + device as source (for now, no other sources are permitted)</li> <li>'egd' — a EGD protocol backend</li> </ul> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e7231cc..4b60885 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3511,7 +3511,10 @@ <attribute name="model"> <value>random</value> </attribute> - <ref name="filePath"/> + <choice> + <value>/dev/random</value> + <value>/dev/hwrng</value> + </choice> </group> <group> <attribute name="model"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 995cf0c..9c96cf1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7423,6 +7423,13 @@ virDomainRNGDefParseXML(const xmlNodePtr node, switch ((enum virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); + if (STRNEQ(def->source.file, "/dev/random") && + STRNEQ(def->source.file, "/dev/hwrng")) { + virReportError(VIR_ERR_XML_ERROR, + _("file '%s' is not a supported random source"), + def->source.file); + goto error; + } break; case VIR_DOMAIN_RNG_BACKEND_EGD: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index ad27132..7ab9dbc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -2,5 +2,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \ -S -M pc -m 214 -smp 1 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ --object 'rng-random,id=rng0,filename=/test/ph<ile' \ +-object rng-random,id=rng0,filename=/dev/hwrng \ -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index 0658f4b..1e2c4be 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -17,7 +17,7 @@ <controller type='usb' index='0'/> <memballoon model='virtio'/> <rng model='virtio'> - <backend model='random'>/test/ph<ile</backend> + <backend model='random'>/dev/hwrng</backend> </rng> </devices> </domain> -- 1.8.1.4
Eric Blake <eblake@redhat.com> writes:
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work.
Acked-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori
[1]https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
* src/conf/domain_conf.c (virDomainRNGDefParseXML): Only allow /dev/random and /dev/hwrng. * docs/schemas/domaincommon.rng: Flag invalid files. * docs/formatdomain.html.in (elementsRng): Document this. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args: Update test to match. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml: Likewise. ---
This needs to be acked before libvirt 1.0.3; otherwise we are stuck supporting arbitrary name passthrough.
docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 5 ++++- src/conf/domain_conf.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1835b39..4cafc92 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4310,7 +4310,8 @@ qemu-kvm -net nic,model=? /dev/null <code>model</code> attribute. Supported source models are: </p> <ul> - <li>'random' — /dev/random (default) or similar device as source</li> + <li>'random' — /dev/random (default) or /dev/hwrng + device as source (for now, no other sources are permitted)</li> <li>'egd' — a EGD protocol backend</li> </ul> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e7231cc..4b60885 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3511,7 +3511,10 @@ <attribute name="model"> <value>random</value> </attribute> - <ref name="filePath"/> + <choice> + <value>/dev/random</value> + <value>/dev/hwrng</value> + </choice> </group> <group> <attribute name="model"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 995cf0c..9c96cf1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7423,6 +7423,13 @@ virDomainRNGDefParseXML(const xmlNodePtr node, switch ((enum virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); + if (STRNEQ(def->source.file, "/dev/random") && + STRNEQ(def->source.file, "/dev/hwrng")) { + virReportError(VIR_ERR_XML_ERROR, + _("file '%s' is not a supported random source"), + def->source.file); + goto error; + } break;
case VIR_DOMAIN_RNG_BACKEND_EGD: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index ad27132..7ab9dbc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -2,5 +2,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \ -S -M pc -m 214 -smp 1 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ --object 'rng-random,id=rng0,filename=/test/ph<ile' \ +-object rng-random,id=rng0,filename=/dev/hwrng \ -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index 0658f4b..1e2c4be 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -17,7 +17,7 @@ <controller type='usb' index='0'/> <memballoon model='virtio'/> <rng model='virtio'> - <backend model='random'>/test/ph<ile</backend> + <backend model='random'>/dev/hwrng</backend> </rng> </devices> </domain> -- 1.8.1.4
On 03/04/2013 04:31 PM, Anthony Liguori wrote:
Eric Blake <eblake@redhat.com> writes:
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work.
Acked-by: Anthony Liguori <aliguori@us.ibm.com>
Thanks; I've pushed this into libvirt. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/04/2013 06:31 PM, Anthony Liguori wrote:
Eric Blake <eblake@redhat.com> writes:
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work. Acked-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
[1]https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
* src/conf/domain_conf.c (virDomainRNGDefParseXML): Only allow /dev/random and /dev/hwrng. * docs/schemas/domaincommon.rng: Flag invalid files. * docs/formatdomain.html.in (elementsRng): Document this. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args: Update test to match. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml: Likewise. ---
This needs to be acked before libvirt 1.0.3; otherwise we are stuck supporting arbitrary name passthrough.
docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 5 ++++- src/conf/domain_conf.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1835b39..4cafc92 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4310,7 +4310,8 @@ qemu-kvm -net nic,model=? /dev/null <code>model</code> attribute. Supported source models are: </p> <ul> - <li>'random' — /dev/random (default) or similar device as source</li> + <li>'random' — /dev/random (default) or /dev/hwrng + device as source (for now, no other sources are permitted)</li> <li>'egd' — a EGD protocol backend</li> </ul> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e7231cc..4b60885 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3511,7 +3511,10 @@ <attribute name="model"> <value>random</value> </attribute> - <ref name="filePath"/> + <choice> + <value>/dev/random</value> + <value>/dev/hwrng</value> + </choice> </group> <group> <attribute name="model"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 995cf0c..9c96cf1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7423,6 +7423,13 @@ virDomainRNGDefParseXML(const xmlNodePtr node, switch ((enum virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); + if (STRNEQ(def->source.file, "/dev/random") && + STRNEQ(def->source.file, "/dev/hwrng")) { + virReportError(VIR_ERR_XML_ERROR, + _("file '%s' is not a supported random source"), + def->source.file); + goto error; + } break;
case VIR_DOMAIN_RNG_BACKEND_EGD: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index ad27132..7ab9dbc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -2,5 +2,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \ -S -M pc -m 214 -smp 1 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ --object 'rng-random,id=rng0,filename=/test/ph<ile' \ +-object rng-random,id=rng0,filename=/dev/hwrng \ -device virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index 0658f4b..1e2c4be 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -17,7 +17,7 @@ <controller type='usb' index='0'/> <memballoon model='virtio'/> <rng model='virtio'> - <backend model='random'>/test/ph<ile</backend> + <backend model='random'>/dev/hwrng</backend> </rng> </devices> </domain> -- 1.8.1.4
ACK.
On 03/04/13 23:49, Eric Blake wrote:
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work.
[1]https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
* src/conf/domain_conf.c (virDomainRNGDefParseXML): Only allow /dev/random and /dev/hwrng. * docs/schemas/domaincommon.rng: Flag invalid files. * docs/formatdomain.html.in (elementsRng): Document this. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args: Update test to match. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml: Likewise. ---
This needs to be acked before libvirt 1.0.3; otherwise we are stuck supporting arbitrary name passthrough.
Okay, this is fair enough for now.
docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 5 ++++- src/conf/domain_conf.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-)
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 995cf0c..9c96cf1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7423,6 +7423,13 @@ virDomainRNGDefParseXML(const xmlNodePtr node, switch ((enum virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); + if (STRNEQ(def->source.file, "/dev/random") && + STRNEQ(def->source.file, "/dev/hwrng")) { + virReportError(VIR_ERR_XML_ERROR, + _("file '%s' is not a supported random source"), + def->source.file); + goto error;
This is yet another part of the stuff that will need to be moved out of the parser when I finish the XML parsing callbacks. Peter
On 03/05/2013 02:28 AM, Peter Krempa wrote:
On 03/04/13 23:49, Eric Blake wrote:
There is some controversy[1] on the qemu list on whether qemu should have ever allowed arbitrary file name passthrough, or whether it should be restricted to JUST /dev/random and /dev/hwrng. It is always easier to add support for additional filenames than it is to remove support for something once released, so this patch restricts libvirt 1.0.3 (where the virtio-random backend was first supported) to just the two uncontroversial names, letting us defer to a later date any decision on whether supporting arbitrary files makes sense. Additionally, since qemu 1.4 does NOT support /dev/fdset/nnn fd passthrough for the backend, limiting to just two known names means that we don't get tempted to try fd passthrough where it won't work.
[1]https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
* src/conf/domain_conf.c (virDomainRNGDefParseXML): Only allow /dev/random and /dev/hwrng. * docs/schemas/domaincommon.rng: Flag invalid files. * docs/formatdomain.html.in (elementsRng): Document this. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args: Update test to match. * tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml: Likewise. ---
This needs to be acked before libvirt 1.0.3; otherwise we are stuck supporting arbitrary name passthrough.
Okay, this is fair enough for now.
docs/formatdomain.html.in | 3 ++- docs/schemas/domaincommon.rng | 5 ++++- src/conf/domain_conf.c | 7 +++++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- 5 files changed, 15 insertions(+), 4 deletions(-)
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 995cf0c..9c96cf1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7423,6 +7423,13 @@ virDomainRNGDefParseXML(const xmlNodePtr node, switch ((enum virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); + if (STRNEQ(def->source.file, "/dev/random") && + STRNEQ(def->source.file, "/dev/hwrng")) { + virReportError(VIR_ERR_XML_ERROR, + _("file '%s' is not a supported random source"), + def->source.file); + goto error;
This is yet another part of the stuff that will need to be moved out of the parser when I finish the XML parsing callbacks.
Is it? I think if something is described in the RNG, it can be validated directly in the parser. I think the only time that we should need to move the validation out of the parser is in cases where it isn't possible to describe in the RNG (i.e. in order to determine validity, you must a) know information specific to the hypervisor or host environment, or b) know the settings of other elements of the domain beyond the current object being parsed.) In this case, the value of def->source.file is being limited dependent on def->backend (or in xpath nomenclature "./devices/rng/backend" is being restricted based on the value of "./devices/rng/backend/@model". There is no dependency on hypervisor, host, or any piece of the domain definition outside this one element. So even if this wasn't a (possibly) temporary restriction, I think it's fine where it is.
participants (4)
-
Anthony Liguori -
Eric Blake -
Laine Stump -
Peter Krempa