Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Al Viro <viro@zeniv.linux.org.uk>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        linux-security-module@vger.kernel.org,
        Paul Moore <paul@paul-moore.com>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>, selinux@vger.kernel.org,
        Casey Schaufler <casey@schaufler-ca.com>,
        Eric Biederman <ebiederm@xmission.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU
In-Reply-To: <20210105195937.GX3579531@ZenIV.linux.org.uk>
References: <20210104232123.31378-1-stephen.s.brennan@oracle.com>
 <20210105055935.GT3579531@ZenIV.linux.org.uk>
 <20210105165005.GV3579531@ZenIV.linux.org.uk>
 <20210105195937.GX3579531@ZenIV.linux.org.uk>
Date:   Tue, 05 Jan 2021 15:25:11 -0800
Message-ID: <87a6tnge5k.fsf@stepbren-lnx.us.oracle.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Al Viro <viro@zeniv.linux.org.uk> writes:

> On Tue, Jan 05, 2021 at 04:50:05PM +0000, Al Viro wrote:
>
>> LSM_AUDIT_DATA_DENTRY is easy to handle - wrap
>>                 audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
>> into grabbing/dropping a->u.dentry->d_lock and we are done.
>
> 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()
      |       avc_audit() // note that has_pid_permissions() didn't get a
      |                   // flags field to propagate, so flags will not
      |                   // contain MAY_NOT_BLOCK
      |         slow_avc_audit()
      |           common_lsm_audit()
      |             dump_common_audit_data()
      task_unlock()

I understand the issue of d_name.name being freed across a preemption is
more general than proc_pid_permission() (as other callers may have
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?