MIME-Version: 1.0
References: <CAHC9VhS=WgqJgqpQq9J+0Pec9u8e1VnvGwqOimR54wm6TRptVA@mail.gmail.com>
 <CAHk-=wiiqvcA3noHDqJt2=ik5ikQbycdFQ7s=uq70FcGxWgXvg@mail.gmail.com>
 <CAN-5tyEKGQu1Y=o8KfsX3q9NkP4XZRos5stwmrT=ZV1hr1fWrQ@mail.gmail.com> <CAHk-=wiDT0aZO6UFnb9sW4rfuxp4xfPTSydnifVgL6H8b5Rb4Q@mail.gmail.com>
In-Reply-To: <CAHk-=wiDT0aZO6UFnb9sW4rfuxp4xfPTSydnifVgL6H8b5Rb4Q@mail.gmail.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Sat, 18 Dec 2021 08:40:06 -0500
Message-ID: <CAN-5tyEfuh9zNUsmrNd6kVFuuwmhVdGXM4JboA4OzgQqiqCC_Q@mail.gmail.com>
Subject: Re: [GIT PULL] SELinux fixes for v5.16 (#3)
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Paul Moore <paul@paul-moore.com>,
        Olga Kornievskaia <kolga@netapp.com>,
        Anna Schumaker <Anna.Schumaker@netapp.com>,
        Scott Mayhew <smayhew@redhat.com>,
        SElinux list <selinux@vger.kernel.org>,
        LSM List <linux-security-module@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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