linux-kernel - Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Jan 2021 00:38:03 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Stephen Brennan <stephen.s.brennan@...cle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
        Casey Schaufler <casey@...aufler-ca.com>,
        Eric Biederman <ebiederm@...ssion.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU

On Tue, Jan 05, 2021 at 07:00:59PM -0500, Paul Moore wrote:

> > > Incidentally, LSM_AUDIT_DATA_DENTRY in mainline is *not* safe wrt
> > > rename() - for long-named dentries it is possible to get preempted
> > > in the middle of
> > >                 audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
> > > and have the bugger renamed, with old name ending up freed.  The
> > > same goes for LSM_AUDIT_DATA_INODE...
> >
> > In the case of proc_pid_permission(), this preemption doesn't seem
> > possible. We have task_lock() (a spinlock) held by ptrace_may_access()
> > during this call, so preemption should be disabled:
> >
> > proc_pid_permission()
> >   has_pid_permissions()
> >     ptrace_may_access()
> >       task_lock()
> >       __ptrace_may_access()
> >       | security_ptrace_access_check()
> >       |   ptrace_access_check -> selinux_ptrace_access_check()
> >       |     avc_has_perm()

		... which does not hit either LSM_AUDIT_DATA_DENTRY nor
LSM_AUDIT_DATA_INODE.  It's really an unrelated issue.

> > preemption enabled). However, it seems like there's another issue here.
> > avc_audit() seems to imply that slow_avc_audit() would sleep:
> >
> > static inline int avc_audit(struct selinux_state *state,
> >                             u32 ssid, u32 tsid,
> >                             u16 tclass, u32 requested,
> >                             struct av_decision *avd,
> >                             int result,
> >                             struct common_audit_data *a,
> >                             int flags)
> > {
> >         u32 audited, denied;
> >         audited = avc_audit_required(requested, avd, result, 0, &denied);
> >         if (likely(!audited))
> >                 return 0;
> >         /* fall back to ref-walk if we have to generate audit */
> >         if (flags & MAY_NOT_BLOCK)
> >                 return -ECHILD;
> >         return slow_avc_audit(state, ssid, tsid, tclass,
> >                               requested, audited, denied, result,
> >                               a);
> > }
> >
> > If there are other cases in here where we might sleep, it would be a
> > problem to sleep with the task lock held, correct?

It can sleep - with LSM_AUDIT_DATA_INODE, which is precisely what
selinux_inode_permission() is hitting.

> I would expect the problem here to be the currently allocated audit
> buffer isn't large enough to hold the full audit record, in which case
> it will attempt to expand the buffer by a call to pskb_expand_head() -
> don't ask why audit buffers are skbs, it's awful - using a gfp flag
> that was established when the buffer was first created.  In this
> particular case it is GFP_ATOMIC|__GFP_NOWARN, which I believe should
> be safe in that it will not sleep on an allocation miss.
> 
> I need to go deal with dinner, so I can't trace the entire path at the
> moment, but I believe the potential audit buffer allocation is the
> main issue.

Nope.  dput() in dump_common_audit_data(), OTOH, is certainly not
safe.  OTTH, it's not really needed there - see vfs.git #work.audit
for (untested) turning that sucker non-blocking.  I hadn't tried
a followup that would get rid of the entire AVC_NONBLOCKING thing yet,
but I suspect that it should simplify the things in there nicely...