[GIT PULL] SELinux fixes for v5.16 (#3)

[GIT PULL] SELinux fixes for v5.16 (#3)

[Paul Moore]

Hi Linus,

Another small SELinux fix for v5.16 to ensure that we don't block on
memory allocations while holding a spinlock.  This passes all our
tests without problem, please merge this for the next v5.16-rcX
release.

Thanks,
-Paul

--
The following changes since commit dc27f3c5d10c58069672215787a96b4fae01818b:

 selinux: fix NULL-pointer dereference when hashtab allocation fails
   (2021-11-19 16:11:39 -0500)

are available in the Git repository at:

 git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
   tags/selinux-pr-20211217

for you to fetch changes up to cc274ae7763d9700a56659f3228641d7069e7a3f:

 selinux: fix sleeping function called from invalid context
   (2021-12-16 17:47:39 -0500)

----------------------------------------------------------------
selinux/stable-5.16 PR 20211217

----------------------------------------------------------------
Scott Mayhew (1):
     selinux: fix sleeping function called from invalid context

security/selinux/hooks.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] SELinux fixes for v5.16 (#3)

[Linus Torvalds]

On Fri, Dec 17, 2021 at 12:02 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Another small SELinux fix for v5.16 to ensure that we don't block on
> memory allocations while holding a spinlock.  This passes all our
> tests without problem, please merge this for the next v5.16-rcX
> release.

Ugh, pulled.

GFP_NOWAIT can very easily fail, so I'm not convinced your tests would
catch any of the interesting cases.

There is only one single caller of the new
security_sb_mnt_opts_compat() callback, and I get the feeling that
maybe we could parse the options first - into a temporary new
superblock, and then at "test" time (when we're under that sb_lock) it
could compare that temporary sb with pre-existing ones?

That would also avoid the need for doing that mount option parsing
over and over and over again for each sb on the 'fs_supers' lists.

I've pulled this, bit it does smell bad to me, and I think that
original commit 69c4a42d72eb ("lsm,selinux: add new hook to compare
new mount to an existing mount") and ec1ade6a0448 ("nfs: account for
selinux security context when deciding to share superblock") may not
have been fully thought out.

It may have *looked* like just adding that check  to
'nfs_compare_super' was a simple and good idea, but it really doesn't
look great.

Adding a few more people to the cc.

                 Linus

Re: [GIT PULL] SELinux fixes for v5.16 (#3)

[Olga Kornievskaia]

On Fri, Dec 17, 2021 at 3:29 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Dec 17, 2021 at 12:02 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Another small SELinux fix for v5.16 to ensure that we don't block on
> > memory allocations while holding a spinlock.  This passes all our
> > tests without problem, please merge this for the next v5.16-rcX
> > release.
>
> Ugh, pulled.
>
> GFP_NOWAIT can very easily fail, so I'm not convinced your tests would
> catch any of the interesting cases.
>
> There is only one single caller of the new
> security_sb_mnt_opts_compat() callback, and I get the feeling that
> maybe we could parse the options first - into a temporary new
> superblock, and then at "test" time (when we're under that sb_lock) it
> could compare that temporary sb with pre-existing ones?
>
> That would also avoid the need for doing that mount option parsing
> over and over and over again for each sb on the 'fs_supers' lists.
>
> I've pulled this, bit it does smell bad to me, and I think that
> original commit 69c4a42d72eb ("lsm,selinux: add new hook to compare
> new mount to an existing mount") and ec1ade6a0448 ("nfs: account for
> selinux security context when deciding to share superblock") may not
> have been fully thought out.

Can you please elaborate on what is problematic with the two patches
you've highlighted. NFS needs a way to determine if the security mount
options have changed between the two mounts in order to determine if
superblock can be shared.

> It may have *looked* like just adding that check  to
> 'nfs_compare_super' was a simple and good idea, but it really doesn't
> look great.
>
> Adding a few more people to the cc.
>
>                  Linus

Re: [GIT PULL] SELinux fixes for v5.16 (#3)

[Linus Torvalds]

On Fri, Dec 17, 2021 at 1:08 PM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> Can you please elaborate on what is problematic with the two patches
> you've highlighted.

Commit ec1ade6a0448 ("nfs: account for selinux security context when
deciding to share superblock") adds the call to
security_sb_mnt_opts_compat() from the nfs_compare_mount_options()
function.

But nfs_compare_mount_options() is called from nfs_compare_super(),
which is used as the the "test" callback function for the "sget_fc()"
call:

        s = sget_fc(fc, compare_super, nfs_set_super);

and sget_fc() traverses all the mounted filesystems of this type -
while holding the superblock lock that protects that list.

So nfs_compare_super() may not sleep. It's called while holding a very
core low-level lock, and it really is supposed to only do a "test".
Not some complex operation that may do dynamic memory allocations and
sleep.

Yet that is exactly what security_sb_mnt_opts_compat() does, as done
in 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an
existing mount").

So those two patches are completely broken.

Now, commit cc274ae7763d ("selinux: fix sleeping function called from
invalid context") that I just merged "fixes" this by making the
allocations in parse_sid() be GFP_NOWAIT.

That is a *HORRIBLE* fix. It's a horrible fix because

 (a) GFP_NOWAIT can fail very easily, causing the mount to randomly
fail for non-obvious reasons.

 (b) even when it doesn't fail, you really shouldn't do things like
this under a very core spinlock.

Also, the original place - nfs_compare_mount_options() is called over
and over for each mount, so you're parsing those same mount options
over and over again. So not only was this sequence buggy, it's really
not very smart to begin with.

That's why I say that a much better approach would have been to parse
the mount options _once_ at the beginning, saving them off in some
temporary supoerblock (or whatever - anything that can hold those
pre-parsed mount options), and then have the "test" callback literally
just check those parsed options.

That's not necessarily the only way to go about this - there are
probably other approaches too, I didn't really think too much about
this. But those two commits on their own are buggy, and the fix is
somewhat problematic too.

                     Linus

Re: [GIT PULL] SELinux fixes for v5.16 (#3)

[Olga Kornievskaia]

On Fri, Dec 17, 2021 at 4:47 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Dec 17, 2021 at 1:08 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > Can you please elaborate on what is problematic with the two patches
> > you've highlighted.
>
> Commit ec1ade6a0448 ("nfs: account for selinux security context when
> deciding to share superblock") adds the call to
> security_sb_mnt_opts_compat() from the nfs_compare_mount_options()
> function.
>
> But nfs_compare_mount_options() is called from nfs_compare_super(),
> which is used as the the "test" callback function for the "sget_fc()"
> call:
>
>         s = sget_fc(fc, compare_super, nfs_set_super);
>
> and sget_fc() traverses all the mounted filesystems of this type -
> while holding the superblock lock that protects that list.
>
> So nfs_compare_super() may not sleep. It's called while holding a very
> core low-level lock, and it really is supposed to only do a "test".
> Not some complex operation that may do dynamic memory allocations and
> sleep.
>
> Yet that is exactly what security_sb_mnt_opts_compat() does, as done
> in 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an
> existing mount").
>
> So those two patches are completely broken.
>
> Now, commit cc274ae7763d ("selinux: fix sleeping function called from
> invalid context") that I just merged "fixes" this by making the
> allocations in parse_sid() be GFP_NOWAIT.
>
> That is a *HORRIBLE* fix. It's a horrible fix because
>
>  (a) GFP_NOWAIT can fail very easily, causing the mount to randomly
> fail for non-obvious reasons.
>
>  (b) even when it doesn't fail, you really shouldn't do things like
> this under a very core spinlock.
>
> Also, the original place - nfs_compare_mount_options() is called over
> and over for each mount, so you're parsing those same mount options
> over and over again. So not only was this sequence buggy, it's really
> not very smart to begin with.
>
> That's why I say that a much better approach would have been to parse
> the mount options _once_ at the beginning, saving them off in some
> temporary supoerblock (or whatever - anything that can hold those
> pre-parsed mount options), and then have the "test" callback literally
> just check those parsed options.
>
> That's not necessarily the only way to go about this - there are
> probably other approaches too, I didn't really think too much about
> this. But those two commits on their own are buggy, and the fix is
> somewhat problematic too.

Thank you for the explanation. We will try to accomplish what you
describe. We'll separate the ability to parse mount options without
checking them and then add the ability to compare/check security
contexts (if that's the right term). Then selinux parsing can allocate
without setting GFP_NOWAIT but do it outside of the nfs_compare_super.

>
>                      Linus

Re: [GIT PULL] SELinux fixes for v5.16 (#3)

[pr-tracker-bot]

The pull request you sent on Fri, 17 Dec 2021 15:02:34 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20211217

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f1f05ef38382021c9279cca8e9589f16fdfd1f40

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html