Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects

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.