linux-kernel - Re: [PATCH 3/9] VFS: Introduce a mount context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1494355884.2659.18.camel@redhat.com>
Date:   Tue, 09 May 2017 14:51:24 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     Miklos Szeredi <mszeredi@...hat.com>,
        David Howells <dhowells@...hat.com>
Cc:     viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-nfs@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/9] VFS: Introduce a mount context

On Tue, 2017-05-09 at 14:02 +0200, Miklos Szeredi wrote:
> On Tue, May 9, 2017 at 11:41 AM, David Howells <dhowells@...hat.com> wrote:
> > Miklos Szeredi <mszeredi@...hat.com> wrote:
> > 
> > > I think that's crazy.  We don't return detailed errors for any other
> > > syscall for path lookup, so why would path lookup for mount be
> > > special.
> > 
> > Firstly, we don't return detailed errors for mount() at the moment either.
> > 
> > Secondly, path lookup might entail automounts, so perhaps we should do it for
> > path lookup too.  Particularly in light of the fact that NFS4 mount uses
> > pathwalk to get from server:/ to server:/the/dir/I/actually/wanted/ so I'm
> > currently losing that error:-/
> > 
> > Thirdly, the security operation I'm talking about is separate to path lookup -
> > though perhaps we should pass LOOKUP_MOUNT as an intent flag into pathwalk so
> > that the security check can be done there; perhaps combined with another one.
> > 
> > Fourthly, why shouldn't we consider extending the facility to other system
> > calls in future?  It would involve copying the string to task_struct and
> > providing a way to retrieve it, but that's not that hard to achieve.
> 
> Maybe we should.   In fact that sounds like a splendid idea.  IMO even
> better, than having errors go via the fsfd descriptor.  Pretty cheap
> on the kernel side, and completely optional on the userspace side.
> 

A question here: What should happen if you go to set an error here, and
one is already set? Should it just free the string and replace it with
the new one? IOW, just keep the latest error? Or is it better to keep
the earlier one?

If you want to put this in the task_struct then I think you'll want to
sort that out. You could easily end up in this situation if a lot of
different kernel subsystems started using it to pass back detailed
errors.

> > 
> > > And why would
> > > 
> > >     fd = open("/foo/bar", O_PATH);
> > >     fsmount(fsfd, fd, NULL);
> > > 
> > > behave differently from
> > > 
> > >     fsmount(fsfd, -1, "/foo/bar");
> > > 
> > > ?
> > 
> > There's argument that the former should return EFAULT.  And that you should
> > set the path to "" and pass AT_EMPTY_PATH.  I should probably make sure it
> > does that - and add a flags field.  statx() was fixed to work this way.
> > 
> > Question for you: Should the MNT_* flags be passed to fsmount(), perhaps in
> > MS_* form?
> 
> MS_* flags are a mess.  I don't think they should be used for any new
> functionality.  MNT_* flags are much better, but there are some
> internal flags there as well.
> 
> I think the struct file model is better, where we have the external
> O_* flags and the internal FMODE_* flags.
> 
> Thanks,
> Miklos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jeff Layton <jlayton@...hat.com>