[PATCH] selinux: fix a type cast problem in cred_init_security()

[PATCH] selinux: fix a type cast problem in cred_init_security()

[Paul Moore]

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;
 }
 

Re: [PATCH] selinux: fix a type cast problem in cred_init_security()

[Christian Göttsche]

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

Re: [PATCH] selinux: fix a type cast problem in cred_init_security()

[Ondrej Mosnacek]

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 :)

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH] selinux: fix a type cast problem in cred_init_security()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH] selinux: fix a type cast problem in cred_init_security()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH] selinux: fix a type cast problem in cred_init_security()

[Ondrej Mosnacek]

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.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.