| Message ID | CAHC9VhS=WgqJgqpQq9J+0Pec9u8e1VnvGwqOimR54wm6TRptVA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [GIT,PULL] SELinux fixes for v5.16 (#3) | expand |
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
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
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!
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
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
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(-)