On Fri, Dec 7, 2018 at 8:04 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 12/6/18 6:57 PM, Paul Moore wrote:
> > On Tue, Dec 4, 2018 at 3:39 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> >> commit 0dc1ba24f7fff6 ("SELINUX: Make selinux cache VFS RCU walks safe")
> >> results in no audit messages at all if in permissive mode because the
> >> cache is updated during the rcu walk and thus no denial occurs on
> >> the subsequent ref walk. Fix this by not updating the cache when
> >> performing a non-blocking permission check. This only affects search
> >> and symlink read checks during rcu walk.
> >>
> >> Fixes: 0dc1ba24f7fff6 ("SELINUX: Make selinux cache VFS RCU walks safe")
> >> Reported-by: BMK <bmktuwien@xxxxxxxxx>
> >> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> >> ---
> >> security/selinux/avc.c | 9 ++++++---
> >> security/selinux/hooks.c | 4 +++-
> >> security/selinux/include/avc.h | 1 +
> >> 3 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> >> index 635e5c1e3e48..f0e7bc0dc442 100644
> >> --- a/security/selinux/avc.c
> >> +++ b/security/selinux/avc.c
> >> @@ -1021,8 +1021,10 @@ static noinline int avc_denied(struct selinux_state *state,
> >> !(avd->flags & AVD_FLAGS_PERMISSIVE))
> >> return -EACCES;
> >>
> >> - avc_update_node(state->avc, AVC_CALLBACK_GRANT, requested, driver,
> >> - xperm, ssid, tsid, tclass, avd->seqno, NULL, flags);
> >> + if (!(flags & AVC_NONBLOCKING))
> >> + avc_update_node(state->avc, AVC_CALLBACK_GRANT, requested,
> >> + driver, xperm, ssid, tsid, tclass, avd->seqno,
> >> + NULL, flags);
> >
> > I realize the issue here is specific to the VFS code paths, but if we
> > are going to introduce a general AVC flag to tell the AVC code not to
> > block, it seems like we should honor it everywhere. I would suggest
> > moving the check into avc_update_node() itself (it's static, so
> > hopefully any perf impact will be negligible).
>
> Ok, I'll do that. Just to clarify though, avc_update_node() does not
> block, nor does any of avc_has_perm_noaudit(). The reason we have to
> skip avc_update_node() in the non-blocking aka rcu-walk code path is not
> because it would block but because it would grant the denied permissions
> in the AVC entry and then avc_audit() would subsequently bail with
> -ECHILD since it would block on calling the audit system, and when we
> retry on the ref-walk code path, the cache entry will already contain
> the permissions and nothing will be denied or audited. So it is a tad
> confusing. I'll add a comment as well.
Yeah, it took me a little while to realize that, a comment in the code
would definitely be welcome. I'm also open to a better spot to put
the AVC_NONBLOCKING check; I suggested moving it inside
avc_update_node() simply to catch any other (future) callers which may
need to use it, but I recognize that isn't a great fit either. As you
point out, ultimately the problem is that we don't want to skip the
audit record, and since the audit queue might block, we have to play
it safe and not update the AVC.
--
paul moore
www.paul-moore.com
