Re: [PATCH v2] binder: use cred instead of task for selinux checks

From: Paul Moore <paul@paul-moore.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Todd Kjos <tkjos@google.com>,
	arve@android.com, tkjos@android.com, maco@android.com,
	christian@brauner.io, James Morris <jmorris@namei.org>,
	Serge Hallyn <serge@hallyn.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	keescook@chromium.org, jannh@google.com,
	Jeffrey Vander Stoep <jeffv@google.com>,
	zohar@linux.ibm.com, linux-security-module@vger.kernel.org,
	selinux@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org, joel@joelfernandes.org,
	kernel-team@android.com, stable@vger.kernel.org
Subject: Re: [PATCH v2] binder: use cred instead of task for selinux checks
Date: Tue, 5 Oct 2021 09:53:31 -0400	[thread overview]
Message-ID: <CAHC9VhTdyc6qagfFDLFteqTpayC4G=tNy1T7mueMKeZzU8QmwQ@mail.gmail.com> (raw)
In-Reply-To: <YVxTlBMSWBkLgSi9@kroah.com>

On Tue, Oct 5, 2021 at 9:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Oct 01, 2021 at 10:55:21AM -0700, Todd Kjos wrote:
> > Save the struct cred associated with a binder process
> > at initial open to avoid potential race conditions
> > when converting to a security ID.
> >
> > Since binder was integrated with selinux, it has passed
> > 'struct task_struct' associated with the binder_proc
> > to represent the source and target of transactions.
> > The conversion of task to SID was then done in the hook
> > implementations. It turns out that there are race conditions
> > which can result in an incorrect security context being used.
> >
> > Fix by saving the 'struct cred' during binder_open and pass
> > it to the selinux subsystem.
> >
> > Fixes: 79af73079d75 ("Add security hooks to binder and implement the
> > hooks for SELinux.")
> > Signed-off-by: Todd Kjos <tkjos@google.com>
> > Cc: stable@vger.kernel.org # 5.14+ (need backport for earlier stables)
> > ---
> > v2: updated comments as suggested by Paul Moore
> >
> >  drivers/android/binder.c          | 14 +++++----
> >  drivers/android/binder_internal.h |  4 +++
> >  include/linux/lsm_hook_defs.h     | 14 ++++-----
> >  include/linux/lsm_hooks.h         | 14 ++++-----
> >  include/linux/security.h          | 28 +++++++++---------
> >  security/security.c               | 14 ++++-----
> >  security/selinux/hooks.c          | 48 +++++++++----------------------
> >  7 files changed, 60 insertions(+), 76 deletions(-)
>
> Ideally I could get an ack from the security developers before taking
> this in my tree...

This should probably go in via one of the security trees, e.g. SELinux
or LSM, rather than the binder/driver tree.

-- 
paul moore
www.paul-moore.com