Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU

[Stephen Brennan]

Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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?