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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux