Re: linux-next: manual merge of the selinux tree with the vfs tree

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Tue, Nov 27, 2018 at 1:52 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Paul,
>
> Today's linux-next merge of the selinux tree got a conflict in:
>
>   security/selinux/hooks.c
>
> between commit:
>
>   0472421f47a9 ("vfs: Remove unused code after filesystem context changes")
>
> from the vfs tree and commit:
>
>   2cbdcb882f97 ("selinux: always allow mounting submounts")
>
> from the selinux tree.
>
> I fixed it up (the former removed the function updated by the latter -
> I am not sure if there are further changes necessary) and can carry the
> fix as necessary. This is now fixed as far as linux-next is concerned,
> but any non trivial conflicts should be mentioned to your upstream
> maintainer when your tree is submitted for merging.  You may also want
> to consider cooperating with the maintainer of the conflicting tree to
> minimise any particularly complex conflicts.

Hm... seems that there was some massive overhaul in the VFS code right
at the wrong moment... There are new hooks for mounting now and the
code that our commit changes is now here:

https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131

It seems that the logic is still the same, just now our patch (or the
VFS one) needs to be updated to change the above line as such
(untested pseudo-patch):

- if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
+ if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))

Thanks for the heads up, Stephen!

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Tue, Nov 27, 2018 at 9:53 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Nov 27, 2018 at 1:52 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Paul,
> >
> > Today's linux-next merge of the selinux tree got a conflict in:
> >
> >   security/selinux/hooks.c
> >
> > between commit:
> >
> >   0472421f47a9 ("vfs: Remove unused code after filesystem context changes")
> >
> > from the vfs tree and commit:
> >
> >   2cbdcb882f97 ("selinux: always allow mounting submounts")
> >
> > from the selinux tree.
> >
> > I fixed it up (the former removed the function updated by the latter -
> > I am not sure if there are further changes necessary) and can carry the
> > fix as necessary. This is now fixed as far as linux-next is concerned,
> > but any non trivial conflicts should be mentioned to your upstream
> > maintainer when your tree is submitted for merging.  You may also want
> > to consider cooperating with the maintainer of the conflicting tree to
> > minimise any particularly complex conflicts.
>
> Hm... seems that there was some massive overhaul in the VFS code right
> at the wrong moment... There are new hooks for mounting now and the
> code that our commit changes is now here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131

For convenience, here are direct links to the most important -next VFS
commits that are related:

https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=c87c47c34750e9ee1ff0345593f3cbf6726b9d4e
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=4786c3427b2517ee9c685f95bf5b3185e332e64d
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=37744f3d21f8dbf6bb65e1ecef38c2cf9503d202
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=for-next&id=0472421f47a97be4b741d55ffd18f68ed9ba7cea

>
> It seems that the logic is still the same, just now our patch (or the
> VFS one) needs to be updated to change the above line as such
> (untested pseudo-patch):
>
> - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
>
> Thanks for the heads up, Stephen!
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

Hi Ondrej,

On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Hm... seems that there was some massive overhaul in the VFS code right
> at the wrong moment... There are new hooks for mounting now and the

The mount changes have been in linux-next since before the last
release ...

> code that our commit changes is now here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> 
> It seems that the logic is still the same, just now our patch (or the
> VFS one) needs to be updated to change the above line as such
> (untested pseudo-patch):
> 
> - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))

OK, so from tomorrow I will use that merge resolution.  Someone needs
to remember to tell Linus about this when the latter of the vfs and
selinux trees reach him.

Thanks for the better resolution.
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Paul Moore]

On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Ondrej,
>
> On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Hm... seems that there was some massive overhaul in the VFS code right
> > at the wrong moment... There are new hooks for mounting now and the
>
> The mount changes have been in linux-next since before the last
> release ...
>
> > code that our commit changes is now here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> >
> > It seems that the logic is still the same, just now our patch (or the
> > VFS one) needs to be updated to change the above line as such
> > (untested pseudo-patch):
> >
> > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
>
> OK, so from tomorrow I will use that merge resolution.  Someone needs
> to remember to tell Linus about this when the latter of the vfs and
> selinux trees reach him.

I will, or at least I'll do my best to remember; since we only have a
few more week until the merge window I like my odds.  FWIW, I
typically do a test merge on top of Linus' tree before sending the
SELinux PR just to verify that everything is relatively clean and
there are no surprises.

