1. Patchew
  2. Libvirt
  3. View series

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

Ashish Mittal posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1505910749-2945-1-git-send-email-ashmit602@gmail.com
src/qemu/qemu_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by Ashish Mittal 6 years, 7 months ago 
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@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)))
-- 
2.5.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by Erik Skultety 6 years, 7 months ago 
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@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? 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by John Ferlan 6 years, 7 months ago 

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@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? 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)

John

> Thanks,
> Erik
> 
>> --
>> 2.5.5
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by Erik Skultety 6 years, 7 months ago 
On Wed, Sep 20, 2017 at 10:41:49AM -0400, John Ferlan wrote:
>
>
> 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@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? 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)

tlsAlias?? I thought it should be @secAlias, since that's the one we're
setting, can you elaborate more? I'm not familiar with our TLS code so I'd like
to learn something :).

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by John Ferlan 6 years, 7 months ago 

On 09/20/2017 11:26 AM, Erik Skultety wrote:
> On Wed, Sep 20, 2017 at 10:41:49AM -0400, John Ferlan wrote:
>>
>>
>> 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@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? 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)
> 
> tlsAlias?? I thought it should be @secAlias, since that's the one we're
> setting, can you elaborate more? I'm not familiar with our TLS code so I'd like
> to learn something :).
> 
> Erik
> 

Well clearly I cannot juggle too many things in my head right now...
Yes, I meant secAlias not tlsAlias.

As for your other somewhat open ended question... For TLS there's a few
definitions in play - they're in cfg->{subsys}TLSx509{param}, where
{subsys} would be default, vnc, spice, chardev, migrate, and now vxhs.
The {param} would be verify and seretUUID).  Those are read from/filled
in from qemu.conf, but can also use default provided values.

For TLS, can either use a {subsys} specific directory to store the
certificates required (see qemu.conf for the list) or they can use a
default directory which may or may not exist, but then comes with a
default environment.

What the user can "change" is the "verify" option which allows them to
require that the client also present a certificate that the server can
validate. That requires a specifically named certificate file in the
certificate directory. QEMU has the code to handle reading (thankfully).

Additionally, the server certificate can be protected by a password.
That's where the secretUUID comes in. This ends up being a UUID for the
secret to decrypt the certificate. Since we have a need to not pass an
encoded password on the command line to qemu, we use the domain master
secret to encrypt the password. This all results in a lot more objects
that have a cascading and tight relationship.

When something uses a certificate it needs to let QEMU know whether it's
a client or server - that's where the listen value comes in. Migration
ends up being both and various chardevs also have both. It gets very
confusing, very fast.

For Veritas - there's already a server "somewhere" that could have a TLS
server certificate that would need a client to provide it's certificate.
The VxHS code also requires the client be authenticated as well - thus
the verify is always true. For libvirt purposes, it's always going to be
the client (e.g. listen = false) and there's no server certificate to be
decrypted, thus no need for a vxhs*secretUUID.

I'm sure to have missed a few details, but hopefully this helps a little
bit at least.  I'm not the expert in this matters ;-)! Just providing
some details on what little bit I figured out!

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by ashish mittal 6 years, 7 months ago 
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@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@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;
    }

Please let me know if any changes are required for this patch. Thanks!

Ashish


> 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@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by Erik Skultety 6 years, 7 months ago 
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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by ashish mittal 6 years, 7 months ago 
On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet@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@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@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
behavior. If the expected behavior is that secAlias should never be NULL
when secinfo != NULL, then a safer change might be -

    if (secinfo) {
        if (!secAlias)
            return -1;
         ....

Regards,
Ashish
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by Erik Skultety 6 years, 7 months ago 
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@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@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@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.

Thanks,
Erik

> behavior. If the expected behavior is that secAlias should never be NULL
> when secinfo != NULL, then a safer change might be -
>
>     if (secinfo) {
>         if (!secAlias)
>             return -1;
>          ....
>
> Regards,
> Ashish

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
Posted by John Ferlan 6 years, 7 months ago 

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@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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.