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

[ashish mittal <ashmit602@xxxxxxxxx>]

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;

    }

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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

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