On Tue, Aug 25, 2020 at 12:45 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Tue, Aug 25, 2020 at 5:01 PM Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> > On Tue, Aug 25, 2020 at 3:00 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > >
> > > There seems to be no reason to use GFP_ATOMIC in these cases.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > ---
> > > security/selinux/hooks.c | 6 +++---
> > > security/selinux/ss/policydb.c | 10 +++++-----
> > > security/selinux/ss/services.c | 4 ++--
> > > 3 files changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 89d3753b7bd5d..4de962daffbde 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what)
> > >
> > > if (WARN(invalid_reason, "Invalid lockdown reason")) {
> > > audit_log(audit_context(),
> > > - GFP_ATOMIC, AUDIT_SELINUX_ERR,
> > > + GFP_KERNEL, AUDIT_SELINUX_ERR,
> > > "lockdown_reason=invalid");
> > > return -EINVAL;
> > > }
> >
> > Have you audited all callers of security_locked_down() to ensure that
> > they are never holding any locks around the call? That's the only one
> > I saw that might be a problem now or in the future.
>
> Hm... didn't realize there were so many :) I'll try to go over them.
>
> As a side note, it would be nice if we had the may (not) sleep
> requirements documented somehow for each hook at the LSM level,
> ideally with a might_sleep() check at the beginning of each
> secuity_*() function that is expected to be called only from a
> non-atomic context... Some hooks already have arguments that specify
> this (inode_permission, inode_follow_link), but for others it is just
> implicit. I'll put this on my long-term TODO list...
Generally the core SELinux permission checking code assumes it can be
called from any context (including hardirq) and under any locking
conditions, and hooks that are for permission checking (not security
blob allocation/management) do the same. This allows permission
checks to be performed while locks are held by the caller. and from
arbitrary contexts I'd be inclined to just leave selinux_lockdown
unchanged given that it might be called anywhere much like capable().
