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

[Erik Skultety]

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