Ondrej, please work with David Howells to ensure that submounts are
handled correctly in his mount rework.

-- 
paul moore
www.paul-moore.com

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Wed, Nov 28, 2018 at 10:52 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Ondrej,
> >
> > On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Hm... seems that there was some massive overhaul in the VFS code right
> > > at the wrong moment... There are new hooks for mounting now and the
> >
> > The mount changes have been in linux-next since before the last
> > release ...
> >
> > > code that our commit changes is now here:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> > >
> > > It seems that the logic is still the same, just now our patch (or the
> > > VFS one) needs to be updated to change the above line as such
> > > (untested pseudo-patch):
> > >
> > > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
> >
> > OK, so from tomorrow I will use that merge resolution.  Someone needs
> > to remember to tell Linus about this when the latter of the vfs and
> > selinux trees reach him.
>
> I will, or at least I'll do my best to remember; since we only have a
> few more week until the merge window I like my odds.  FWIW, I
> typically do a test merge on top of Linus' tree before sending the
> SELinux PR just to verify that everything is relatively clean and
> there are no surprises.
>
> Ondrej, please work with David Howells to ensure that submounts are
> handled correctly in his mount rework.

OK, I will verify that the SELinux submount fix rebased on top of
vfs/work.mount in the way I suggested above passes the same testing
(seliinux-testsuite + NFS crossmnt reproducer). I am now building two
kernels (vfs/work.mount with and without the fix) to test. Let me know
if there is anything more to do.

Thanks,

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Paul Moore]

On Thu, Nov 29, 2018 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, Nov 28, 2018 at 10:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > Hi Ondrej,
> > >
> > > On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Hm... seems that there was some massive overhaul in the VFS code right
> > > > at the wrong moment... There are new hooks for mounting now and the
> > >
> > > The mount changes have been in linux-next since before the last
> > > release ...
> > >
> > > > code that our commit changes is now here:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> > > >
> > > > It seems that the logic is still the same, just now our patch (or the
> > > > VFS one) needs to be updated to change the above line as such
> > > > (untested pseudo-patch):
> > > >
> > > > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > > > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
> > >
> > > OK, so from tomorrow I will use that merge resolution.  Someone needs
> > > to remember to tell Linus about this when the latter of the vfs and
> > > selinux trees reach him.
> >
> > I will, or at least I'll do my best to remember; since we only have a
> > few more week until the merge window I like my odds.  FWIW, I
> > typically do a test merge on top of Linus' tree before sending the
> > SELinux PR just to verify that everything is relatively clean and
> > there are no surprises.
> >
> > Ondrej, please work with David Howells to ensure that submounts are
> > handled correctly in his mount rework.
>
> OK, I will verify that the SELinux submount fix rebased on top of
> vfs/work.mount in the way I suggested above passes the same testing
> (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
> kernels (vfs/work.mount with and without the fix) to test. Let me know
> if there is anything more to do.

Thanks.

The big thing is just making sure that we don't regress on the fix in
selinux/next if/when David's mount rework hits Linus' tree.

-- 
paul moore
www.paul-moore.com

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Al Viro]

On Thu, Nov 29, 2018 at 05:23:24PM -0500, Paul Moore wrote:

> > OK, I will verify that the SELinux submount fix rebased on top of
> > vfs/work.mount in the way I suggested above passes the same testing
> > (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
> > kernels (vfs/work.mount with and without the fix) to test. Let me know
> > if there is anything more to do.
> 
> Thanks.
> 
> The big thing is just making sure that we don't regress on the fix in
> selinux/next if/when David's mount rework hits Linus' tree.

FWIW, the whole thing is getting massaged/reordered/etc. and I would
like some input from you guys at some point - assuming that I recover
the ability to talk about LSM without obscenities...

Question: what *should* happen if we try to cross into a submount and find
that the thing on the other side is already mounted elsewhere, with incompatible
LSM options?  Ditto for referrals, with an extra twist - what if we are given
3 alternatives, the first two already mounted elsewhere with incompatible
options, the third one not mounted anywhere yet?

Incidentally, should smack have ->sb_clone_mnt_opts()?

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Casey Schaufler]

On 11/29/2018 3:51 PM, Al Viro wrote:

