Re: [PATCH] binder: use cred instead of task for selinux checks
On Thu, Sep 30, 2021 at 10:45 PM 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
> Cc: sta...@vger.kernel.org # 5.14 (need backport for earlier stables)
> ---
> drivers/android/binder.c | 14 +
> drivers/android/binder_internal.h | 3 ++
> include/linux/lsm_hook_defs.h | 14 -
> include/linux/security.h | 28 +-
> security/security.c | 14 -
> security/selinux/hooks.c | 48 +--
> 6 files changed, 52 insertions(+), 69 deletions(-)
Thanks Todd, I'm happy to see someone with a better understanding of
binder than me pitch in to clean this up :) A couple of quick
comments/questions below ...
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 9edacc8b9768..ca599ebdea4a 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5055,6 +5056,7 @@ static int binder_open(struct inode *nodp, struct file
> *filp)
> spin_lock_init(&proc->outer_lock);
> get_task_struct(current->group_leader);
> proc->tsk = current->group_leader;
> + proc->cred = get_cred(filp->f_cred);
Is it *always* true that filp->f_cred is going to be the same as
current->group_leader->cred? Or rather does this help resolve the
issue of wanting the subjective creds but not being able to access
them mentioned in the task_sid_binder() comment? If the latter, it
might be nice to add something to the related comment in struct
binder_ref (below).
> INIT_LIST_HEAD(&proc->todo);
> init_waitqueue_head(&proc->freeze_wait);
> proc->default_priority = task_nice(current);
> diff --git a/drivers/android/binder_internal.h
> b/drivers/android/binder_internal.h
> index 402c4d4362a8..886fc327a534 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -364,6 +364,8 @@ struct binder_ref {
> *(invariant after initialized)
> * @tsk task_struct for group_leader of process
> *(invariant after initialized)
> + * @cred struct cred for group_leader of process
> + *(invariant after initialized)
Related to the question above. At the very least the comment should
probably be changed to indicate to make it clear the creds are coming
directly from the binder file/device and not always the group_leader.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e7ebd45ca345..c8bf3db90c8b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -255,29 +255,6 @@ static inline u32 task_sid_obj(const struct task_struct
> *task)
> return sid;
> }
>
> -/*
> - * get the security ID of a task for use with binder
> - */
> -static inline u32 task_sid_binder(const struct task_struct *task)
> -{
> - /*
> -* In many case where this function is used we should be using the
> -* task's subjective SID, but we can't reliably access the subjective
> -* creds of a task other than our own so we must use the objective
> -* creds/SID, which are safe to access. The downside is that if a
> task
> -* is temporarily overriding it's creds it will not be reflected here;
> -* however, it isn't clear that binder would handle that case well
> -* anyway.
> -*
> -* If this ever changes and we can safely reference the subjective
> -* creds/SID of another task, this function will make it easier to
> -* identify the various places where we make use of the task SIDs in
> -* the binder code. It is also likely that we will need to adjust
> -* the main drivers/android binder code as well.
> -*/
> - return task_sid_obj(task);
> -}
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: use cred instead of task for selinux checks
On Fri, Oct 1, 2021 at 10:38 AM Paul Moore wrote:
> On Thu, Sep 30, 2021 at 10:45 PM 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
> > Cc: sta...@vger.kernel.org # 5.14 (need backport for earlier stables)
> > ---
> > drivers/android/binder.c | 14 +
> > drivers/android/binder_internal.h | 3 ++
> > include/linux/lsm_hook_defs.h | 14 -
> > include/linux/security.h | 28 +-
> > security/security.c | 14 -
> > security/selinux/hooks.c | 48 +--
> > 6 files changed, 52 insertions(+), 69 deletions(-)
>
> Thanks Todd, I'm happy to see someone with a better understanding of
> binder than me pitch in to clean this up :) A couple of quick
> comments/questions below ...
Ooops, I was a little over zealous when trimming my response and I
accidentally cut off my comment that the associated comment blocks in
include/linux/lsm_hooks.h should also be updated to reflect the binder
LSM hook changes.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: use cred instead of task for selinux checks
On Tue, Oct 5, 2021 at 9:31 AM Greg KH 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
> > Cc: sta...@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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] binder: use cred instead of task for selinux checks
sk" is also wrong, because binder
> > > > > >>> looks at
> > > > > >>> the task that opened the binder device, not the task currently
> > > > > >>> performing the action.)
> > > > > >> I'm confused. Are you saying that the existing binder code is
> > > > > >> completely broken? Are you saying that neither "task" is correct?
> > > > > > Yeah, basically
> > > > >
> > > > > Well, hot biscuits and gravy!
> > > > >
> > > > > > - but luckily the actual impact this has is limited by
> > > > > > the transitions that SELinux permits. If domain1 has no way to
> > > > > > transition to domain2, then it can't abuse this bug to pretend to be
> > > > > > domain2. I do have a reproducer that lets Android's "shell" domain
> > > > > > send a binder transaction that appears to come from "runas", but
> > > > > > luckily "runas" has no interesting privileges with regards to
> > > > > > binder,
> > > > > > so that's not exploitable.
> > > > >
> > > > > You're counting on the peculiarities of the SELinux policy you're
> > > > > assuming is used to mask the fact that the hook isn't really doing
> > > > > what it is supposed to? Ouch.
> > > >
> > > > I'm not saying I like the current situation - I do think that this
> > > > needs to change. I'm just saying it probably isn't *exploitable*, and
> > > > exploitability often hinges on these little circumstantial details.
> > > >
> > > > > >> How does passing the creds from the wrong tasks "fix" the problem?
> > > > > > This patch is not passing the creds from the "wrong" tasks at all.
> > > > > > It
> > > > > > relies on the basic idea that when a security context opens a
> > > > > > resource, and then hands that resource to another context for
> > > > > > read/write operations, then you can effectively treat this as a
> > > > > > delegation of privileges from the original opener, and perform
> > > > > > access
> > > > > > checks against the credentials using which the resource was opened.
> > > > >
> > > > > OK. I can understand that without endorsing it.
> > > > >
> > > > > > In particular, we already have those semantics in the core kernel
> > > > > > for
> > > > > > ->read() and ->write() VFS operations - they are *not allowed* to
> > > > > > look
> > > > > > at the credentials of the caller, and if they want to make security
> > > > > > checks, they have to instead check against file->f_cred, which are
> > > > > > the
> > > > > > credentials using which the file was originally opened. (Yes, some
> > > > > > places still get that wrong.) Passing a file descriptor to another
> > > > > > task is a delegation of access, and the other task can then call
> > > > > > syscalls like read() / write() / mmap() on the file descriptor
> > > > > > without
> > > > > > needing to have any access to the underlying file.
> > > > >
> > > > > A mechanism sufficiently entrenched.
> > > >
> > > > It's not just "entrenched", it is a fundamental requirement for being
> > > > able to use file descriptor passing with syscalls like write(). If
> > > > task A gives a file descriptor to task B, then task B must be able to
> > > > write() to that FD without having to worry that the FD actually refers
> > > > to some sort of special file that interprets the written data as some
> > > > type of command, or something like that, and that this leads to task B
> > > > unknowingly passing through access checks.
> > > >
> > > > > > You can't really attribute binder transactions to specific tasks
> > > > > > that
> > > > > > are actually involved in the specific transaction, neither on the
> > > > > > sending side nor on the receiving side, because binder is built
> > > > > > around
> > > > > > passing data through memory mappings. Memory mappings can be
> > > > > > accessed
> > > > > > by multiple tasks, and even a task that does not currently have it
> > > > > > mapped could e.g. map it at a later time. And on top of that you
> > > > > > have
> > > > > > the problem that the receiving task might also go through privileged
> > > > > > execve() transitions.
> > > > >
> > > > > OK. I'm curious now as to why the task_struct was being passed to the
> > > > > hook in the first place.
> > > >
> > > > Probably because that's what most other LSM hooks looked like and the
> > > > authors/reviewers of the patch didn't realize that this model doesn't
> > > > really work for binder? FWIW, these hooks were added in commit
> > > > 79af73079d75 ("Add security hooks to binder and implement the hooks
> > > > for SELinux."). The commit message also just talks about "processes".
> > >
> > > Note that in the same code path (binder_transaction), sender_euid is
> > > set from proc->tsk and security_ctx is based on proc->tsk. If we are
> > > changing the hooks to operate on the opener cred, then presumably we
> > > should be doing that for sender_euid and replace the
> > > security_task_getsecid_obj() call with security_cred_getsecid()?
> >
> > Stephan, do you want that to be included in this patch? Or should I
> > follow this up with another patch for the sender_euid case?
>
> Either way is fine with me. Fixing sender_euid arguably is a fix that
> should go all the way back to the introduction of binder unless I
> misunderstand; it is independent of SELinux. Fixing the
> security_task_getsecid -> cred_secid only goes back to
> ec74136ded792deed80780a2f8baf3521eeb72f9. So having it as 3 separate
> patches may help with the different Fixes tags and back-porting
> purposes.
Yes, as annoying as it may be, please do separate patches as the
-stable and distro folks will have an easier time that way.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] binder: use euid from cred instead of using task
On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
>
> Set a transaction's sender_euid from the 'struct cred'
> saved at binder_open() instead of looking up the euid
> from the binder proc's 'struct task'. This ensures
> the euid is associated with the security context that
> of the task that opened binder.
>
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Signed-off-by: Todd Kjos
> Suggested-by: Stephen Smalley
> Cc: sta...@vger.kernel.org # 4.4+
> ---
> v3: added this patch to series
>
> drivers/android/binder.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This is an interesting ordering of the patches. Unless I'm missing
something I would have expected patch 3/3 to come first, followed by
2/3, with patch 1/3 at the end; basically the reverse of what was
posted here.
My reading of the previous thread was that Casey has made his peace
with these changes so unless anyone has any objections I'll plan on
merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
selinux/next.
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 989afd0804ca..26382e982c5e 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2711,7 +2711,7 @@ static void binder_transaction(struct binder_proc *proc,
> t->from = thread;
> else
> t->from = NULL;
> - t->sender_euid = task_euid(proc->tsk);
> + t->sender_euid = proc->cred->euid;
> t->to_proc = target_proc;
> t->to_thread = target_thread;
> t->code = tr->code;
> --
> 2.33.0.800.g4c38ced690-goog
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid
On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
>
> Use the 'struct cred' saved at binder_open() to lookup
> the security ID via security_cred_getsecid(). This
> ensures that the security context that opened binder
> is the one used to generate the secctx.
>
> Fixes: ec74136ded79 ("binder: create node flag to request sender's
> security context")
> Signed-off-by: Todd Kjos
> Suggested-by: Stephen Smalley
> Reported-by: kernel test robot
> Cc: sta...@vger.kernel.org # 5.4+
> ---
> v3: added this patch to series
> v4: fix build-break for !CONFIG_SECURITY
>
> drivers/android/binder.c | 11 +--
> include/linux/security.h | 4
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index ca599ebdea4a..989afd0804ca 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc
> *proc,
> u32 secid;
> size_t added_size;
>
> - /*
> -* Arguably this should be the task's subjective LSM secid but
> -* we can't reliably access the subjective creds of a task
> -* other than our own so we must use the objective creds,
> which
> -* are safe to access. The downside is that if a task is
> -* temporarily overriding it's creds it will not be reflected
> -* here; however, it isn't clear that binder would handle that
> -* case well anyway.
> -*/
> - security_task_getsecid_obj(proc->tsk, &secid);
> + security_cred_getsecid(proc->cred, &secid);
> ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 6344d3362df7..f02cc0211b10 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred
> *new,
> {
> }
>
> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +}
Since security_cred_getsecid() doesn't return an error code we should
probably set the secid to 0 in this case, for example:
static inline void security_cred_getsecid(...)
{
*secid = 0;
}
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] binder: use euid from cred instead of using task
On Fri, Oct 8, 2021 at 5:25 PM Casey Schaufler wrote:
>
> On 10/8/2021 2:12 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
> >> Set a transaction's sender_euid from the 'struct cred'
> >> saved at binder_open() instead of looking up the euid
> >> from the binder proc's 'struct task'. This ensures
> >> the euid is associated with the security context that
> >> of the task that opened binder.
> >>
> >> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> >> Signed-off-by: Todd Kjos
> >> Suggested-by: Stephen Smalley
> >> Cc: sta...@vger.kernel.org # 4.4+
> >> ---
> >> v3: added this patch to series
> >>
> >> drivers/android/binder.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > This is an interesting ordering of the patches. Unless I'm missing
> > something I would have expected patch 3/3 to come first, followed by
> > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > posted here.
> >
> > My reading of the previous thread was that Casey has made his peace
> > with these changes
>
> Yes. I will address the stacking concerns more directly.
> I am still somewhat baffled by the intent of the hook, the data
> passed to it, and the SELinux policy enforcement decisions, but
> that's beyond my scope.
Okay, I just wanted to make sure there were no objections.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] binder: use euid from cred instead of using task
On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos wrote:
>
> On Fri, Oct 8, 2021 at 2:12 PM Paul Moore wrote:
> >
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
> > >
> > > Set a transaction's sender_euid from the 'struct cred'
> > > saved at binder_open() instead of looking up the euid
> > > from the binder proc's 'struct task'. This ensures
> > > the euid is associated with the security context that
> > > of the task that opened binder.
> > >
> > > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > > Signed-off-by: Todd Kjos
> > > Suggested-by: Stephen Smalley
> > > Cc: sta...@vger.kernel.org # 4.4+
> > > ---
> > > v3: added this patch to series
> > >
> > > drivers/android/binder.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > This is an interesting ordering of the patches. Unless I'm missing
> > something I would have expected patch 3/3 to come first, followed by
> > 2/3, with patch 1/3 at the end; basically the reverse of what was
> > posted here.
>
> 2/3 and 3/3 both depend on 1/3 (add "cred" member of struct
> binder_proc). I kept that in 1/3 to keep that patch the same as what
> had already been reviewed. I didn't think much about the ordering
> between 2/3 and 3/3 -- but I agree that it would have been sensible to
> reverse their order.
>
> >
> > My reading of the previous thread was that Casey has made his peace
> > with these changes so unless anyone has any objections I'll plan on
> > merging 2/3 and 3/3 into selinux/stable-5.15 and merging 1/3 into
> > selinux/next.
>
> Thanks Paul. I'm not familiar with the branch structure, but you need
> 1/3 in selinux/stable-5.15 to resolve the dependency on proc->cred.
Yep, thanks. My eyes kinda skipped over that part when looking at the
patchset but that would have fallen out as soon as I merged them.
Unfortunately that pretty much defeats the purpose of splitting this
into three patches. While I suppose one could backport patches 2/3
and 3/3 individually, both of them have a very small footprint
especially considering their patch 1/3 dependency. At the very least
it looks like patch 2/3 needs to be respun to address the
!CONFIG_SECURITY case and seeing the split patches now I think the
smart thing is to just combine them into a single patch. I apologize
for the bad recommendation earlier, I should have followed that thread
a bit closer after the discussion with Casey and Stephen.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid
On Mon, Oct 11, 2021 at 5:59 PM Casey Schaufler wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
> >> Use the 'struct cred' saved at binder_open() to lookup
> >> the security ID via security_cred_getsecid(). This
> >> ensures that the security context that opened binder
> >> is the one used to generate the secctx.
> >>
> >> Fixes: ec74136ded79 ("binder: create node flag to request sender's
> >> security context")
> >> Signed-off-by: Todd Kjos
> >> Suggested-by: Stephen Smalley
> >> Reported-by: kernel test robot
> >> Cc: sta...@vger.kernel.org # 5.4+
> >> ---
> >> v3: added this patch to series
> >> v4: fix build-break for !CONFIG_SECURITY
> >>
> >> drivers/android/binder.c | 11 +--
> >> include/linux/security.h | 4
> >> 2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 100644
> >> --- a/drivers/android/binder.c
> >> +++ b/drivers/android/binder.c
> >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc
> >> *proc,
> >> u32 secid;
> >> size_t added_size;
> >>
> >> - /*
> >> -* Arguably this should be the task's subjective LSM secid
> >> but
> >> -* we can't reliably access the subjective creds of a task
> >> -* other than our own so we must use the objective creds,
> >> which
> >> -* are safe to access. The downside is that if a task is
> >> -* temporarily overriding it's creds it will not be
> >> reflected
> >> -* here; however, it isn't clear that binder would handle
> >> that
> >> -* case well anyway.
> >> -*/
> >> - security_task_getsecid_obj(proc->tsk, &secid);
> >> + security_cred_getsecid(proc->cred, &secid);
> >> ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
> >> if (ret) {
> >> return_error = BR_FAILED_REPLY;
> >> diff --git a/include/linux/security.h b/include/linux/security.h
> >> index 6344d3362df7..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct
> >> cred *new,
> >> {
> >> }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32
> >> *secid)
> >> +{
> >> +}
> >
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> > static inline void security_cred_getsecid(...)
> > {
> > *secid = 0;
> > }
>
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?
It follows a pattern with the other LSM hooks when !CONFIG_SECURITY,
and I'd much rather us keep things consistent.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid
On Tue, Oct 12, 2021 at 5:41 AM Dan Carpenter wrote:
>
> On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> > On 10/11/2021 2:33 PM, Paul Moore wrote:
> > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote:
> > >> Use the 'struct cred' saved at binder_open() to lookup
> > >> the security ID via security_cred_getsecid(). This
> > >> ensures that the security context that opened binder
> > >> is the one used to generate the secctx.
> > >>
> > >> Fixes: ec74136ded79 ("binder: create node flag to request sender's
> > >> security context")
> > >> Signed-off-by: Todd Kjos
> > >> Suggested-by: Stephen Smalley
> > >> Reported-by: kernel test robot
> > >> Cc: sta...@vger.kernel.org # 5.4+
> > >> ---
> > >> v3: added this patch to series
> > >> v4: fix build-break for !CONFIG_SECURITY
> > >>
> > >> drivers/android/binder.c | 11 +--
> > >> include/linux/security.h | 4
> > >> 2 files changed, 5 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > >> index ca599ebdea4a..989afd0804ca 100644
> > >> --- a/drivers/android/binder.c
> > >> +++ b/drivers/android/binder.c
> > >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc
> > >> *proc,
> > >> u32 secid;
> > >> size_t added_size;
> > >>
> > >> - /*
> > >> -* Arguably this should be the task's subjective LSM
> > >> secid but
> > >> -* we can't reliably access the subjective creds of a
> > >> task
> > >> -* other than our own so we must use the objective
> > >> creds, which
> > >> -* are safe to access. The downside is that if a task is
> > >> -* temporarily overriding it's creds it will not be
> > >> reflected
> > >> -* here; however, it isn't clear that binder would
> > >> handle that
> > >> -* case well anyway.
> > >> -*/
> > >> - security_task_getsecid_obj(proc->tsk, &secid);
> > >> + security_cred_getsecid(proc->cred, &secid);
> > >> ret = security_secid_to_secctx(secid, &secctx,
> > >> &secctx_sz);
> > >> if (ret) {
> > >> return_error = BR_FAILED_REPLY;
> > >> diff --git a/include/linux/security.h b/include/linux/security.h
> > >> index 6344d3362df7..f02cc0211b10 100644
> > >> --- a/include/linux/security.h
> > >> +++ b/include/linux/security.h
> > >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct
> > >> cred *new,
> > >> {
> > >> }
> > >>
> > >> +static inline void security_cred_getsecid(const struct cred *c, u32
> > >> *secid)
> > >> +{
> > >> +}
> > > Since security_cred_getsecid() doesn't return an error code we should
> > > probably set the secid to 0 in this case, for example:
> > >
> > > static inline void security_cred_getsecid(...)
> > > {
> > > *secid = 0;
> > > }
> >
> > If CONFIG_SECURITY is unset there shouldn't be any case where
> > the secid value is ever used for anything. Are you suggesting that
> > it be set out of an abundance of caution?
>
> The security_secid_to_secctx() function is probably inlined so probably
> KMSan will not warn about this. But Smatch will warn about passing
> unitialized variables. You probably wouldn't recieve and email about
> it, and I would just add an exception that security_cred_getsecid()
> should be ignored.
I'd much rather just see the secid set to zero in the !CONFIG_SECURITY case.
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 0/3] binder: use cred instead of task for security context
On Tue, Oct 12, 2021 at 12:56 PM Todd Kjos wrote: > > This series fixes the possible use of an incorrect security context > when checking selinux permissions, getting a security ID, or lookup > up the euid. > > The previous behavior was to save the group_leader 'struct task_struct' > in binder_open() and using that to obtain security IDs or euids. > > This has been shown to be unreliable, so this series instead saves the > 'struct cred' of the task that called binder_open(). This cred is used > for these lookups instead of the task. Hi Todd, I just merged all three patches into selinux/next, thanks for your help patience on this patchset. Ultimately I merged these patches into selinux/next as opposed to selinux/stable-5.15 because I felt that a couple of weeks in -next before going to Linus would be a good thing. I'm also not certain how widespread binder is outside of Android so I figured the practical difference between next and stable-5.15 is likely very small. Regardless, all of your Fixes and stable tags remain in the patches so as soon as they go up to Linus during the next merge window the stable folks will be notified. -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix test regression due to sender_euid change
On Fri, Nov 12, 2021 at 1:07 PM Todd Kjos wrote:
>
> This is a partial revert of commit
> 29bc22ac5e5b ("binder: use euid from cred instead of using task").
> Setting sender_euid using proc->cred caused some Android system test
> regressions that need further investigation. It is a partial
> reversion because subsequent patches rely on proc->cred.
>
> Cc: sta...@vger.kernel.org # 4.4+
> Fixes: 29bc22ac5e5b ("binder: use euid from cred instead of using task")
> Signed-off-by: Todd Kjos
> Change-Id: I9b1769a3510fed250bb21859ef8beebabe034c66
> ---
> - the issue was introduced in 5.16-rc1, so please apply to 5.16
> - this should apply cleanly to all stable branches back to 4.4
> that contain "binder: use euid from cred instead of using task"
>
>
> drivers/android/binder.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This looks okay to me. I assume this is going in via GregKH's tree?
Acked-by: Paul Moore
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 49fb74196d02..cffbe57a8e08 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2710,7 +2710,7 @@ static void binder_transaction(struct binder_proc *proc,
> t->from = thread;
> else
> t->from = NULL;
> - t->sender_euid = proc->cred->euid;
> + t->sender_euid = task_euid(proc->tsk);
> t->to_proc = target_proc;
> t->to_thread = target_thread;
> t->code = tr->code;
> --
> 2.34.0.rc1.387.gb447b232ab-goog
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
Hello all. When running the selinux-testsuite (link below) against v5.1-rc1 I hit the BUG_ON() at the top of binder_alloc_do_buffer_copy() (trace below). I'm hoping this is a known issue with a fix already in the works? * https://github.com/SELinuxProject/selinux-testsuite [ 823.232432] [ cut here ] [ 823.234746] kernel BUG at drivers/android/binder_alloc.c:1141! [ 823.237447] invalid opcode: [#1] SMP PTI [ 823.239421] CPU: 1 PID: 3644 Comm: test_binder Not tainted 5.1.0-0.rc1.git0.1.2.secnext.fc31.x86_64 #1 [ 823.243538] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011 [ 823.246079] RIP: 0010:binder_alloc_do_buffer_copy+0x34/0x210 [ 823.248613] Code: 0a 41 55 49 89 fb 41 54 41 89 f4 48 8d 77 38 48 8b 42 58 55 53 48 39 f1 0f 84 17 01 00 00 48 8b 49 58 48 29 c1 49 39 c9 76 02 <0f> 0b 4c 29 c9 49 39 ca 77 f6 41 f6 c2 03 75 f0 0f b6 4a 28 f6 c1 [ 823.256404] RSP: 0018:b04e41093b68 EFLAGS: 00010202 [ 823.258513] RAX: 7fb600c52000 RBX: a0d48e24a0213e28 RCX: 0020 [ 823.261375] RDX: 9c09b058a9c0 RSI: 9c09189165b0 RDI: 9c0918916578 [ 823.264225] RBP: 9c09b058a9c0 R08: b04e41093c80 R09: 0028 [ 823.267044] R10: a0d48e24a0213e28 R11: 9c0918916578 R12: [ 823.269758] R13: 9c09b67c9660 R14: 9c09b116fb40 R15: 8acd4d08 [ 823.272482] FS: 7fbeb3438800() GS:9c09b7a8() knlGS: [ 823.275595] CS: 0010 DS: ES: CR0: 80050033 [ 823.277676] CR2: 55b102d31cc9 CR3: 000234648000 CR4: 001406e0 [ 823.280347] Call Trace: [ 823.281287] binder_get_object+0x60/0xf0 [ 823.282728] binder_transaction+0xc2e/0x2370 [ 823.284268] ? __check_object_size+0x41/0x15d [ 823.285849] ? binder_thread_read+0x9e2/0x1460 [ 823.287342] ? binder_update_ref_for_handle+0x83/0x1a0 [ 823.289066] binder_thread_write+0x2ae/0xfc0 [ 823.290513] ? finish_wait+0x80/0x80 [ 823.291729] binder_ioctl+0x659/0x836 [ 823.292980] do_vfs_ioctl+0x40a/0x670 [ 823.294234] ksys_ioctl+0x5e/0x90 [ 823.295364] __x64_sys_ioctl+0x16/0x20 [ 823.296609] do_syscall_64+0x5b/0x150 [ 823.297796] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 823.299423] RIP: 0033:0x7fbeb35e782b [ 823.300580] Code: 0f 1e fa 48 8b 05 5d 96 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 2d 96 0c 00 f7 d8 64 89 01 48 [ 823.306473] RSP: 002b:7ffdfae2f198 EFLAGS: 0287 ORIG_RAX: 0010 [ 823.308868] RAX: ffda RBX: RCX: 7fbeb35e782b [ 823.311029] RDX: 7ffdfae2f1b0 RSI: c0306201 RDI: 0003 [ 823.313206] RBP: 7ffdfae30210 R08: 010fa330 R09: [ 823.315379] R10: 00400644 R11: 0287 R12: 00401190 [ 823.317459] R13: 7ffdfae304c0 R14: R15: [ 823.319510] Modules linked in: crypto_user nfnetlink xt_multiport bluetooth ecdh_generic rfkill sctp overlay ip6table_security xt_CONNSECMARK xt_SECMARK xt_state xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c iptable_security ah6 xfrm6_mode_transport ah4 xfrm4_mode_transport ip6table_mangle ip6table_filter ip6_tables iptable_mangle xt_mark xt_AUDIT ib_isert iscsi_target_mod ib_srpt target_core_mod ib_srp scsi_transport_srp rpcrdma rdma_ucm ib_iser ib_umad ib_ipoib rdma_cm iw_cm libiscsi scsi_transport_iscsi ib_cm mlx5_ib ib_uverbs ib_core sunrpc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel joydev virtio_balloon i2c_piix4 drm_kms_helper virtio_net net_failover failover ttm drm mlx5_core crc32c_intel virtio_blk ata_generic virtio_console mlxfw serio_raw pata_acpi qemu_fw_cfg [last unloaded: arp_tables] [ 823.339786] ---[ end trace 6f761f654b297775 ]--- -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Mon, Mar 18, 2019 at 6:51 PM Todd Kjos wrote: > On Mon, Mar 18, 2019 at 2:31 PM Paul Moore wrote: > > Hello all. > > > > When running the selinux-testsuite (link below) against v5.1-rc1 I hit > > the BUG_ON() at the top of binder_alloc_do_buffer_copy() (trace > > below). I'm hoping this is a known issue with a fix already in the > > works? > > > Sadly, this is the first report of this, so no fix in flight. I'll try > to get a fix up in the next few days. No problem, thanks for letting me know. If you need some testing help, let me know. -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Tue, Mar 19, 2019 at 12:51 PM Todd Kjos wrote: > Paul, > > I think this patch will fix it... can you run the selinux-testsuite > with the patch to verify? (the conditional assumed that size_t can go > negative) Building a test kernel now, I'll report back as soon as it is finished. Thanks. > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 9a7c431469b3..bb9a661ffecc 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2240,7 +2240,8 @@ static size_t binder_get_object(struct binder_proc > *proc, > size_t object_size = 0; > > read_size = min_t(size_t, sizeof(*object), buffer->data_size - > offset); > - if (read_size < sizeof(*hdr) || !IS_ALIGNED(offset, sizeof(u32))) > + if (offset > buffer->data_size || read_size < sizeof(*hdr) || > + !IS_ALIGNED(offset, sizeof(u32))) > return 0; > binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, > offset, read_size); > > On Mon, Mar 18, 2019 at 4:02 PM Paul Moore wrote: > > > > On Mon, Mar 18, 2019 at 6:51 PM Todd Kjos wrote: > > > On Mon, Mar 18, 2019 at 2:31 PM Paul Moore wrote: > > > > Hello all. > > > > > > > > When running the selinux-testsuite (link below) against v5.1-rc1 I hit > > > > the BUG_ON() at the top of binder_alloc_do_buffer_copy() (trace > > > > below). I'm hoping this is a known issue with a fix already in the > > > > works? > > > > > > > > > Sadly, this is the first report of this, so no fix in flight. I'll try > > > to get a fix up in the next few days. > > > > No problem, thanks for letting me know. If you need some testing > > help, let me know. > > > > -- > > paul moore > > www.paul-moore.com -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Tue, Mar 19, 2019 at 3:33 PM Paul Moore wrote: > On Tue, Mar 19, 2019 at 12:51 PM Todd Kjos wrote: > > Paul, > > > > I think this patch will fix it... can you run the selinux-testsuite > > with the patch to verify? (the conditional assumed that size_t can go > > negative) > > Building a test kernel now, I'll report back as soon as it is finished. Good news, the BUG_ON() panic is now gone, but I'm seeing a test failure, although to be fair I saw some binder test failures during the merge window (I generally don't worry about failures until -rc1 is released). The selinux-testsuite binder tests have been working since spring 2018, but it's possible there might be a bug in the tests that is just now showing up. Have you ever looked at the selinux-testsuite tests for binder? * https://github.com/SELinuxProject/selinux-testsuite/tree/master/tests/binder > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > index 9a7c431469b3..bb9a661ffecc 100644 > > --- a/drivers/android/binder.c > > +++ b/drivers/android/binder.c > > @@ -2240,7 +2240,8 @@ static size_t binder_get_object(struct binder_proc > > *proc, > > size_t object_size = 0; > > > > read_size = min_t(size_t, sizeof(*object), buffer->data_size - > > offset); > > - if (read_size < sizeof(*hdr) || !IS_ALIGNED(offset, sizeof(u32))) > > + if (offset > buffer->data_size || read_size < sizeof(*hdr) || > > + !IS_ALIGNED(offset, sizeof(u32))) > > return 0; > > binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, > > offset, read_size); > > > > On Mon, Mar 18, 2019 at 4:02 PM Paul Moore wrote: > > > > > > On Mon, Mar 18, 2019 at 6:51 PM Todd Kjos wrote: > > > > On Mon, Mar 18, 2019 at 2:31 PM Paul Moore wrote: > > > > > Hello all. > > > > > > > > > > When running the selinux-testsuite (link below) against v5.1-rc1 I hit > > > > > the BUG_ON() at the top of binder_alloc_do_buffer_copy() (trace > > > > > below). I'm hoping this is a known issue with a fix already in the > > > > > works? > > > > > > > > > > > > Sadly, this is the first report of this, so no fix in flight. I'll try > > > > to get a fix up in the next few days. > > > > > > No problem, thanks for letting me know. If you need some testing > > > help, let me know. -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Tue, Mar 19, 2019 at 6:16 PM Todd Kjos wrote: > On Tue, Mar 19, 2019 at 3:08 PM Paul Moore wrote: > > > > On Tue, Mar 19, 2019 at 3:33 PM Paul Moore wrote: > > > On Tue, Mar 19, 2019 at 12:51 PM Todd Kjos wrote: > > > > Paul, > > > > > > > > I think this patch will fix it... can you run the selinux-testsuite > > > > with the patch to verify? (the conditional assumed that size_t can go > > > > negative) > > > > > > Building a test kernel now, I'll report back as soon as it is finished. > > > > Good news, the BUG_ON() panic is now gone, > > Great. Thanks for testing it. Thanks for the fix :) > > but I'm seeing a test > > failure, although to be fair I saw some binder test failures during > > the merge window (I generally don't worry about failures until -rc1 is > > released). The selinux-testsuite binder tests have been working since > > spring 2018, but it's possible there might be a bug in the tests that > > is just now showing up. Have you ever looked at the selinux-testsuite > > tests for binder? > > No, I didn't know they existed until yesterday. Glad to have more test > coverage. Were they running clean on 5.0.0? Yep. They were added to the test suite last May and have been running clean since then. > Is there a public dashboard where I can take a look at those binder failures? Not really. I send test results to a not-yet-publicized mailing list, but there is more detail in the GitHub issue below (my last comment has the verbose test output): * https://github.com/SELinuxProject/selinux-kernel/issues/46 > > * > > https://github.com/SELinuxProject/selinux-testsuite/tree/master/tests/binder > > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > > > index 9a7c431469b3..bb9a661ffecc 100644 > > > > --- a/drivers/android/binder.c > > > > +++ b/drivers/android/binder.c > > > > @@ -2240,7 +2240,8 @@ static size_t binder_get_object(struct > > > > binder_proc *proc, > > > > size_t object_size = 0; > > > > > > > > read_size = min_t(size_t, sizeof(*object), buffer->data_size - > > > > offset); > > > > - if (read_size < sizeof(*hdr) || !IS_ALIGNED(offset, > > > > sizeof(u32))) > > > > + if (offset > buffer->data_size || read_size < sizeof(*hdr) || > > > > + !IS_ALIGNED(offset, sizeof(u32))) > > > > return 0; > > > > binder_alloc_copy_from_buffer(&proc->alloc, object, buffer, > > > > offset, read_size); > > > > > > > > On Mon, Mar 18, 2019 at 4:02 PM Paul Moore wrote: > > > > > > > > > > On Mon, Mar 18, 2019 at 6:51 PM Todd Kjos wrote: > > > > > > On Mon, Mar 18, 2019 at 2:31 PM Paul Moore > > > > > > wrote: > > > > > > > Hello all. > > > > > > > > > > > > > > When running the selinux-testsuite (link below) against v5.1-rc1 > > > > > > > I hit > > > > > > > the BUG_ON() at the top of binder_alloc_do_buffer_copy() (trace > > > > > > > below). I'm hoping this is a known issue with a fix already in > > > > > > > the > > > > > > > works? > > > > > > > > > > > > > > > > > > Sadly, this is the first report of this, so no fix in flight. I'll > > > > > > try > > > > > > to get a fix up in the next few days. > > > > > > > > > > No problem, thanks for letting me know. If you need some testing > > > > > help, let me know. > > > > -- > > paul moore > > www.paul-moore.com -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Tue, Mar 19, 2019 at 9:08 PM Todd Kjos wrote:
> Paul,
>
> Looking at a snippet of the test output:
>
> Service Provider read_consumed: 8
> Service Provider command: BR_NOOP
> Service Provider command: BR_FAILED_REPLY <-- the txn
> failed as expected.
> Manager read_consumed: 8
> Manager command: BR_NOOP
> Manager command: BR_TRANSACTION_COMPLETE
> not ok 3
> <- but for some reason didn't exit(103)
> # Failed test at ./test line 56.
>
> It looks like the test script expects test_binder to fail with
> exit(103) after processing the Server Provider commands. It's not
> clear why it didn't, since the return of a BR_FAILED_REPLY for that
> transaction should have executed this code (line 392 of
> test_binder.c):
>
> if (cmd == BR_FAILED_REPLY ||
> cmd == BR_DEAD_REPLY ||
> cmd == BR_DEAD_BINDER) {
> fprintf(stderr,
> "Failed response from Service Provider using Managers
> FD\n");
> exit(103);
> }
>
> Could this be an issue with the test? At least it doesn't look like a
> transaction is succeeding when it shouldn't.
Hi Todd,
Thanks for looking into this further. Look a bit more at the test, it
appears that the code above (line 392) only comes into play if the
service provider is handling a BR_REPLY, but looking at the test
output it doesn't appear that this is the case.
# runcon -t test_binder_provider_no_im_t ./test_binder -v -r 2 provider
Service Provider PID: 2095 Process context:
unconfined_u:unconfined_r:test_binder_provider_no_im_t:s0-s0:c0.c1023
Service Provider sending transaction to Manager - ADD_TEST_SERVICE
Service Provider read_consumed: 48
Service Provider command: BR_NOOP
Service Provider command: BR_INCREFS
Service Provider command: BR_ACQUIRE
Service Provider command: BR_TRANSACTION_COMPLETE
Service Provider read_consumed: 8
Service Provider command: BR_NOOP
Service Provider command: BR_FAILED_REPLY
However, things get weird. In the course of writing this email I
booted into my 5.0.0-1.1.secnext kernel (which passed the binder test
earlier) and now that kernel is failing in the same way (the test
hasn't changed). This test system is a Fedora Rawhide system which is
updated before I make a test run and I'm wondering if there is some
other userspace component which may be affecting this ... ? I've now
tried this on two different, current Rawhide VMs, hosted on two
different systems and I'm seeing the same thing, so I don't think it's
a *bad* system/VM?
> On Tue, Mar 19, 2019 at 5:15 PM Todd Kjos wrote:
> >
> > [...]
> >
> > > > Is there a public dashboard where I can take a look at those binder
> > > > failures?
> > >
> > > Not really. I send test results to a not-yet-publicized mailing list,
> > > but there is more detail in the GitHub issue below (my last comment
> > > has the verbose test output):
> > >
> > > * https://github.com/SELinuxProject/selinux-kernel/issues/46
> > >
> >
> > Ok, so it looks like something was introduced that causes binder to be
> > too permissive (test 3 transaction succeeded when failure is
> > expected). I don't know of any recent binder changes that could have
> > caused that.
> >
> > It will take me a while to set up this test environment. Is this easy
> > for you to run? Any chance of bisecting or at least trying a few
> > versions to narrow it down? Here's a list of the recent patchset -- it
> > would be useful to know which caused it (or if none of them did):
> >
> > 9e98c678c2d6a Linux 5.1-rc1
> > ...
> > 26528be6720bb binder: fix handling of misaligned binder object
> > bde4a19fc04f5 binder: use userspace pointer as base of buffer space
> > c41358a5f5217 binder: remove user_buffer_offset
> > db6b0b810bf94 binder: avoid kernel vm_area for buffer fixups
> > 7a67a39320dfb binder: add function to copy binder object from buffer
> > 8ced0c6231ead binder: add functions to copy to/from binder buffers
> > 1a7c3d9bb7a92 binder: create userspace-to-binder-buffer copy function
> > ...
> > 1c163f4c7b3f6 (tag: v5.0) Linux 5.0
> >
> > Thanks,
> >
> > -Todd
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Wed, Mar 20, 2019 at 11:54 AM Todd Kjos wrote: > So, then it sounds like the test is not running properly ... Yes, the test is almost surely broken to some extent, although the kernel hitting the BUG_ON() was clearly a bug too :) > Can I add a "Tested-by: Paul Moore " on my patch > submission to fix the BUG_ON (the exact patch you tested) ? Yep. Thanks for your help on fixing that. -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Wed, Mar 20, 2019 at 3:50 PM Todd Kjos wrote:
>
> Paul,
>
> Looking at main() in test_binder.c...
>
> int main(int argc, char **argv)
> {
>
> [...]
>
> // Line 493
> struct binder_write_read bwr;
> struct flat_binder_object obj;
> struct {
> uint32_t cmd;
> struct binder_transaction_data txn;
> } __attribute__((packed)) writebuf;
> unsigned int readbuf[32];
>
> [...]
> // Line 630
> writebuf.txn.data.ptr.buffer = (uintptr_t)&obj;
> writebuf.txn.data.ptr.offsets = (uintptr_t)&obj + // [A]
> sizeof(struct
> flat_binder_object);
>
> bwr.write_buffer = (uintptr_t)&writebuf;
> bwr.write_size = sizeof(writebuf);
>
> It looks like bwr.txn.data.ptr.offsets points off the end of obj (see
> [A] above), which means the binder driver will read compiler-dependent
> stack data as the offset for the object. If it happens to be 0, then
> the test will work (read the object from offset 0). If it's not 0,
> then most likely offset > data_size (which is what found that BUG_ON
> case). With my patch applied, this will just cause an error to be
> returned (what you are seeing now).
>
> Same thing when you test with v5.0 -- if the offset happens to be 0,
> then the test will succeed. If not, then the test will fail because
> the transaction fails in an unexpected way.
That might explain why the test used to work, but now fails - a
different compiler (I rebuild the test before each test run).
Keeping in mind I'm really quite ignorant when it comes to binder, how
would you suggest fixing the test?
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Wed, Mar 20, 2019 at 7:26 PM Todd Kjos wrote:
> I can send you a patch tomorrow (I won't be able to test it though).
I may not know much about binder, but I do know how to run the test suite :)
Thanks Todd.
> On Wed, Mar 20, 2019 at 4:23 PM Paul Moore wrote:
> >
> > On Wed, Mar 20, 2019 at 3:50 PM Todd Kjos wrote:
> > >
> > > Paul,
> > >
> > > Looking at main() in test_binder.c...
> > >
> > > int main(int argc, char **argv)
> > > {
> > >
> > > [...]
> > >
> > > // Line 493
> > > struct binder_write_read bwr;
> > > struct flat_binder_object obj;
> > > struct {
> > > uint32_t cmd;
> > > struct binder_transaction_data txn;
> > > } __attribute__((packed)) writebuf;
> > > unsigned int readbuf[32];
> > >
> > > [...]
> > > // Line 630
> > > writebuf.txn.data.ptr.buffer = (uintptr_t)&obj;
> > > writebuf.txn.data.ptr.offsets = (uintptr_t)&obj + // [A]
> > > sizeof(struct
> > > flat_binder_object);
> > >
> > > bwr.write_buffer = (uintptr_t)&writebuf;
> > > bwr.write_size = sizeof(writebuf);
> > >
> > > It looks like bwr.txn.data.ptr.offsets points off the end of obj (see
> > > [A] above), which means the binder driver will read compiler-dependent
> > > stack data as the offset for the object. If it happens to be 0, then
> > > the test will work (read the object from offset 0). If it's not 0,
> > > then most likely offset > data_size (which is what found that BUG_ON
> > > case). With my patch applied, this will just cause an error to be
> > > returned (what you are seeing now).
> > >
> > > Same thing when you test with v5.0 -- if the offset happens to be 0,
> > > then the test will succeed. If not, then the test will fail because
> > > the transaction fails in an unexpected way.
> >
> > That might explain why the test used to work, but now fails - a
> > different compiler (I rebuild the test before each test run).
> >
> > Keeping in mind I'm really quite ignorant when it comes to binder, how
> > would you suggest fixing the test?
> >
> > --
> > paul moore
> > www.paul-moore.com
--
paul moore
www.paul-moore.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: v5.1-rc1 binder_alloc_do_buffer_copy() BUG_ON triggered by selinux-testsuite
On Thu, Mar 21, 2019 at 11:48 AM Todd Kjos wrote: > On Thu, Mar 21, 2019 at 2:50 AM Ondrej Mosnacek wrote: > > > > On Thu, Mar 21, 2019 at 12:26 AM Todd Kjos wrote: > > > I can send you a patch tomorrow (I won't be able to test it though). > > > > So, I was a bit quicker than you and I think I managed to fix the test > > myself :) > > > > See: > > https://github.com/SELinuxProject/selinux-testsuite/pull/50/commits/b559c3f54eae6130cb9e79c295b0f94db26e09e4 > > Looks good. Thanks! I'm getting clean runs on my test system now too - thanks everyone! -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 06/12] audit: Use timespec64 to represent audit timestamps
On Fri, Apr 7, 2017 at 8:57 PM, Deepa Dinamani wrote:
> struct timespec is not y2038 safe.
> Audit timestamps are recorded in string format into
> an audit buffer for a given context.
> These mark the entry timestamps for the syscalls.
> Use y2038 safe struct timespec64 to represent the times.
> The log strings can handle this transition as strings can
> hold upto 1024 characters.
>
> Signed-off-by: Deepa Dinamani
> Reviewed-by: Arnd Bergmann
> Acked-by: Paul Moore
> Acked-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 4 ++--
> kernel/audit.c| 10 +-
> kernel/audit.h| 2 +-
> kernel/auditsc.c | 6 +++---
> 4 files changed, 11 insertions(+), 11 deletions(-)
I have no problem merging this patch into audit/next for v4.12, would
you prefer me to do that so at least this patch is merged?
It would probably make life a small bit easier for us in the audit
world too as it would reduce the potential merge conflict. However,
that's a relatively small thing to worry about.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 6fdfefc..f830508 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -332,7 +332,7 @@ static inline void audit_ptrace(struct task_struct *t)
> /* Private API (for audit.c only) */
> extern unsigned int audit_serial(void);
> extern int auditsc_get_stamp(struct audit_context *ctx,
> - struct timespec *t, unsigned int *serial);
> + struct timespec64 *t, unsigned int *serial);
> extern int audit_set_loginuid(kuid_t loginuid);
>
> static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
> @@ -511,7 +511,7 @@ static inline void __audit_seccomp(unsigned long syscall,
> long signr, int code)
> static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> { }
> static inline int auditsc_get_stamp(struct audit_context *ctx,
> - struct timespec *t, unsigned int *serial)
> + struct timespec64 *t, unsigned int *serial)
> {
> return 0;
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2f4964c..fcbf377 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1625,10 +1625,10 @@ unsigned int audit_serial(void)
> }
>
> static inline void audit_get_stamp(struct audit_context *ctx,
> - struct timespec *t, unsigned int *serial)
> + struct timespec64 *t, unsigned int *serial)
> {
> if (!ctx || !auditsc_get_stamp(ctx, t, serial)) {
> - *t = CURRENT_TIME;
> + ktime_get_real_ts64(t);
> *serial = audit_serial();
> }
> }
> @@ -1652,7 +1652,7 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> int type)
> {
> struct audit_buffer *ab;
> - struct timespec t;
> + struct timespec64 t;
> unsigned int uninitialized_var(serial);
>
> if (audit_initialized != AUDIT_INITIALIZED)
> @@ -1705,8 +1705,8 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> }
>
> audit_get_stamp(ab->ctx, &t, &serial);
> - audit_log_format(ab, "audit(%lu.%03lu:%u): ",
> -t.tv_sec, t.tv_nsec/100, serial);
> + audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> +(unsigned long long)t.tv_sec, t.tv_nsec/100,
> serial);
>
> return ab;
> }
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 0f1cf6d..cdf96f4 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -112,7 +112,7 @@ struct audit_context {
> enum audit_statestate, current_state;
> unsigned intserial; /* serial number for record */
> int major; /* syscall number */
> - struct timespec ctime; /* time of syscall entry */
> + struct timespec64 ctime; /* time of syscall entry */
> unsigned long argv[4];/* syscall arguments */
> longreturn_code;/* syscall return code */
> u64 prio;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e59ffc7..a2d9217 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1532,7 +1532,7 @@ void __audit_syscall_entry(int major, unsigned long a1,
> unsigned long a2,
> return;
>
> context->serial = 0;
> - context->ctime = CURRENT_TIME;
> + ktime_get_real_ts64(&
Re: [PATCH 06/12] audit: Use timespec64 to represent audit timestamps
On Sat, Apr 8, 2017 at 1:58 PM, Deepa Dinamani wrote: >> I have no problem merging this patch into audit/next for v4.12, would >> you prefer me to do that so at least this patch is merged? > > This would be fine. > But, I think whoever takes the last 2 deletion patches should also take them. > I'm not sure how that part works out. FWIW, I just merged this into the audit/next branch so at least this will go into v4.12. >> It would probably make life a small bit easier for us in the audit >> world too as it would reduce the potential merge conflict. However, >> that's a relatively small thing to worry about. -- paul moore www.paul-moore.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
