linux-kernel - Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151009184838.GM2189@localhost.localdomain>
Date:	Fri, 9 Oct 2015 21:48:38 +0300
From:	Sergei Zviagintsev <sergei@...v.net>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...glemail.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 38/44] kdbus: Fix error path in kdbus_user_lookup()

Hi David,

On Thu, Oct 08, 2015 at 05:06:57PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:32 PM, Sergei Zviagintsev <sergei@...v.net> wrote:
> > If idr_alloc() fails, we shouldn't call idr_remove() as latter produces
> > warning when called on non-allocated ids. Split cleanup code into three
> > parts for differrent cleanup scenarios before and after idr_alloc().
> >
> > Signed-off-by: Sergei Zviagintsev <sergei@...v.net>
> > ---
> >  ipc/kdbus/domain.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/ipc/kdbus/domain.c b/ipc/kdbus/domain.c
> > index ac9f760c150d..31cd09fb572f 100644
> > --- a/ipc/kdbus/domain.c
> > +++ b/ipc/kdbus/domain.c
> > @@ -208,7 +208,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >         u = kzalloc(sizeof(*u), GFP_KERNEL);
> >         if (!u) {
> >                 ret = -ENOMEM;
> > -               goto exit;
> > +               goto exit_unlock;
> >         }
> >
> >         kref_init(&u->kref);
> > @@ -225,7 +225,7 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >                         ret = idr_alloc(&domain->user_idr, u, __kuid_val(uid),
> >                                         __kuid_val(uid) + 1, GFP_KERNEL);
> >                         if (ret < 0)
> > -                               goto exit;
> > +                               goto exit_free;
> >                 }
> >         }
> >
> > @@ -235,19 +235,19 @@ struct kdbus_user *kdbus_user_lookup(struct kdbus_domain *domain, kuid_t uid)
> >          */
> >         ret = ida_simple_get(&domain->user_ida, 1, 0, GFP_KERNEL);
> >         if (ret < 0)
> > -               goto exit;
> > +               goto exit_idr;
> >
> 
> Why not simply assign "u->uid = uid;" _after_ doing the idr operations?

If I understand it right, in this case we have to firstly assign
INVALID_UID to u->uid (to check it with uid_valid() in the error path)
and then do 'u->uid = uid'. But from the first sight it would be not so
obvious and may require adding some comment on it. Isn't it better to
stay explicit here by maintaining several goto labels?

> 
> Thanks
> David
> 
> >         u->id = ret;
> >         mutex_unlock(&domain->lock);
> >         return u;
> >
> > -exit:
> > -       if (u) {
> > -               if (uid_valid(u->uid))
> > -                       idr_remove(&domain->user_idr, __kuid_val(u->uid));
> > -               kdbus_domain_unref(u->domain);
> > -               kfree(u);
> > -       }
> > +exit_idr:
> > +       if (uid_valid(u->uid))
> > +               idr_remove(&domain->user_idr, __kuid_val(u->uid));
> > +exit_free:
> > +       kdbus_domain_unref(u->domain);
> > +       kfree(u);
> > +exit_unlock:
> >         mutex_unlock(&domain->lock);
> >         return ERR_PTR(ret);
> >  }
> > --
> > 1.8.3.1
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/