I've added linux-security-module to the CC list.

> On Thu, Nov 29, 2018 at 05:23:24PM -0500, Paul Moore wrote:
>
>>> OK, I will verify that the SELinux submount fix rebased on top of
>>> vfs/work.mount in the way I suggested above passes the same testing
>>> (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
>>> kernels (vfs/work.mount with and without the fix) to test. Let me know
>>> if there is anything more to do.
>> Thanks.
>>
>> The big thing is just making sure that we don't regress on the fix in
>> selinux/next if/when David's mount rework hits Linus' tree.
> FWIW, the whole thing is getting massaged/reordered/etc. and I would
> like some input from you guys at some point - assuming that I recover
> the ability to talk about LSM without obscenities...
>
> Question: what *should* happen if we try to cross into a submount and find
> that the thing on the other side is already mounted elsewhere, with incompatible
> LSM options?  Ditto for referrals, with an extra twist - what if we are given
> 3 alternatives, the first two already mounted elsewhere with incompatible
> options, the third one not mounted anywhere yet?

I fear that the safe answer and the containers answer are likely
to differ. The safe answer has to be to refuse the mount.

> Incidentally, should smack have ->sb_clone_mnt_opts()?

Probably, but I could never figure out what it was for,
and haven't identified a problem with not using it.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Al Viro]

On Thu, Nov 29, 2018 at 04:57:20PM -0800, Casey Schaufler wrote:

> > Question: what *should* happen if we try to cross into a submount and find
> > that the thing on the other side is already mounted elsewhere, with incompatible
> > LSM options?  Ditto for referrals, with an extra twist - what if we are given
> > 3 alternatives, the first two already mounted elsewhere with incompatible
> > options, the third one not mounted anywhere yet?
> 
> I fear that the safe answer and the containers answer are likely
> to differ. The safe answer has to be to refuse the mount.
> 
> > Incidentally, should smack have ->sb_clone_mnt_opts()?
> 
> Probably, but I could never figure out what it was for,
> and haven't identified a problem with not using it.

Transferring the Linux S&M options when crossing into a submount.

Frankly, the set of mount-related hooks is atrocious - way too much
duplication between them (sb_kern_mount vs. sb_set_mnt_opts vs.
sb_parse_opts_str vs. sb_clone_mnt_opts) and that, actually, is the
worst part of safely untangling the mount-API series ;-/

And then there's sb_mount, with 3 instances and arseloads of
races in 2 out of 3.  Consider e.g. this:
        if (need_dev) {
                /* Get mount point or device file. */
                if (!dev_name || kern_path(dev_name, LOOKUP_FOLLOW, &path)) {
                        error = -ENOENT;
                        goto out;
                }
                obj.path1 = path;
                requested_dev_name = tomoyo_realpath_from_path(&path);
                if (!requested_dev_name) {
                        error = -ENOENT;
                        goto out;
                }
in tomoyo.  OK, so we do a pathname resolution of dev_name (including
the source in mount --bind case).  Then we apply checks to it...
and proceed to...
        if (obj.path1.dentry)
                path_put(&obj.path1);
... discard the result of lookup.  Then the caller proceeds to do
the work, including (at various locations, depending upon the
mount(2) flags, fs type, etc.) looking dev_name up.  Could you spell TOCTOU?

Or, for example, this:
        if (!dev_name || !*dev_name)
                return -EINVAL;

        flags &= MS_REC | MS_BIND;

        error = kern_path(dev_name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &old_path);
        if (error)
                return error;

        get_buffers(buffer, old_buffer);
        error = fn_for_each_confined(label, profile,
                        match_mnt(profile, path, buffer, &old_path, old_buffer,
                                  NULL, flags, NULL, false));
        put_buffers(buffer, old_buffer);
        path_put(&old_path);
Same story, same TOCTOU race, this time in apparmour...

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Al Viro]

On Fri, Nov 30, 2018 at 01:27:07AM +0000, Al Viro wrote:

> And then there's sb_mount, with 3 instances and arseloads of
> races in 2 out of 3.

PS: the 3rd one (in selinux) is, AFAICS, TOCTOU-free, because
it ignores everything except the mountpoint, which is already
looked up by the caller.  No idea what any out-of-tree ones do,
of course, but judging by the in-tree sample...

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Thu, Nov 29, 2018 at 11:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Nov 28, 2018 at 10:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > Hi Ondrej,
> > >
> > > On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Hm... seems that there was some massive overhaul in the VFS code right
> > > > at the wrong moment... There are new hooks for mounting now and the
> > >
> > > The mount changes have been in linux-next since before the last
> > > release ...
> > >
> > > > code that our commit changes is now here:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> > > >
> > > > It seems that the logic is still the same, just now our patch (or the
> > > > VFS one) needs to be updated to change the above line as such
> > > > (untested pseudo-patch):
> > > >
> > > > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > > > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
> > >
> > > OK, so from tomorrow I will use that merge resolution.  Someone needs
> > > to remember to tell Linus about this when the latter of the vfs and
> > > selinux trees reach him.
> >
> > I will, or at least I'll do my best to remember; since we only have a
> > few more week until the merge window I like my odds.  FWIW, I
> > typically do a test merge on top of Linus' tree before sending the
> > SELinux PR just to verify that everything is relatively clean and
> > there are no surprises.
> >
> > Ondrej, please work with David Howells to ensure that submounts are
> > handled correctly in his mount rework.
>
> OK, I will verify that the SELinux submount fix rebased on top of
> vfs/work.mount in the way I suggested above passes the same testing
> (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
> kernels (vfs/work.mount with and without the fix) to test. Let me know
> if there is anything more to do.

I tested the proposed patch ([1]; fixed as per correction from David
Howells) applied on top of patches v4.19-rc3..vfs/work.mount applied
on top of the 4.19.5-300 Fedora 29 kernel.

However, the submount test was still failing, so I looked again at the
list of the possible 'purpose' values and it turns out the value used
by NFS et al. is actually FS_CONTEXT_FOR_ROOT_MOUNT (it is actually
documented nicely in Documentation/filesystems/mount_api.txt). So I'll
need to build a new test kernel with updated patch ([2]) and retest...

BTW, the vfs/work.mount changes alone seem to cause some overlay test
failures (I didn't test a clean 4.19.5 so it may be due to some stable
patch as well):

Test Summary Report
-------------------
overlay/test              (Wstat: 3072 Tests: 119 Failed: 12)
 Failed tests:  66, 74, 76-77, 79, 87, 95, 103, 108, 110-111
               117
 Non-zero exit status: 12

The failing tests are all in the context mount section, but I don't
think this is (directly) related to [3] because there are much more
tests failing and the kernel I was testing didn't include the
problematic OverlayFS patch. Perhaps the VFS patches somehow broke the
parsing of the context= mount option?

[1] https://gitlab.com/omos/linux-public/commit/fe5478717ddde92e3ea599e14051ad57522fdf47
[2] https://gitlab.com/omos/linux-public/commit/f5c58adc7babd62e4bfe8cda799459d263dc5186
[3] https://github.com/SELinuxProject/selinux-kernel/issues/43

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Sat, Dec 1, 2018 at 10:32 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Nov 29, 2018 at 11:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Nov 28, 2018 at 10:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > > Hi Ondrej,
> > > >
> > > > On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Hm... seems that there was some massive overhaul in the VFS code right
> > > > > at the wrong moment... There are new hooks for mounting now and the
> > > >
> > > > The mount changes have been in linux-next since before the last
> > > > release ...
> > > >
> > > > > code that our commit changes is now here:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> > > > >
> > > > > It seems that the logic is still the same, just now our patch (or the
> > > > > VFS one) needs to be updated to change the above line as such
> > > > > (untested pseudo-patch):
> > > > >
> > > > > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > > > > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
> > > >
> > > > OK, so from tomorrow I will use that merge resolution.  Someone needs
> > > > to remember to tell Linus about this when the latter of the vfs and
> > > > selinux trees reach him.
> > >
> > > I will, or at least I'll do my best to remember; since we only have a
> > > few more week until the merge window I like my odds.  FWIW, I
> > > typically do a test merge on top of Linus' tree before sending the
> > > SELinux PR just to verify that everything is relatively clean and
> > > there are no surprises.
> > >
> > > Ondrej, please work with David Howells to ensure that submounts are
> > > handled correctly in his mount rework.
> >
> > OK, I will verify that the SELinux submount fix rebased on top of
> > vfs/work.mount in the way I suggested above passes the same testing
> > (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
> > kernels (vfs/work.mount with and without the fix) to test. Let me know
> > if there is anything more to do.
>
> I tested the proposed patch ([1]; fixed as per correction from David
> Howells) applied on top of patches v4.19-rc3..vfs/work.mount applied
> on top of the 4.19.5-300 Fedora 29 kernel.
>
> However, the submount test was still failing, so I looked again at the
> list of the possible 'purpose' values and it turns out the value used
> by NFS et al. is actually FS_CONTEXT_FOR_ROOT_MOUNT (it is actually
> documented nicely in Documentation/filesystems/mount_api.txt). So I'll
> need to build a new test kernel with updated patch ([2]) and retest...

Unfortunately, even with FS_CONTEXT_FOR_ROOT_MOUNT the submount test
is still failing. (Actually, re-reading the description for
FS_CONTEXT_FOR_ROOT_MOUNT it seems it is not actually related to
submounts, although we should probably still skip the check for it,
too.) I will need to dig deeper into this on Monday...

FWIW, the generated AVC denial is still the same so it must be failing
the check in that particular hook (the FILESYSTEM__MOUNT permission is
checked only in that single place):

type=AVC msg=audit(02.12.2018 01:55:03.626:604) : avc:  denied  {
mount } for  pid=4036 comm=cat name=/ dev="0:48" ino=2
scontext=unconfined_u:unconfined_r:test_readnfs_t:s0-s0:c0.c1023
tcontext=system_u:object_r:nfs_t:s0 tclass=filesystem permissive=0

>
> BTW, the vfs/work.mount changes alone seem to cause some overlay test
> failures (I didn't test a clean 4.19.5 so it may be due to some stable
> patch as well):
>
> Test Summary Report
> -------------------
> overlay/test              (Wstat: 3072 Tests: 119 Failed: 12)
>  Failed tests:  66, 74, 76-77, 79, 87, 95, 103, 108, 110-111
>                117
>  Non-zero exit status: 12
>
> The failing tests are all in the context mount section, but I don't
> think this is (directly) related to [3] because there are much more
> tests failing and the kernel I was testing didn't include the
> problematic OverlayFS patch. Perhaps the VFS patches somehow broke the
> parsing of the context= mount option?
>
> [1] https://gitlab.com/omos/linux-public/commit/fe5478717ddde92e3ea599e14051ad57522fdf47
> [2] https://gitlab.com/omos/linux-public/commit/f5c58adc7babd62e4bfe8cda799459d263dc5186
> [3] https://github.com/SELinuxProject/selinux-kernel/issues/43
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Sun, Dec 2, 2018 at 10:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Sat, Dec 1, 2018 at 10:32 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Thu, Nov 29, 2018 at 11:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Wed, Nov 28, 2018 at 10:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Nov 27, 2018 at 6:50 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > > > Hi Ondrej,
> > > > >
> > > > > On Tue, 27 Nov 2018 09:53:32 +0100 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > >
> > > > > > Hm... seems that there was some massive overhaul in the VFS code right
> > > > > > at the wrong moment... There are new hooks for mounting now and the
> > > > >
> > > > > The mount changes have been in linux-next since before the last
> > > > > release ...
> > > > >
> > > > > > code that our commit changes is now here:
> > > > > >
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/tree/security/selinux/hooks.c?h=for-next#n3131
> > > > > >
> > > > > > It seems that the logic is still the same, just now our patch (or the
> > > > > > VFS one) needs to be updated to change the above line as such
> > > > > > (untested pseudo-patch):
> > > > > >
> > > > > > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > > > > > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
> > > > >
> > > > > OK, so from tomorrow I will use that merge resolution.  Someone needs
> > > > > to remember to tell Linus about this when the latter of the vfs and
> > > > > selinux trees reach him.
> > > >
> > > > I will, or at least I'll do my best to remember; since we only have a
> > > > few more week until the merge window I like my odds.  FWIW, I
> > > > typically do a test merge on top of Linus' tree before sending the
> > > > SELinux PR just to verify that everything is relatively clean and
> > > > there are no surprises.
> > > >
> > > > Ondrej, please work with David Howells to ensure that submounts are
> > > > handled correctly in his mount rework.
> > >
> > > OK, I will verify that the SELinux submount fix rebased on top of
> > > vfs/work.mount in the way I suggested above passes the same testing
> > > (seliinux-testsuite + NFS crossmnt reproducer). I am now building two
> > > kernels (vfs/work.mount with and without the fix) to test. Let me know
> > > if there is anything more to do.
> >
> > I tested the proposed patch ([1]; fixed as per correction from David
> > Howells) applied on top of patches v4.19-rc3..vfs/work.mount applied
> > on top of the 4.19.5-300 Fedora 29 kernel.
> >
> > However, the submount test was still failing, so I looked again at the
> > list of the possible 'purpose' values and it turns out the value used
> > by NFS et al. is actually FS_CONTEXT_FOR_ROOT_MOUNT (it is actually
> > documented nicely in Documentation/filesystems/mount_api.txt). So I'll
> > need to build a new test kernel with updated patch ([2]) and retest...
>
> Unfortunately, even with FS_CONTEXT_FOR_ROOT_MOUNT the submount test
> is still failing. (Actually, re-reading the description for
> FS_CONTEXT_FOR_ROOT_MOUNT it seems it is not actually related to
> submounts, although we should probably still skip the check for it,
> too.) I will need to dig deeper into this on Monday...

I think I figured out what's the problem. NFS still creates the
submount via the old vfs_submount() call, which calls
vfs_kern_mount(), which creates an fs_context with
FS_CONTEXT_FOR_USER_MOUNT because FS_CONTEXT_FOR_SUBMOUNT needs the
mountpoint dentry reference and there is currently no way to pass that
to vfs_kern_mount(). This is further complicated by the fact that
vfs_submount() accepts only a const reference to the mountpoint, while
vfs_new_fs_context() expects a non-const one...

I think all users of the old vfs_submount call should be converted to
the new API before the VFS changes are merged into mainline, otherwise
they will break the SELinux submount fix. We could work around it in
the SELinux hook by checking the fc->sb_flags[_mask] for SB_SUBMOUNT,
but I guess that would be a hack.

>
> FWIW, the generated AVC denial is still the same so it must be failing
> the check in that particular hook (the FILESYSTEM__MOUNT permission is
> checked only in that single place):
>
> type=AVC msg=audit(02.12.2018 01:55:03.626:604) : avc:  denied  {
> mount } for  pid=4036 comm=cat name=/ dev="0:48" ino=2
> scontext=unconfined_u:unconfined_r:test_readnfs_t:s0-s0:c0.c1023
> tcontext=system_u:object_r:nfs_t:s0 tclass=filesystem permissive=0
>
> >
> > BTW, the vfs/work.mount changes alone seem to cause some overlay test
> > failures (I didn't test a clean 4.19.5 so it may be due to some stable
> > patch as well):
> >
> > Test Summary Report
> > -------------------
> > overlay/test              (Wstat: 3072 Tests: 119 Failed: 12)
> >  Failed tests:  66, 74, 76-77, 79, 87, 95, 103, 108, 110-111
> >                117
> >  Non-zero exit status: 12
> >
> > The failing tests are all in the context mount section, but I don't
> > think this is (directly) related to [3] because there are much more
> > tests failing and the kernel I was testing didn't include the
> > problematic OverlayFS patch. Perhaps the VFS patches somehow broke the
> > parsing of the context= mount option?
> >
> > [1] https://gitlab.com/omos/linux-public/commit/fe5478717ddde92e3ea599e14051ad57522fdf47
> > [2] https://gitlab.com/omos/linux-public/commit/f5c58adc7babd62e4bfe8cda799459d263dc5186
> > [3] https://github.com/SELinuxProject/selinux-kernel/issues/43
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Al Viro]

On Mon, Dec 03, 2018 at 11:12:59AM +0100, Ondrej Mosnacek wrote:

> I think I figured out what's the problem. NFS still creates the
> submount via the old vfs_submount() call, which calls
> vfs_kern_mount(), which creates an fs_context with
> FS_CONTEXT_FOR_USER_MOUNT because FS_CONTEXT_FOR_SUBMOUNT needs the
> mountpoint dentry reference and there is currently no way to pass that
> to vfs_kern_mount(). This is further complicated by the fact that
> vfs_submount() accepts only a const reference to the mountpoint, while
> vfs_new_fs_context() expects a non-const one...
> 
> I think all users of the old vfs_submount call should be converted to
> the new API before the VFS changes are merged into mainline, otherwise
> they will break the SELinux submount fix. We could work around it in
> the SELinux hook by checking the fc->sb_flags[_mask] for SB_SUBMOUNT,
> but I guess that would be a hack.

Could you take a look at vfs.git#Q28?  There's still a massive reshuffling
going on, so there will be more branches; this one is the latest at the
moment.

I really hate the situation around sb_clone_mnt_opts/sb_set_mnt_opts and
I'm none too fond of the way fs_context_validate is done, so there will
be quite a bit of LSM tweaking.  If we are doing that, let's do it
right...

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Mon, Dec 3, 2018 at 10:56 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Dec 03, 2018 at 11:12:59AM +0100, Ondrej Mosnacek wrote:
>
> > I think I figured out what's the problem. NFS still creates the
> > submount via the old vfs_submount() call, which calls
> > vfs_kern_mount(), which creates an fs_context with
> > FS_CONTEXT_FOR_USER_MOUNT because FS_CONTEXT_FOR_SUBMOUNT needs the
> > mountpoint dentry reference and there is currently no way to pass that
> > to vfs_kern_mount(). This is further complicated by the fact that
> > vfs_submount() accepts only a const reference to the mountpoint, while
> > vfs_new_fs_context() expects a non-const one...
> >
> > I think all users of the old vfs_submount call should be converted to
> > the new API before the VFS changes are merged into mainline, otherwise
> > they will break the SELinux submount fix. We could work around it in
> > the SELinux hook by checking the fc->sb_flags[_mask] for SB_SUBMOUNT,
> > but I guess that would be a hack.
>
> Could you take a look at vfs.git#Q28?  There's still a massive reshuffling
> going on, so there will be more branches; this one is the latest at the
> moment.

I just tested the Q28 branch rebased onto a recent Fedora rawhide
kernel (4.20.0-0.rc5.git0.1) and that code seems to be working fine.
The submount test failed with Q28 and succeeds with Q28+fix, as
expected. Also, the overlay tests failures are gone now (except for
the 4 known failures from GH issue #43, since I had to rebase onto
4.20-rcX).

This is the commit that I used as the SELinux submount fix:
https://gitlab.com/omos/linux-public/commit/47922f9c70a83008388b836c285f94c40da1af2b

Kernel builds:
Unfixed Q28:  https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/833311/
Fixed Q28:  https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/833312/

Selinux-testsuite reports:
=== Q28 ===
Test Summary Report
-------------------
overlay/test              (Wstat: 1024 Tests: 119 Failed: 4)
  Failed tests:  81, 83, 107, 112
  Non-zero exit status: 4
submount/test             (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=54, Tests=615, 117 wallclock secs ( 0.20 usr  0.04 sys +  1.64
cusr  1.29 csys =  3.17 CPU)
Result: FAIL
Failed 2/54 test programs. 5/615 subtests failed.

=== Q28 + FIX ===
Test Summary Report
-------------------
overlay/test              (Wstat: 1024 Tests: 119 Failed: 4)
  Failed tests:  81, 83, 107, 112
  Non-zero exit status: 4
Files=54, Tests=615, 117 wallclock secs ( 0.22 usr  0.05 sys +  1.54
cusr  1.37 csys =  3.18 CPU)
Result: FAIL
Failed 1/54 test programs. 4/615 subtests failed.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Al Viro]

On Wed, Dec 05, 2018 at 10:37:56AM +0100, Ondrej Mosnacek wrote:

> I just tested the Q28 branch rebased onto a recent Fedora rawhide
> kernel (4.20.0-0.rc5.git0.1) and that code seems to be working fine.
> The submount test failed with Q28 and succeeds with Q28+fix, as
> expected. Also, the overlay tests failures are gone now (except for
> the 4 known failures from GH issue #43, since I had to rebase onto
> 4.20-rcX).
> 
> This is the commit that I used as the SELinux submount fix:
> https://gitlab.com/omos/linux-public/commit/47922f9c70a83008388b836c285f94c40da1af2b

FWIW, I'm none too happy about the fix.  Observations:
	* sb_get_tree (and sb_kern_mount past the "LSM: lift parsing
LSM options into the caller of ->sb_kern_mount()" in this series)
is equivalent to sb_set_mnt_opts() + (for userland mounts) an selinux-only
MAC check.  IOW, application of options (for LSMs that have those
in the first place) + actual "are we permitted to mount that?" check.
	* the second part should be done only for userland mounts -
not automounting, not kernel-internal ones, etc.
	* a very common pattern is "vfs_get_tree, vfs_create_mount if we
hadn't failed that, then unconditional put_fs_context".  So much that it
clearly deserves a helper - too much boilerplate as it is.
	* look at the callers of vfs_get_tree():
1) afs_mntpt_do_automount(): submount, helper fodder.  No MAC.
2) nfs_do_submount(): submount, standalone, but caller will very shortly follow
with the rest of the helper.  No MAC.
3) btrfs_mount_root(): helper fodder, cloned context, probably no point
in the actual MAC - we are in ->get_tree(), the caller will decide if
it wants to bother.
4) do_nfs4_mount(): NFS counterpart of the above.
5) mount_one_hugetlbfs(): kernel-internal, helper fodder, no MAC.
6) pid_ns_prepare_proc(): kernel-internal, helper fodder, no MAC.
7) mq_create_mount(): kernel-internal, helper fodder, no MAC.

