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

[ashish mittal <ashmit602@xxxxxxxxx>]

On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list