| Message ID | 164329905457.31174.9220154812163631144.stgit@olly (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: fix a type cast problem in cred_init_security() | expand |
On Thu, 27 Jan 2022 at 16:57, Paul Moore <paul@paul-moore.com> wrote: > > In the process of removing an explicit type cast to preserve a cred > const qualifier in cred_init_security() we ran into a problem where > the task_struct::real_cred field is defined with the "__rcu" > attribute but the selinux_cred() function parameter is not, leading > to a sparse warning: > > security/selinux/hooks.c:216:36: sparse: sparse: > incorrect type in argument 1 (different address spaces) > @@ expected struct cred const *cred > @@ got struct cred const [noderef] __rcu *real_cred > > As we don't want to add the "__rcu" attribute to the selinux_cred() > parameter, we're going to add an explicit cast back to > cred_init_security(). > > Fixes: b084e189b01a ("selinux: simplify cred_init_security") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index eae7dbd62df1..c057896e7dcd 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -213,7 +213,7 @@ static void cred_init_security(void) > { > struct task_security_struct *tsec; > > - tsec = selinux_cred(current->real_cred); > + tsec = selinux_cred((__force const struct cred *)current->real_cred); > tsec->osid = tsec->sid = SECINITSID_KERNEL; > } > Thanks for cleaning up. Just out of curiosity: the kernel doc[1] suggests using prepare_creds() + commit_creds() to update creds. Is is not required here because this is initialization code and the members osid and sid are only used by initialized SELinux? [1]: https://www.kernel.org/doc/html/v5.16/security/credentials.html#altering-credentials
On Thu, Jan 27, 2022 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote: > In the process of removing an explicit type cast to preserve a cred > const qualifier in cred_init_security() we ran into a problem where > the task_struct::real_cred field is defined with the "__rcu" > attribute but the selinux_cred() function parameter is not, leading > to a sparse warning: > > security/selinux/hooks.c:216:36: sparse: sparse: > incorrect type in argument 1 (different address spaces) > @@ expected struct cred const *cred > @@ got struct cred const [noderef] __rcu *real_cred > > As we don't want to add the "__rcu" attribute to the selinux_cred() > parameter, we're going to add an explicit cast back to > cred_init_security(). > > Fixes: b084e189b01a ("selinux: simplify cred_init_security") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index eae7dbd62df1..c057896e7dcd 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -213,7 +213,7 @@ static void cred_init_security(void) > { > struct task_security_struct *tsec; > > - tsec = selinux_cred(current->real_cred); > + tsec = selinux_cred((__force const struct cred *)current->real_cred); > tsec->osid = tsec->sid = SECINITSID_KERNEL; > } How about using unrcu_pointer() instead of the cast? It seems to do effectively the same while looking less ugly :)
On Thu, Jan 27, 2022 at 11:26 AM Christian Göttsche <cgzones@googlemail.com> wrote: > On Thu, 27 Jan 2022 at 16:57, Paul Moore <paul@paul-moore.com> wrote: > > > > In the process of removing an explicit type cast to preserve a cred > > const qualifier in cred_init_security() we ran into a problem where > > the task_struct::real_cred field is defined with the "__rcu" > > attribute but the selinux_cred() function parameter is not, leading > > to a sparse warning: > > > > security/selinux/hooks.c:216:36: sparse: sparse: > > incorrect type in argument 1 (different address spaces) > > @@ expected struct cred const *cred > > @@ got struct cred const [noderef] __rcu *real_cred > > > > As we don't want to add the "__rcu" attribute to the selinux_cred() > > parameter, we're going to add an explicit cast back to > > cred_init_security(). > > > > Fixes: b084e189b01a ("selinux: simplify cred_init_security") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/hooks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index eae7dbd62df1..c057896e7dcd 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -213,7 +213,7 @@ static void cred_init_security(void) > > { > > struct task_security_struct *tsec; > > > > - tsec = selinux_cred(current->real_cred); > > + tsec = selinux_cred((__force const struct cred *)current->real_cred); > > tsec->osid = tsec->sid = SECINITSID_KERNEL; > > } > > > > Thanks for cleaning up. > Just out of curiosity: the kernel doc[1] suggests using > prepare_creds() + commit_creds() to update creds. > Is is not required here because this is initialization code and the > members osid and sid are only used by initialized SELinux? Generally, yes, you are correct in that you would want to do the prepare/commit dance when altering credentials, but this is a special case as it is only used to set the credentials of the initial task when it is created during boot. At this point in the system's lifetime we really don't have to worry about the same things that we do when the system is up and running so it is safe (and easier!) to just modify the credential directly.
On Thu, Jan 27, 2022 at 12:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Thu, Jan 27, 2022 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote: > > In the process of removing an explicit type cast to preserve a cred > > const qualifier in cred_init_security() we ran into a problem where > > the task_struct::real_cred field is defined with the "__rcu" > > attribute but the selinux_cred() function parameter is not, leading > > to a sparse warning: > > > > security/selinux/hooks.c:216:36: sparse: sparse: > > incorrect type in argument 1 (different address spaces) > > @@ expected struct cred const *cred > > @@ got struct cred const [noderef] __rcu *real_cred > > > > As we don't want to add the "__rcu" attribute to the selinux_cred() > > parameter, we're going to add an explicit cast back to > > cred_init_security(). > > > > Fixes: b084e189b01a ("selinux: simplify cred_init_security") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/hooks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index eae7dbd62df1..c057896e7dcd 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -213,7 +213,7 @@ static void cred_init_security(void) > > { > > struct task_security_struct *tsec; > > > > - tsec = selinux_cred(current->real_cred); > > + tsec = selinux_cred((__force const struct cred *)current->real_cred); > > tsec->osid = tsec->sid = SECINITSID_KERNEL; > > } > > How about using unrcu_pointer() instead of the cast? It seems to do > effectively the same while looking less ugly :) Ah ha! I didn't know that function/macro existed :) I'm stuck in meetings most of the rest of today but I'll get out a v2 as soon as I can. Thanks.
On Thu, Jan 27, 2022 at 6:27 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jan 27, 2022 at 12:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Jan 27, 2022 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote: > > > In the process of removing an explicit type cast to preserve a cred > > > const qualifier in cred_init_security() we ran into a problem where > > > the task_struct::real_cred field is defined with the "__rcu" > > > attribute but the selinux_cred() function parameter is not, leading > > > to a sparse warning: > > > > > > security/selinux/hooks.c:216:36: sparse: sparse: > > > incorrect type in argument 1 (different address spaces) > > > @@ expected struct cred const *cred > > > @@ got struct cred const [noderef] __rcu *real_cred > > > > > > As we don't want to add the "__rcu" attribute to the selinux_cred() > > > parameter, we're going to add an explicit cast back to > > > cred_init_security(). > > > > > > Fixes: b084e189b01a ("selinux: simplify cred_init_security") > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > security/selinux/hooks.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index eae7dbd62df1..c057896e7dcd 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -213,7 +213,7 @@ static void cred_init_security(void) > > > { > > > struct task_security_struct *tsec; > > > > > > - tsec = selinux_cred(current->real_cred); > > > + tsec = selinux_cred((__force const struct cred *)current->real_cred); > > > tsec->osid = tsec->sid = SECINITSID_KERNEL; > > > } > > > > How about using unrcu_pointer() instead of the cast? It seems to do > > effectively the same while looking less ugly :) > > Ah ha! I didn't know that function/macro existed :) Full disclosure: I also discovered it today :) First I thought of rcu_dereference_raw(), but I decided to skim through <linux/rcupdate.h> looking for a better fit (rcu_dereference_raw() also does READ_ONCE(), which is not really necessary in this case) and found this one.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index eae7dbd62df1..c057896e7dcd 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -213,7 +213,7 @@ static void cred_init_security(void) { struct task_security_struct *tsec; - tsec = selinux_cred(current->real_cred); + tsec = selinux_cred((__force const struct cred *)current->real_cred); tsec->osid = tsec->sid = SECINITSID_KERNEL; }
In the process of removing an explicit type cast to preserve a cred const qualifier in cred_init_security() we ran into a problem where the task_struct::real_cred field is defined with the "__rcu" attribute but the selinux_cred() function parameter is not, leading to a sparse warning: security/selinux/hooks.c:216:36: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct cred const *cred @@ got struct cred const [noderef] __rcu *real_cred As we don't want to add the "__rcu" attribute to the selinux_cred() parameter, we're going to add an explicit cast back to cred_init_security(). Fixes: b084e189b01a ("selinux: simplify cred_init_security") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/hooks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)