Re: [PATCH v2] selinux: various sparse fixes

[Ondrej Mosnacek <omosnace@xxxxxxxxxx>]

On Thu, Jan 27, 2022 at 9:04 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Thu, Jan 27, 2022 at 2:03 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 27, 2022 at 7:22 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > When running the SELinux code through sparse, there are a handful of
> > > warnings.  This patch resolves some of these warnings caused by
> > > "__rcu" mismatches.
> > >
> > >  % make W=1 C=1 security/selinux/
> > >
> > > Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> > > ---
> > >  security/selinux/hooks.c   |    6 +++---
> > >  security/selinux/ibpkey.c  |    2 +-
> > >  security/selinux/netnode.c |    5 +++--
> > >  security/selinux/netport.c |    2 +-
> > >  4 files changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 221e642025f5..0e857f86f5a7 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2534,7 +2534,7 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > >         if (rc) {
> > >                 clear_itimer();
> > >
> > > -               spin_lock_irq(&current->sighand->siglock);
> > > +               spin_lock_irq(unrcu_pointer(&current->sighand->siglock));
> > >                 if (!fatal_signal_pending(current)) {
> > >                         flush_sigqueue(&current->pending);
> > >                         flush_sigqueue(&current->signal->shared_pending);
> > > @@ -2542,13 +2542,13 @@ static void selinux_bprm_committed_creds(struct linux_binprm *bprm)
> > >                         sigemptyset(&current->blocked);
> > >                         recalc_sigpending();
> > >                 }
> > > -               spin_unlock_irq(&current->sighand->siglock);
> > > +               spin_unlock_irq(unrcu_pointer(&current->sighand->siglock));
> >
> > Shouldn't this be:
> >
> > spin_[un]lock_irq(&unrcu_pointer(current->sighand)->siglock);
>
> Maybe.
>
> The __rcu space annotation is definitely on task_struct::sighand, but
> my (quick) look at unrcu_pointer() was that the the de-rcu'ification
> applies to all of the dereferencing that is passed as the macro
> argument.  Because of that I decided to pass the entire dereferencing
> chain to the unrcu_pointer() macro just in case.  If that way of
> thinking is incorrect please let me know, otherwise I would rather
> just leave it as it is in v2.

While it does work this way, too, it just doesn't feel right. IMHO
it's more self-documenting to mark the exact pointer for which we are
applying the RCU access exception.

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