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]

Message-ID: <87a6tnge5k.fsf@stepbren-lnx.us.oracle.com>
Date:   Tue, 05 Jan 2021 15:25:11 -0800
From:   Stephen Brennan <stephen.s.brennan@...cle.com>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org,
        Paul Moore <paul@...l-moore.com>,
        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

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