Passing a NULL value for the argument secAlias to the function qemuDomainGetTLSObjects would cause a segmentation fault in libvirtd. Changed code to not dereference a NULL secAlias. Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7dd6e5f..9ecdf0a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, } if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, - *secAlias, qemuCaps, tlsProps) < 0) + secAlias ? *secAlias : NULL, qemuCaps, + tlsProps) < 0) return -1; if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias)))

-- 2.5.5 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 09/20/2017 09:11 AM, Erik Skultety wrote: > On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote: >> Passing a NULL value for the argument secAlias to the function >> qemuDomainGetTLSObjects would cause a segmentation fault in >> libvirtd. >> >> Changed code to not dereference a NULL secAlias. >> >> Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; >> --- >> src/qemu/qemu_hotplug.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 7dd6e5f..9ecdf0a 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, >> } >> >> if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, >> - *secAlias, qemuCaps, tlsProps) < 0) >> + secAlias ? *secAlias : NULL, qemuCaps, >> + tlsProps) < 0) >> return -1; >> >> if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) > > A few lines above this context, we check whether we have a valid reference to > @secinfo and if so, we try to fill the @secAlias. Can we guarantee that when > @secinfo is passed, @secAlias has been set as well? I'm asking because I > haven't touched the TLS code yet and I just ran a few argument combinations > mentally and this one got me wondering. If the combination I'm describing is a > pure non-sense, the patch can go straight in. > True, right, ugh. A result of multiple rewrites along the way in patch review resulting in me missing something. In the case of the Veritas series, it won't matter because secinfo will be NULL, but that condition should be if (secinfo && tlsAlias)

On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote: > Passing a NULL value for the argument secAlias to the function > qemuDomainGetTLSObjects would cause a segmentation fault in > libvirtd. > > Changed code to not dereference a NULL secAlias. > > Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; > --- > src/qemu/qemu_hotplug.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 7dd6e5f..9ecdf0a 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, > } > > if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, > - *secAlias, qemuCaps, tlsProps) < 0) > + secAlias ? *secAlias : NULL, qemuCaps, > + tlsProps) < 0) > return -1; > > if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) A few lines above this context, we check whether we have a valid reference to @secinfo and if so, we try to fill the @secAlias. Can we guarantee that when @secinfo is passed, @secAlias has been set as well?

I'm asking because I haven't touched the TLS code yet and I just ran a few argument combinations mentally and this one got me wondering. If the combination I'm describing is a pure non-sense, the patch can go straight in. Thanks, Erik > -- > 2.5.5 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote: > On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > > On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote: > > > Passing a NULL value for the argument secAlias to the function > > > qemuDomainGetTLSObjects would cause a segmentation fault in > > > libvirtd. > > > > > > Changed code to not dereference a NULL secAlias. > > > > > > Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; > > > --- > > > src/qemu/qemu_hotplug.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > > > index 7dd6e5f..9ecdf0a 100644 > > > --- a/src/qemu/qemu_hotplug.c > > > +++ b/src/qemu/qemu_hotplug.c > > > @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps, > > > } > > > > > > if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify, > > > - *secAlias, qemuCaps, tlsProps) < 0) > > > + secAlias ? *secAlias : NULL, > > qemuCaps, > > > + tlsProps) < 0) > > > return -1; > > > > > > if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) > > > > A few lines above this context, we check whether we have a valid reference > > to > > @secinfo and if so, we try to fill the @secAlias. Can we guarantee that > > when > > @secinfo is passed, @secAlias has been set as well? > > > When secinfo is passed, the line marked below should guarantee that > secAlias is set to not-null. > > if (secinfo) { > if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) > return -1; > > if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) > <=== this should ensure secAlias != NULL or return -1 > return -1; > } > See John's reply to my response, long story short, in case of Veritas, this couldn't happen, but in general, we should cover the use case I'm describing as well, which is just a matter of very simple 'if' statement adjustment. Erik

On Thu, Sep 21, 2017 at 01:24:29AM -0700, ashish mittal wrote: > On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet(a)redhat.com&gt; wrote: > >> On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote: >>> On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet(a)redhat.com&gt; >> wrote: >>> >>>> On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote: >>>>> Passing a NULL value for the argument secAlias to the function >>>>> qemuDomainGetTLSObjects would cause a segmentation fault in >>>>> libvirtd. >>>>> >>>>> Changed code to not dereference a NULL secAlias. >>>>> >>>>> Signed-off-by: Ashish Mittal <ashmit602(a)gmail.com&gt; >>>>> --- >>>>> src/qemu/qemu_hotplug.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >>>>> index 7dd6e5f..9ecdf0a 100644 >>>>> --- a/src/qemu/qemu_hotplug.c >>>>> +++ b/src/qemu/qemu_hotplug.c >>>>> @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr >> qemuCaps, >>>>> } >>>>> >>>>> if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, >> tlsVerify, >>>>> - *secAlias, qemuCaps, tlsProps) >> < 0) >>>>> + secAlias ? *secAlias : NULL, >>>> qemuCaps, >>>>> + tlsProps) < 0) >>>>> return -1; >>>>> >>>>> if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias))) >>>> >>>> A few lines above this context, we check whether we have a valid >> reference >>>> to >>>> @secinfo and if so, we try to fill the @secAlias. Can we guarantee that >>>> when >>>> @secinfo is passed, @secAlias has been set as well? >>> >>> >>> When secinfo is passed, the line marked below should guarantee that >>> secAlias is set to not-null. >>> >>> if (secinfo) { >>> if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) >>> return -1; >>> >>> if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) >>> <=== this should ensure secAlias != NULL or return -1 >>> return -1; >>> } >>> >> >> See John's reply to my response, long story short, in case of Veritas, this >> couldn't happen, but in general, we should cover the use case I'm >> describing as >> well, which is just a matter of very simple 'if' statement adjustment. >> >> Erik >> > > if (secinfo) { > if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) > return -1; > > if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) > return -1; > } > > Above code segment suggests that if secinfo != NULL, then secAlias should > never be NULL . If that were not the case, we would have already seen a > crash from such a case. > > I'm afraid changing the above "if" condition to "if (secinfo && secAlias)" > is changing the behavior of the code to say "It is OK if secinfo != NULL > and secAlias == NULL, I'll just skip the setting of *secAlias". I don't > know the code well enough to say if this, or the above, is expected True, good point, in that case I'd make a very tiny adjustment and go with: if (!secAlias || !(*secAlias = qemuDomainGetSecretAESAlias())) return -1; Whatever the case, you're right in your reasoning and better be safe than sorry with this.