On Tue, Jul 02, 2024 at 18:49:13 +0530, Rayhan Faizel wrote:
> The current hostdev parsing logic sets rawio or sgio even if the hostdev type
> is not 'scsi'. The rawio field in virDomainHostdevSubsysSCSI overlaps with
> wwpn field in virDomainHostdevSubsysSCSIVHost, consequently setting a bogus
> pointer value such as 0x1 or 0x2 from virDomainHostdevSubsysSCSIVHost's
> point of view. This leads to a segmentation fault when it attempts to free
> wwpn.
>
> While setting sgio does not appear to crash, it shares the same flawed logic
> as setting rawio.
>
> Instead, we test the presence of their xpaths along with the hostdev type
> before setting either attributes. This patch also adds two test cases to
> exercise both scenarios.
Fixes: bdb95b520c53f9bacc6504fc51381bac4813be38
> Signed-off-by: Rayhan Faizel <rayhan.faizel@xxxxxxxxx>
> ---
> src/conf/domain_conf.c | 33 ++++++++-------
> ...scsi-vhost-rawio-invalid.x86_64-latest.err | 1 +
> .../hostdev-scsi-vhost-rawio-invalid.xml | 41 +++++++++++++++++++
> ...-scsi-vhost-sgio-invalid.x86_64-latest.err | 1 +
> .../hostdev-scsi-vhost-sgio-invalid.xml | 41 +++++++++++++++++++
> tests/qemuxmlconftest.c | 2 +
> 6 files changed, 102 insertions(+), 17 deletions(-)
> create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.x86_64-latest.err
> create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-rawio-invalid.xml
> create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.x86_64-latest.err
> create mode 100644 tests/qemuxmlconfdata/hostdev-scsi-vhost-sgio-invalid.xml
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 334152c4ff..5f8c6f2672 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6220,7 +6220,6 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> virDomainHostdevSubsysMediatedDev *mdevsrc = &def->source.subsys.u.mdev;
> virTristateBool managed;
> g_autofree char *model = NULL;
> - int rv;
>
> /* @managed can be read from the xml document - it is always an
> * attribute of the toplevel element, no matter what type of
> @@ -6256,33 +6255,33 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
> return -1;
> }
>
> - if ((rv = virXMLPropEnum(node, "sgio",
> - virDomainDeviceSGIOTypeFromString,
> - VIR_XML_PROP_NONZERO,
> - &scsisrc->sgio)) < 0) {
> + if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
> + virXPathBoolean("boolean(./@sgio)", ctxt)) {
Rather than doing an extra XPath query I'd prefer if you fix it by
introducing temporary variables which are filled instead of
'&scsisrc->sgio' and then the value is transferred when the check
passes.
