selinux_file_permission() on pipes/pseudo-files - performance issue

[Stephen Smalley]

On Tue, Oct 27, 2020 at 11:02 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
>
> Hello,
>
> It has been reported to me that read/write syscalls on a pipe created
> via the pipe(2) family of syscalls spend a large percentage of time in
> avc_lookup() via selinux_file_permission(). This is specific to pipes
> (and also accept(2)'d sockets, I think) because these pipe fds are
> created using alloc_file_pseudo() as opposed to do_dentry_open(),
> which means that the security_file_open() hook is never called on
> them.
>
> In SELinux, this means that in selinux_file_permission() the
> read/write permission is always revalidated, leading to suboptimal
> performance, because SELinux re-checks the read/write perms of an open
> file only if the subject/target label or policy is different from when
> these permissions were checked during selinux_file_open().
>
> So I decided to try and see what would happen if I add a
> security_file_open() call to alloc_file(). This worked well for pipes
> - all domains that call pipe(2) seem to already have the necessary
> permissions to pass the open check, at least in Fedora policy - but I
> got lots of denials from accept(2), which also calls alloc_file()
> under the hood to create the new socket fd. The problem there is that
> programs usually call only recvmsg(2)/sendmsg(2) on fds returned by
> accept(2), thereby avoiding read/write checks on sock_file, which
> means that the domains don't normally have such permissions. Only
> programs that open actual socket files on the filesystem would
> unavoidably need read/write (or so I think), yet they wouldn't need
> them for the subsequent recvmsg(2)/sendmsg(2) calls.
>
> So I'm wondering if anyone has any idea how this could be fixed
> (optimized) without introducing regressions or awkward exceptions in
> the code. At this point, I don't see any way to do it regression-free
> without either adding a new hook or changing security_file_open() to
> distinguish between do_dentry_open() and alloc_file() + calling it
> from both places...

I don't think you want to reuse security_file_open() here.
There is an existing security_file_alloc() hook called on
__alloc_file(), but we have no inode information there so it cannot
cache the inode SID or perform any check on it.  You could potentially
lift that hook to all of the callers after the file has been
populated, or introduce a new hook in alloc_file() as you said.
Wondering though why this has never come up before if it is a
significant overhead in real workloads.  Earlier concerns about the
overhead on open files led to commit
788e7dd4c22e6f41b3a118fd8c291f831f6fddbb which introduced what later
became security_file_open().  But seemingly pipes/sockets weren't a
concern there.