| Message ID | 20211007004629.1113572-4-tkjos@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | binder: use cred instead of task for security context | expand |
On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > 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. 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
On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > > 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. > > 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. > > > 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
On 10/8/2021 2:12 PM, Paul Moore wrote: > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> >> 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. > > 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. > 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
On Fri, Oct 8, 2021 at 5:25 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 10/8/2021 2:12 PM, Paul Moore wrote: > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > >> 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. > > > > 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.
On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote: > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > > > 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. > > > > > 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.
On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote: > > > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > > > > 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. > > > > > > > > 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. I'm happy to submit a single patch for all of this. Another part of the rationale for splitting it into 3 patches was correctly identify the patch that introduced the patch that introduced the issue -- so each of the 3 had a different "Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes" tag and perhaps mention the other two in the commit message? -Todd > > -- > paul moore > www.paul-moore.com
On Mon, Oct 11, 2021 at 7:39 PM Todd Kjos <tkjos@google.com> wrote: > > On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote: > > > > > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > > > > > 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. > > > > > > > > > > > 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. > > I'm happy to submit a single patch for all of this. Another part of > the rationale > for splitting it into 3 patches was correctly identify the patch that introduced > the patch that introduced the issue -- so each of the 3 had a different > "Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes" > tag and perhaps mention the other two in the commit message? Couldn't you just split patch 1 into the "add cred to binder proc" part and "use cred in LSM/SELinux hooks" part, combine patch 3 with the "add cred to binder proc" part to create new patch 1, then "use cred in LSM/SELinux hooks" part is patch 2, and "switch task_getsecid to cred_getsecid" to patch 3? Then patch 1 can be cherry-picked/ported all the way back to the introduction of binder, patch 2 all the way back to the introduction of binder LSM/SELinux hooks, and patch 3 just back to where passing the secctx across binder was introduced.
On Tue, Oct 12, 2021 at 5:24 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Mon, Oct 11, 2021 at 7:39 PM Todd Kjos <tkjos@google.com> wrote: > > > > On Mon, Oct 11, 2021 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Fri, Oct 8, 2021 at 5:24 PM Todd Kjos <tkjos@google.com> wrote: > > > > > > > > On Fri, Oct 8, 2021 at 2:12 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > > > > > > 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. > > > > > > > > > > > > > > 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. > > > > I'm happy to submit a single patch for all of this. Another part of > > the rationale > > for splitting it into 3 patches was correctly identify the patch that introduced > > the patch that introduced the issue -- so each of the 3 had a different > > "Fixes:" tag. Should I cite the oldest (binder introduction) with the "Fixes" > > tag and perhaps mention the other two in the commit message? > > Couldn't you just split patch 1 into the "add cred to binder proc" > part and "use cred in LSM/SELinux hooks" part, combine patch 3 with > the "add cred to binder proc" part to create new patch 1, then "use > cred in LSM/SELinux hooks" part is patch 2, and "switch task_getsecid > to cred_getsecid" to patch 3? Then patch 1 can be cherry-picked/ported > all the way back to the introduction of binder, patch 2 all the way > back to the introduction of binder LSM/SELinux hooks, and patch 3 just > back to where passing the secctx across binder was introduced. Sending a v5 with this refactoring and the !CONFIG_SECURITY fix
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;
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 <tkjos@google.com> 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(-)