8) do_new_mount(): almost a helper fodder, wants MAC (mount(2) guts)
9) fsopen(2): standalone, wants MAC (it's mount(2)-equivalent)
10) vfs_kern_mount(): that's a bit more complicated.  It is, again,
a helper fodder.  The need of MAC depends upon the caller, in theory.
Callers:
	simple_pin_fs() - kernel-internal, no MAC
	init_mount_tree() - no MAC, that's the absolute root and that, by
definition, is much too early in the boot (before initramfs, etc.) for
any LSM shite to be applicable.  In any case, it's done by the kernel.
	kern_mount() - kernel-internal, no MAC
	cpuset_get_tree() - part of ->get_tree(), caller will decide if they
want the damn MAC.
	vfs_submount() - automounts, presumably no MAC.

Conclusion: fuck the MAC in vfs_get_tree().  Let's just lift it into
do_new_mount()/fsopen().  The only thing we really need there is to
keep ->s_umount held (exclusive) until after the MAC.  So let vfs_get_tree()
return with fc->root->d_sb->s_umount locked and have mount_create_mount()
(which is _always_ preceded by successful vfs_get_tree()), unlock the
sucker.  In vfs_get_tree() we need to do sb_set_mnt_opts(), with the
rest of it (selinux-only) called by do_new_mount()/fsopen(2).  All there
is to it...

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Casey Schaufler]

