On Tue, Jul 02, 2024 at 16:05:33 +0200, Peter Krempa wrote:
> 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.
And since it was easy to miss the fact that it's a union and thus
overwrites other stuff please also add a comment so that the code
doesn't invite to be "optimized" again.
