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

[Erik Skultety]

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