On 12/5/2018 8:16 AM, Al Viro wrote:
> On Wed, Dec 05, 2018 at 10:37:56AM +0100, Ondrej Mosnacek wrote:
>
>> I just tested the Q28 branch rebased onto a recent Fedora rawhide
>> kernel (4.20.0-0.rc5.git0.1) and that code seems to be working fine.

Not so good with Smack.

	# mount -t tmpfs -o size=512m,smackfsroot=Pop tmpfs /mnt
	# attr -S -g SMACK64 /mnt
	Attribute "SMACK64" had a 1 byte value for /mnt:
	_
	#

attr should have reported a 3 byte value "Pop".

Re: linux-next: manual merge of the selinux tree with the vfs tree

[David Howells]

Ondrej Mosnacek <omosnace@redhat.com> wrote:

> - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))

It's not a bitmask, so you can't do that.  You'd need to do:

	if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT ||
	    fc->purpose == FS_CONTEXT_FOR_SUBMOUNT)

or use a switch.

David

Re: linux-next: manual merge of the selinux tree with the vfs tree

[Ondrej Mosnacek]

On Fri, Nov 30, 2018 at 4:10 PM David Howells <dhowells@redhat.com> wrote:
> Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> > - if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT)
> > + if (fc->purpose == (FS_CONTEXT_FOR_KERNEL_MOUNT|FS_CONTEXT_FOR_SUBMOUNT))
>
> It's not a bitmask, so you can't do that.  You'd need to do:
>
>         if (fc->purpose == FS_CONTEXT_FOR_KERNEL_MOUNT ||
>             fc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
>
> or use a switch.

Damn, you're right... I should have noticed that there is '==' instead of '&' :/

Thanks,

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.