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] [day] [month] [year] [list]

Date:   Thu, 14 Jan 2021 14:51:17 -0800
From:   Stephen Brennan <stephen.s.brennan@...cle.com>
To:     Al Viro <viro@...iv.linux.org.uk>, Paul Moore <paul@...l-moore.com>
Cc:     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

Al Viro <viro@...iv.linux.org.uk> writes:
> 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...

I went ahead and pulled down this branch and combined it with my
pid_revalidate change. Further, I audited all the inode get_link and
permission() implementations, as well as dentry d_revalidate()
implementations, in fs/proc (more on that below). Together, all these
patches have run stable under a steady high load of concurrent PS
processes on a 104CPU machine for over an hour, and greatly reduced the
%sys utilization which the patch originally addressed. How would you
like to proceed with the #work.audit changes? I could include them in a
v5 of this patch series.

Regarding my audit (ha) of dentry and inode functions in the fs/proc/
directory:

* get_link() receives a NULL dentry pointer when called in RCU mode.
* permission() receives MAY_NOT_BLOCK in the mode parameter when called
  from RCU.
* d_revalidate() receives LOOKUP_RCU in flags.

There were generally three groups I found. Group (1) are functions which
contain a check at the top of the function and return -ECHILD, and so
appear to be trivially RCU safe (although this is by dropping out of RCU
completely). Group (2) are functions which have no explicit check, but
on my audit, I was confident that there were no sleeping function calls,
and thus were RCU safe as is. Group (3) are functions which appeared to
be unsafe for some reason or another.

Group (1):
 proc_ns_get_link()
 proc_pid_get_link()
 map_files_d_revalidate()
 proc_misc_d_revalidate()
 tid_fd_revalidate()

Group (2):
 proc_get_link()
 proc_self_get_link()
 proc_thread_self_get_link()
 proc_fd_permission()

Group (3):
 pid_revalidate()            -- addressed by my patch
 proc_map_files_get_link()
 proc_pid_permission()       -- addressed by Al's work.audit branch

proc_map_files_get_link() calls capable() which ends up calling a
security hook, which can get into the audit guts, and so I marked it as
potentially unsafe, and added a patch to bail out of this function
before the capable() check. However, I doubt this is really necessary.

So to conclude, depending on how Al wants to move forward with the
work.audit branch, I could send a full series with the proposed changes.

Stephen