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 [off-list ref] wrote:

On Fri, Oct 8, 2021 at 2:12 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos [off-list ref] wrote:

quoted

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 <redacted>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: stable@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.

quoted

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