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

[John Ferlan]

On 09/21/2017 04:33 AM, Erik Skultety wrote:
> 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 at redhat.com> 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 at redhat.com>
>>> 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 at gmail.com>
>>>>>> ---
>>>>>>  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.
> 

I squashed this into the v3 patch and pushed.

Tks -

John