[PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

Since 4.8 follow_automount() overrides the credentials with
&init_cred before calling d_automount(). When
rpcauth_lookupcred() is called in this context it is now using
fs[ug]id from the override creds instead of from the user's
creds, which can cause authentication to fail. To fix this, take
the ids from current_real_cred() instead.

Cc: stable@vger.kernel.org # v4.8+
CC: Eric W. Biederman <ebiederm@xmission.com>
Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 net/sunrpc/auth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 2bff63a73cf8..e6197b2bda86 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int flags)
 {
 	struct auth_cred acred;
 	struct rpc_cred *ret;
-	const struct cred *cred = current_cred();
+	const struct cred *cred = current_real_cred();
 
 	dprintk("RPC:       looking up %s cred\n",
 		auth->au_ops->au_name);
-- 
2.7.4

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Trond Myklebust]

T24gVGh1LCAyMDE2LTEyLTE1IGF0IDExOjEzIC0wNjAwLCBTZXRoIEZvcnNoZWUgd3JvdGU6DQo+
IFNpbmNlIDQuOCBmb2xsb3dfYXV0b21vdW50KCkgb3ZlcnJpZGVzIHRoZSBjcmVkZW50aWFscyB3
aXRoDQo+ICZpbml0X2NyZWQgYmVmb3JlIGNhbGxpbmcgZF9hdXRvbW91bnQoKS4gV2hlbg0KPiBy
cGNhdXRoX2xvb2t1cGNyZWQoKSBpcyBjYWxsZWQgaW4gdGhpcyBjb250ZXh0IGl0IGlzIG5vdyB1
c2luZw0KPiBmc1t1Z11pZCBmcm9tIHRoZSBvdmVycmlkZSBjcmVkcyBpbnN0ZWFkIG9mIGZyb20g
dGhlIHVzZXIncw0KPiBjcmVkcywgd2hpY2ggY2FuIGNhdXNlIGF1dGhlbnRpY2F0aW9uIHRvIGZh
aWwuIFRvIGZpeCB0aGlzLCB0YWtlDQo+IHRoZSBpZHMgZnJvbSBjdXJyZW50X3JlYWxfY3JlZCgp
IGluc3RlYWQuDQo+IA0KPiBDYzogc3RhYmxlQHZnZXIua2VybmVsLm9yZyAjIHY0LjgrDQo+IEND
OiBFcmljIFcuIEJpZWRlcm1hbiA8ZWJpZWRlcm1AeG1pc3Npb24uY29tPg0KPiBGaXhlczogYWVh
YTRhNzlmZjZhICgiZnM6IENhbGwgZF9hdXRvbW91bnQgd2l0aCB0aGUgZmlsZXN5c3RlbXMNCj4g
Y3JlZHMiKQ0KPiBTaWduZWQtb2ZmLWJ5OiBTZXRoIEZvcnNoZWUgPHNldGguZm9yc2hlZUBjYW5v
bmljYWwuY29tPg0KPiAtLS0NCj4gwqBuZXQvc3VucnBjL2F1dGguYyB8IDIgKy0NCj4gwqAxIGZp
bGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRpZmYgLS1n
aXQgYS9uZXQvc3VucnBjL2F1dGguYyBiL25ldC9zdW5ycGMvYXV0aC5jDQo+IGluZGV4IDJiZmY2
M2E3M2NmOC4uZTYxOTdiMmJkYTg2IDEwMDY0NA0KPiAtLS0gYS9uZXQvc3VucnBjL2F1dGguYw0K
PiArKysgYi9uZXQvc3VucnBjL2F1dGguYw0KPiBAQCAtNjIyLDcgKzYyMiw3IEBAIHJwY2F1dGhf
bG9va3VwY3JlZChzdHJ1Y3QgcnBjX2F1dGggKmF1dGgsIGludA0KPiBmbGFncykNCj4gwqB7DQo+
IMKgCXN0cnVjdCBhdXRoX2NyZWQgYWNyZWQ7DQo+IMKgCXN0cnVjdCBycGNfY3JlZCAqcmV0Ow0K
PiAtCWNvbnN0IHN0cnVjdCBjcmVkICpjcmVkID0gY3VycmVudF9jcmVkKCk7DQo+ICsJY29uc3Qg
c3RydWN0IGNyZWQgKmNyZWQgPSBjdXJyZW50X3JlYWxfY3JlZCgpOw0KPiDCoA0KPiDCoAlkcHJp
bnRrKCJSUEM6wqDCoMKgwqDCoMKgwqBsb29raW5nIHVwICVzIGNyZWRcbiIsDQo+IMKgCQlhdXRo
LT5hdV9vcHMtPmF1X25hbWUpOw0KDQpBbW9uZyBvdGhlciB0aGluZ3MsIHRoaXMgd2lsbCBicmVh
ayB0aGUgYWNjZXNzKCkgc3lzY2FsbC4gSXQncw0KY29tcGxldGVseSB0aGUgd3JvbmcgbGV2ZWwg
aW4gd2hpY2ggdG8gb3ZlcnJpZGUgY3JlZGVudGlhbHMuDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0
DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1
c3RAcHJpbWFyeWRhdGEuY29tDQo=

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> > Since 4.8 follow_automount() overrides the credentials with
> > &init_cred before calling d_automount(). When
> > rpcauth_lookupcred() is called in this context it is now using
> > fs[ug]id from the override creds instead of from the user's
> > creds, which can cause authentication to fail. To fix this, take
> > the ids from current_real_cred() instead.
> > 
> > Cc: stable@vger.kernel.org # v4.8+
> > CC: Eric W. Biederman <ebiederm@xmission.com>
> > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> > creds")
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  net/sunrpc/auth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > index 2bff63a73cf8..e6197b2bda86 100644
> > --- a/net/sunrpc/auth.c
> > +++ b/net/sunrpc/auth.c
> > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> > flags)
> >  {
> >  	struct auth_cred acred;
> >  	struct rpc_cred *ret;
> > -	const struct cred *cred = current_cred();
> > +	const struct cred *cred = current_real_cred();
> >  
> >  	dprintk("RPC:       looking up %s cred\n",
> >  		auth->au_ops->au_name);
> 
> Among other things, this will break the access() syscall.

Okay, I see that now.

> It's completely the wrong level in which to override credentials.

The reason for it is that sget() now has a capability check which will
fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
alternatives? A few ideas:

 - Instead of using a completely differnet set of creds, we could copy
   the current creds and raise CAP_SYS_ADMIN. This won't work if
   curreent is in a different user ns however.

 - Filesystems could get around the capability check by using
   sget_userns() during automount.

 - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
   check if that is set.

Any opinions or other ideas?

Thanks,
Seth

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> > > Since 4.8 follow_automount() overrides the credentials with
> > > &init_cred before calling d_automount(). When
> > > rpcauth_lookupcred() is called in this context it is now using
> > > fs[ug]id from the override creds instead of from the user's
> > > creds, which can cause authentication to fail. To fix this, take
> > > the ids from current_real_cred() instead.
> > > 
> > > Cc: stable@vger.kernel.org # v4.8+
> > > CC: Eric W. Biederman <ebiederm@xmission.com>
> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> > > creds")
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > >  net/sunrpc/auth.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > > index 2bff63a73cf8..e6197b2bda86 100644
> > > --- a/net/sunrpc/auth.c
> > > +++ b/net/sunrpc/auth.c
> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> > > flags)
> > >  {
> > >  	struct auth_cred acred;
> > >  	struct rpc_cred *ret;
> > > -	const struct cred *cred = current_cred();
> > > +	const struct cred *cred = current_real_cred();
> > >  
> > >  	dprintk("RPC:       looking up %s cred\n",
> > >  		auth->au_ops->au_name);
> > 
> > Among other things, this will break the access() syscall.
> 
> Okay, I see that now.
> 
> > It's completely the wrong level in which to override credentials.
> 
> The reason for it is that sget() now has a capability check which will
> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
> alternatives? A few ideas:
> 
>  - Instead of using a completely differnet set of creds, we could copy
>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>    curreent is in a different user ns however.
> 
>  - Filesystems could get around the capability check by using
>    sget_userns() during automount.
> 
>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>    check if that is set.
> 
> Any opinions or other ideas?

I haven't seen any responses, possibly just got lost in the shuffle
during the holidays (I know it slipped my mind for a while).

Eric, what do you think about the last option above? From what I can see
looking up rpc credentials just isn't going to work with current_cred
overridden as we're doing for automount.

Thanks,
Seth

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
>> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
>> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
>> > > Since 4.8 follow_automount() overrides the credentials with
>> > > &init_cred before calling d_automount(). When
>> > > rpcauth_lookupcred() is called in this context it is now using
>> > > fs[ug]id from the override creds instead of from the user's
>> > > creds, which can cause authentication to fail. To fix this, take
>> > > the ids from current_real_cred() instead.
>> > > 
>> > > Cc: stable@vger.kernel.org # v4.8+
>> > > CC: Eric W. Biederman <ebiederm@xmission.com>
>> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
>> > > creds")
>> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>> > > ---
>> > >  net/sunrpc/auth.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > 
>> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> > > index 2bff63a73cf8..e6197b2bda86 100644
>> > > --- a/net/sunrpc/auth.c
>> > > +++ b/net/sunrpc/auth.c
>> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
>> > > flags)
>> > >  {
>> > >  	struct auth_cred acred;
>> > >  	struct rpc_cred *ret;
>> > > -	const struct cred *cred = current_cred();
>> > > +	const struct cred *cred = current_real_cred();
>> > >  
>> > >  	dprintk("RPC:       looking up %s cred\n",
>> > >  		auth->au_ops->au_name);
>> > 
>> > Among other things, this will break the access() syscall.
>> 
>> Okay, I see that now.
>> 
>> > It's completely the wrong level in which to override credentials.
>> 
>> The reason for it is that sget() now has a capability check which will
>> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
>> alternatives? A few ideas:
>> 
>>  - Instead of using a completely differnet set of creds, we could copy
>>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>>    curreent is in a different user ns however.
>> 
>>  - Filesystems could get around the capability check by using
>>    sget_userns() during automount.
>> 
>>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>>    check if that is set.
>> 
>> Any opinions or other ideas?
>
> I haven't seen any responses, possibly just got lost in the shuffle
> during the holidays (I know it slipped my mind for a while).
>
> Eric, what do you think about the last option above? From what I can see
> looking up rpc credentials just isn't going to work with current_cred
> overridden as we're doing for automount.

I got as far as there wasn't a correct thing to apply, and I have been
bogged down in enough other things that I haven't gotten back to this
one.

My gut feel is that we propbably want to do a little more reworking on
the autmount path.  But I don't exactly have a concrete proposal for
you at the moment.  I just found another 10 year old bug in the mount
code...

Eric

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

On Wed, Jan 11, 2017 at 01:21:57PM +1300, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
> >> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> >> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> >> > > Since 4.8 follow_automount() overrides the credentials with
> >> > > &init_cred before calling d_automount(). When
> >> > > rpcauth_lookupcred() is called in this context it is now using
> >> > > fs[ug]id from the override creds instead of from the user's
> >> > > creds, which can cause authentication to fail. To fix this, take
> >> > > the ids from current_real_cred() instead.
> >> > > 
> >> > > Cc: stable@vger.kernel.org # v4.8+
> >> > > CC: Eric W. Biederman <ebiederm@xmission.com>
> >> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> >> > > creds")
> >> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >> > > ---
> >> > >  net/sunrpc/auth.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > > 
> >> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> >> > > index 2bff63a73cf8..e6197b2bda86 100644
> >> > > --- a/net/sunrpc/auth.c
> >> > > +++ b/net/sunrpc/auth.c
> >> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> >> > > flags)
> >> > >  {
> >> > >  	struct auth_cred acred;
> >> > >  	struct rpc_cred *ret;
> >> > > -	const struct cred *cred = current_cred();
> >> > > +	const struct cred *cred = current_real_cred();
> >> > >  
> >> > >  	dprintk("RPC:       looking up %s cred\n",
> >> > >  		auth->au_ops->au_name);
> >> > 
> >> > Among other things, this will break the access() syscall.
> >> 
> >> Okay, I see that now.
> >> 
> >> > It's completely the wrong level in which to override credentials.
> >> 
> >> The reason for it is that sget() now has a capability check which will
> >> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
> >> alternatives? A few ideas:
> >> 
> >>  - Instead of using a completely differnet set of creds, we could copy
> >>    the current creds and raise CAP_SYS_ADMIN. This won't work if
> >>    curreent is in a different user ns however.
> >> 
> >>  - Filesystems could get around the capability check by using
> >>    sget_userns() during automount.
> >> 
> >>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
> >>    check if that is set.
> >> 
> >> Any opinions or other ideas?
> >
> > I haven't seen any responses, possibly just got lost in the shuffle
> > during the holidays (I know it slipped my mind for a while).
> >
> > Eric, what do you think about the last option above? From what I can see
> > looking up rpc credentials just isn't going to work with current_cred
> > overridden as we're doing for automount.
> 
> I got as far as there wasn't a correct thing to apply, and I have been
> bogged down in enough other things that I haven't gotten back to this
> one.
> 
> My gut feel is that we propbably want to do a little more reworking on
> the autmount path.  But I don't exactly have a concrete proposal for
> you at the moment.  I just found another 10 year old bug in the mount
> code...

With automounts we're mounting based on the credentials used when
mounting the parent. That's what your patch "fs: Call d_automount with
the filesystems creds" did, but it's overriding the credentials too
early in the call stack and causing these rpcauth problems.

But I think we should also be setting the submount's s_user_ns to be the
same as the parent's, not to current_user_ns. At that point we'd be
using the same credentials and user ns as when mounting the parent super
block, so we know the capability check would pass (since it passed for
the original mount) and don't really need to do it.

So if we could pass down through the call stack that a given mount
request is an automount from super block s (or at dentry d) then the fix
would be trivial. I don't see any way of passing that information
through currently though, without doing something undesirable like
adding arguments to the mount filesystem op.

Seth

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Wed, Jan 11, 2017 at 01:21:57PM +1300, Eric W. Biederman wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>> 
>> > On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
>> >> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
>> >> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
>> >> > > Since 4.8 follow_automount() overrides the credentials with
>> >> > > &init_cred before calling d_automount(). When
>> >> > > rpcauth_lookupcred() is called in this context it is now using
>> >> > > fs[ug]id from the override creds instead of from the user's
>> >> > > creds, which can cause authentication to fail. To fix this, take
>> >> > > the ids from current_real_cred() instead.
>> >> > > 
>> >> > > Cc: stable@vger.kernel.org # v4.8+
>> >> > > CC: Eric W. Biederman <ebiederm@xmission.com>
>> >> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
>> >> > > creds")
>> >> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>> >> > > ---
>> >> > >  net/sunrpc/auth.c | 2 +-
>> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > > 
>> >> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> >> > > index 2bff63a73cf8..e6197b2bda86 100644
>> >> > > --- a/net/sunrpc/auth.c
>> >> > > +++ b/net/sunrpc/auth.c
>> >> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
>> >> > > flags)
>> >> > >  {
>> >> > >  	struct auth_cred acred;
>> >> > >  	struct rpc_cred *ret;
>> >> > > -	const struct cred *cred = current_cred();
>> >> > > +	const struct cred *cred = current_real_cred();
>> >> > >  
>> >> > >  	dprintk("RPC:       looking up %s cred\n",
>> >> > >  		auth->au_ops->au_name);
>> >> > 
>> >> > Among other things, this will break the access() syscall.
>> >> 
>> >> Okay, I see that now.
>> >> 
>> >> > It's completely the wrong level in which to override credentials.
>> >> 
>> >> The reason for it is that sget() now has a capability check which will
>> >> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
>> >> alternatives? A few ideas:
>> >> 
>> >>  - Instead of using a completely differnet set of creds, we could copy
>> >>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>> >>    curreent is in a different user ns however.
>> >> 
>> >>  - Filesystems could get around the capability check by using
>> >>    sget_userns() during automount.
>> >> 
>> >>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>> >>    check if that is set.
>> >> 
>> >> Any opinions or other ideas?
>> >
>> > I haven't seen any responses, possibly just got lost in the shuffle
>> > during the holidays (I know it slipped my mind for a while).
>> >
>> > Eric, what do you think about the last option above? From what I can see
>> > looking up rpc credentials just isn't going to work with current_cred
>> > overridden as we're doing for automount.
>> 
>> I got as far as there wasn't a correct thing to apply, and I have been
>> bogged down in enough other things that I haven't gotten back to this
>> one.
>> 
>> My gut feel is that we propbably want to do a little more reworking on
>> the autmount path.  But I don't exactly have a concrete proposal for
>> you at the moment.  I just found another 10 year old bug in the mount
>> code...
>
> With automounts we're mounting based on the credentials used when
> mounting the parent. That's what your patch "fs: Call d_automount with
> the filesystems creds" did, but it's overriding the credentials too
> early in the call stack and causing these rpcauth problems.
>
> But I think we should also be setting the submount's s_user_ns to be the
> same as the parent's, not to current_user_ns. At that point we'd be
> using the same credentials and user ns as when mounting the parent super
> block, so we know the capability check would pass (since it passed for
> the original mount) and don't really need to do it.

If we special case the submount code that would be fine.  Normal sget
needs to continue to use current_user_ns.

> So if we could pass down through the call stack that a given mount
> request is an automount from super block s (or at dentry d) then the fix
> would be trivial. I don't see any way of passing that information
> through currently though, without doing something undesirable like
> adding arguments to the mount filesystem op.

So here are the pieces I have in my thinking about this.
1) We need to capture the cred of the mounter of the filesystem
   like overlayfs does but in general.  Call it s_cred or possibly
   s_mounter_cred.

   That would allow us to remove the hard coded cred in the automount
   path, and would allow unprivileged mounts to eventually use this
   functionality.

2) Al Viro mentioned to me that it would be very nice if the automount
   could coud run in a separate stack because this is one place where
   the stack can get very deep in the vfs.

My gut feel is that solving for those two constraints: the code running
in a separate thread, and the capturing not just s_user_ns but the cred
of the mounter should give us enough constraints to figure out how to
structure the code for long term maintenance.

I especially think the part about using the mounters creds will likely
solve the sunrpc problem.  I could of course be wrong but using the
creds of the process that happened to walk across the mount point seems
completely the wrong thing.  In general it feels wrong to expose which
user triggered the automount to me.  As that means the semantics can
very per user.

Now when we looked at autofs the semantics were in fact varying per
user (ugh).  So NFS may have the same legacy requirements.

But as a general maintainability principle I don't like vfs state that
varies depending upon the user.

Eric

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Eric W. Biederman]

With respect to nfs and automounts.

Does NFS have different automount behavior based on the user
performing the automount?

If NFS does not have different automount behavior depending on the user
we just use the creds of the original mounter of NFS?

If NFS does have different automount behavior depending on the user
(ouch!) we need to go through the call path and see where it makes
sense to over ride things and where it does not.



Seth the fundamental problem with your patch was that you were patching
a location that is used for more just mounts.

I am strongly wishing that we could just change follow_automount from:


	old_cred = override_creds(&init_cred);
	mnt = path->dentry->d_op->d_automount(path);
	revert_creds(old_cred);

to:

	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
	mnt = path->dentry->d_op->d_automount(path);
	revert_creds(old_cred);

And all will be well with nfs.  That does remain possible.

But looking at the code path you touched it seems to lookup the cred
based purely on the local uid, gid, and groups.  Which suggests to
me that even the original mounters creds may not be enough :(

At which point I am not certain of the solution.  But I fear that like
autofs NFS actually cares which user is transition the magic mountpoint,
and may return different data depending on who transitions the
mountpoint first.  Ick!  Nasty Nasty Ick!

Eric

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Trond Myklebust]

T24gV2VkLCAyMDE3LTAxLTI1IGF0IDEyOjI4ICsxMzAwLCBFcmljIFcuIEJpZWRlcm1hbiB3cm90
ZToNCj4gV2l0aCByZXNwZWN0IHRvIG5mcyBhbmQgYXV0b21vdW50cy4NCj4gDQo+IERvZXMgTkZT
IGhhdmUgZGlmZmVyZW50IGF1dG9tb3VudCBiZWhhdmlvciBiYXNlZCBvbiB0aGUgdXNlcg0KPiBw
ZXJmb3JtaW5nIHRoZSBhdXRvbW91bnQ/DQo+IA0KPiBJZiBORlMgZG9lcyBub3QgaGF2ZSBkaWZm
ZXJlbnQgYXV0b21vdW50IGJlaGF2aW9yIGRlcGVuZGluZyBvbiB0aGUNCj4gdXNlcg0KPiB3ZSBq
dXN0IHVzZSB0aGUgY3JlZHMgb2YgdGhlIG9yaWdpbmFsIG1vdW50ZXIgb2YgTkZTPw0KPiANCj4g
SWYgTkZTIGRvZXMgaGF2ZSBkaWZmZXJlbnQgYXV0b21vdW50IGJlaGF2aW9yIGRlcGVuZGluZyBv
biB0aGUgdXNlcg0KPiAob3VjaCEpIHdlIG5lZWQgdG8gZ28gdGhyb3VnaCB0aGUgY2FsbCBwYXRo
IGFuZCBzZWUgd2hlcmUgaXQgbWFrZXMNCj4gc2Vuc2UgdG8gb3ZlciByaWRlIHRoaW5ncyBhbmQg
d2hlcmUgaXQgZG9lcyBub3QuDQoNClRoZSByZWFzb24gd2h5IHRoZSBORlMgY2xpZW50IGNyZWF0
ZXMgYSBtb3VudHBvaW50IGlzIGJlY2F1c2Ugb24NCmVudGVyaW5nIGEgZGlyZWN0b3J5LCBpdCBk
ZXRlY3RzIHRoYXQgdGhlcmUgaXMgZWl0aGVyIGEgc2ltaWxhcg0KbW91bnRwb2ludCBvbiB0aGUg
c2VydmVyLCBvciB0aGVyZSBpcyBhIHJlZmVycmFsICh3aGljaCBhY3RzIHNvcnQgb2YNCmxpa2Ug
YSBzeW1saW5rLCBleGNlcHQgaXQgcG9pbnRzIHRvIGEgcGF0aCBvbiBvbmUgb3IgbW9yZSBkaWZm
ZXJlbnQgTkZTDQpzZXJ2ZXJzKS4NCldpdGhvdXQgdGhhdCBtb3VudHBvaW50LCBtb3N0IHRoaW5n
cyB3b3VsZCB3b3JrLCBidXQgdGhlIHVzZXIgd291bGQgZW5kDQp1cCBzZWVpbmcgbmFzdHkgbm9u
LXBvc2l4IGZlYXR1cmVzIGxpa2UgZHVwbGljYXRlIGlub2RlIG51bWJlcnMuDQoNCldlIGRvIG5v
dCB3YW50IHRvIHVzZSBhbnkgY3JlZHMgb3RoZXIgdGhhbiB1c2VyIGNyZWRzIGhlcmUsIGJlY2F1
c2UgYXMNCmZhciBhcyB0aGUgc2VjdXJpdHkgbW9kZWwgaXMgY29uY2VybmVkLCB0aGUgcHJvY2Vz
cyBpcyBqdXN0IGNyb3NzaW5nDQppbnRvIGFuIGV4aXN0aW5nIGRpcmVjdG9yeS4NCj4gDQo+IA0K
PiANCj4gU2V0aCB0aGUgZnVuZGFtZW50YWwgcHJvYmxlbSB3aXRoIHlvdXIgcGF0Y2ggd2FzIHRo
YXQgeW91IHdlcmUNCj4gcGF0Y2hpbmcNCj4gYSBsb2NhdGlvbiB0aGF0IGlzIHVzZWQgZm9yIG1v
cmUganVzdCBtb3VudHMuDQo+IA0KPiBJIGFtIHN0cm9uZ2x5IHdpc2hpbmcgdGhhdCB3ZSBjb3Vs
ZCBqdXN0IGNoYW5nZSBmb2xsb3dfYXV0b21vdW50DQo+IGZyb206DQo+IA0KPiANCj4gCW9sZF9j
cmVkID0gb3ZlcnJpZGVfY3JlZHMoJmluaXRfY3JlZCk7DQo+IAltbnQgPSBwYXRoLT5kZW50cnkt
PmRfb3AtPmRfYXV0b21vdW50KHBhdGgpOw0KPiAJcmV2ZXJ0X2NyZWRzKG9sZF9jcmVkKTsNCj4g
DQo+IHRvOg0KPiANCj4gCW9sZF9jcmVkID0gb3ZlcnJpZGVfY3JlZHMocGF0aC0+bW50LT5tbnRf
c2ItPnNfY3JlZCk7DQo+IAltbnQgPSBwYXRoLT5kZW50cnktPmRfb3AtPmRfYXV0b21vdW50KHBh
dGgpOw0KPiAJcmV2ZXJ0X2NyZWRzKG9sZF9jcmVkKTsNCj4gDQo+IEFuZCBhbGwgd2lsbCBiZSB3
ZWxsIHdpdGggbmZzLsKgwqBUaGF0IGRvZXMgcmVtYWluIHBvc3NpYmxlLg0KDQpOby4gVGhhdCB3
b3VsZCBicmVhayBwZXJtaXNzaW9ucyBjaGVja2luZy4gU2VlIGFib3ZlLg0KDQo+IA0KPiBCdXQg
bG9va2luZyBhdCB0aGUgY29kZSBwYXRoIHlvdSB0b3VjaGVkIGl0IHNlZW1zIHRvIGxvb2t1cCB0
aGUgY3JlZA0KPiBiYXNlZCBwdXJlbHkgb24gdGhlIGxvY2FsIHVpZCwgZ2lkLCBhbmQgZ3JvdXBz
LsKgwqBXaGljaCBzdWdnZXN0cyB0bw0KPiBtZSB0aGF0IGV2ZW4gdGhlIG9yaWdpbmFsIG1vdW50
ZXJzIGNyZWRzIG1heSBub3QgYmUgZW5vdWdoIDooDQo+IA0KPiBBdCB3aGljaCBwb2ludCBJIGFt
IG5vdCBjZXJ0YWluIG9mIHRoZSBzb2x1dGlvbi7CoMKgQnV0IEkgZmVhciB0aGF0DQo+IGxpa2UN
Cj4gYXV0b2ZzIE5GUyBhY3R1YWxseSBjYXJlcyB3aGljaCB1c2VyIGlzIHRyYW5zaXRpb24gdGhl
IG1hZ2ljDQo+IG1vdW50cG9pbnQsDQo+IGFuZCBtYXkgcmV0dXJuIGRpZmZlcmVudCBkYXRhIGRl
cGVuZGluZyBvbiB3aG8gdHJhbnNpdGlvbnMgdGhlDQo+IG1vdW50cG9pbnQgZmlyc3QuwqDCoElj
ayHCoMKgTmFzdHkgTmFzdHkgSWNrIQ0KPiANCg0KVGhlIHNlY3VyaXR5IG1vZGVsIGlzIHRoZSBz
YW1lIGFzIHRoYXQgb2YgYW4gb3JkaW5hcnkgZGlyZWN0b3J5LiBUaGUNCm9ubHkgZGlmZmVyZW5j
ZSBpcyB0aGF0IHdlIGNyZWF0ZSBhIG5ldyBzdXBlcmJsb2NrLiBXaHkgaXMgdGhhdCAiSWNrIj8N
Cg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgUHJp
bWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Eric W. Biederman]

Trond Myklebust <trondmy@primarydata.com> writes:

> On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
>> With respect to nfs and automounts.
>> 
>> Does NFS have different automount behavior based on the user
>> performing the automount?
>> 
>> If NFS does not have different automount behavior depending on the
>> user
>> we just use the creds of the original mounter of NFS?
>> 
>> If NFS does have different automount behavior depending on the user
>> (ouch!) we need to go through the call path and see where it makes
>> sense to over ride things and where it does not.
>
> The reason why the NFS client creates a mountpoint is because on
> entering a directory, it detects that there is either a similar
> mountpoint on the server, or there is a referral (which acts sort of
> like a symlink, except it points to a path on one or more different NFS
> servers).
> Without that mountpoint, most things would work, but the user would end
> up seeing nasty non-posix features like duplicate inode numbers.
>
> We do not want to use any creds other than user creds here, because as
> far as the security model is concerned, the process is just crossing
> into an existing directory.

But sget needs different creds.

Because the user who authorizes the mounting of the filesystem is
different than the user who is crossing into the new filesystem.

The local system now cares about that distinction even if the nfs server
does not.

>> Seth the fundamental problem with your patch was that you were
>> patching
>> a location that is used for more just mounts.
>> 
>> I am strongly wishing that we could just change follow_automount
>> from:
>> 
>> 
>> 	old_cred = override_creds(&init_cred);
>> 	mnt = path->dentry->d_op->d_automount(path);
>> 	revert_creds(old_cred);
>> 
>> to:
>> 
>> 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
>> 	mnt = path->dentry->d_op->d_automount(path);
>> 	revert_creds(old_cred);
>> 
>> And all will be well with nfs.  That does remain possible.
>
> No. That would break permissions checking. See above.

Then we need to look much harder at the permission checking
model of d_automount because we need to permission check against
both sets of creds.

>> But looking at the code path you touched it seems to lookup the cred
>> based purely on the local uid, gid, and groups.  Which suggests to
>> me that even the original mounters creds may not be enough :(
>> 
>> At which point I am not certain of the solution.  But I fear that
>> like
>> autofs NFS actually cares which user is transition the magic
>> mountpoint,
>> and may return different data depending on who transitions the
>> mountpoint first.  Ick!  Nasty Nasty Ick!
>> 
>
> The security model is the same as that of an ordinary directory. The
> only difference is that we create a new superblock. Why is that "Ick"?

The Ick is if the superblock that is created depends on the user
crossing the mountpoint.

AKA Do we get a different mountpoint if user A triggers the automount
vs user B triggering the automount?

If the problem is just root squash and not letting root trigger the
automount it is much less ick.  Weird but not ick.

Eric

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Trond Myklebust]

QWRkaW5nIERhdmlkIEhvd2VsbHMgYW5kIFN0ZXZlIEZyZW5jaCBhcyBJIGJlbGlldmUgYm90aCBB
RlMgYW5kIENJRlMNCmhhdmUgdGhlIGV4YWN0IHNhbWUgcmVxdWlyZW1lbnRzIGFuZCBORlMgaGVy
ZS4NCg0KT24gV2VkLCAyMDE3LTAxLTI1IGF0IDEyOjU2ICsxMzAwLCBFcmljIFcuIEJpZWRlcm1h
biB3cm90ZToNCj4gVHJvbmQgTXlrbGVidXN0IDx0cm9uZG15QHByaW1hcnlkYXRhLmNvbT4gd3Jp
dGVzOg0KPiANCj4gPiBPbiBXZWQsIDIwMTctMDEtMjUgYXQgMTI6MjggKzEzMDAsIEVyaWMgVy4g
QmllZGVybWFuIHdyb3RlOg0KPiA+ID4gV2l0aCByZXNwZWN0IHRvIG5mcyBhbmQgYXV0b21vdW50
cy4NCj4gPiA+IA0KPiA+ID4gRG9lcyBORlMgaGF2ZSBkaWZmZXJlbnQgYXV0b21vdW50IGJlaGF2
aW9yIGJhc2VkIG9uIHRoZSB1c2VyDQo+ID4gPiBwZXJmb3JtaW5nIHRoZSBhdXRvbW91bnQ/DQo+
ID4gPiANCj4gPiA+IElmIE5GUyBkb2VzIG5vdCBoYXZlIGRpZmZlcmVudCBhdXRvbW91bnQgYmVo
YXZpb3IgZGVwZW5kaW5nIG9uDQo+ID4gPiB0aGUNCj4gPiA+IHVzZXINCj4gPiA+IHdlIGp1c3Qg
dXNlIHRoZSBjcmVkcyBvZiB0aGUgb3JpZ2luYWwgbW91bnRlciBvZiBORlM/DQo+ID4gPiANCj4g
PiA+IElmIE5GUyBkb2VzIGhhdmUgZGlmZmVyZW50IGF1dG9tb3VudCBiZWhhdmlvciBkZXBlbmRp
bmcgb24gdGhlDQo+ID4gPiB1c2VyDQo+ID4gPiAob3VjaCEpIHdlIG5lZWQgdG8gZ28gdGhyb3Vn
aCB0aGUgY2FsbCBwYXRoIGFuZCBzZWUgd2hlcmUgaXQNCj4gPiA+IG1ha2VzDQo+ID4gPiBzZW5z
ZSB0byBvdmVyIHJpZGUgdGhpbmdzIGFuZCB3aGVyZSBpdCBkb2VzIG5vdC4NCj4gPiANCj4gPiBU
aGUgcmVhc29uIHdoeSB0aGUgTkZTIGNsaWVudCBjcmVhdGVzIGEgbW91bnRwb2ludCBpcyBiZWNh
dXNlIG9uDQo+ID4gZW50ZXJpbmcgYSBkaXJlY3RvcnksIGl0IGRldGVjdHMgdGhhdCB0aGVyZSBp
cyBlaXRoZXIgYSBzaW1pbGFyDQo+ID4gbW91bnRwb2ludCBvbiB0aGUgc2VydmVyLCBvciB0aGVy
ZSBpcyBhIHJlZmVycmFsICh3aGljaCBhY3RzIHNvcnQNCj4gPiBvZg0KPiA+IGxpa2UgYSBzeW1s
aW5rLCBleGNlcHQgaXQgcG9pbnRzIHRvIGEgcGF0aCBvbiBvbmUgb3IgbW9yZSBkaWZmZXJlbnQN
Cj4gPiBORlMNCj4gPiBzZXJ2ZXJzKS4NCj4gPiBXaXRob3V0IHRoYXQgbW91bnRwb2ludCwgbW9z
dCB0aGluZ3Mgd291bGQgd29yaywgYnV0IHRoZSB1c2VyIHdvdWxkDQo+ID4gZW5kDQo+ID4gdXAg
c2VlaW5nIG5hc3R5IG5vbi1wb3NpeCBmZWF0dXJlcyBsaWtlIGR1cGxpY2F0ZSBpbm9kZSBudW1i
ZXJzLg0KPiA+IA0KPiA+IFdlIGRvIG5vdCB3YW50IHRvIHVzZSBhbnkgY3JlZHMgb3RoZXIgdGhh
biB1c2VyIGNyZWRzIGhlcmUsIGJlY2F1c2UNCj4gPiBhcw0KPiA+IGZhciBhcyB0aGUgc2VjdXJp
dHkgbW9kZWwgaXMgY29uY2VybmVkLCB0aGUgcHJvY2VzcyBpcyBqdXN0DQo+ID4gY3Jvc3NpbmcN
Cj4gPiBpbnRvIGFuIGV4aXN0aW5nIGRpcmVjdG9yeS4NCj4gDQo+IEJ1dCBzZ2V0IG5lZWRzIGRp
ZmZlcmVudCBjcmVkcy4NCj4gDQo+IEJlY2F1c2UgdGhlIHVzZXIgd2hvIGF1dGhvcml6ZXMgdGhl
IG1vdW50aW5nIG9mIHRoZSBmaWxlc3lzdGVtIGlzDQo+IGRpZmZlcmVudCB0aGFuIHRoZSB1c2Vy
IHdobyBpcyBjcm9zc2luZyBpbnRvIHRoZSBuZXcgZmlsZXN5c3RlbS4NCj4gDQo+IFRoZSBsb2Nh
bCBzeXN0ZW0gbm93IGNhcmVzIGFib3V0IHRoYXQgZGlzdGluY3Rpb24gZXZlbiBpZiB0aGUgbmZz
DQo+IHNlcnZlcg0KPiBkb2VzIG5vdC4NCg0KV2h5PyBUaGUgZmlsZXN5c3RlbSBpcyBhbHJlYWR5
IG1vdW50ZWQuIFdlJ3JlIGNyZWF0aW5nIGEgbmV3DQptb3VudHBvaW50LCBidXQgd2UgY291bGQg
ZXF1YWxseSB3ZWxsIGp1c3Qgc2F5ICdzb2QgdGhhdCcgYW5kIGNyZWF0ZSBhbg0Kb3JkaW5hcnkg
ZGlyZWN0b3J5LiBUaGUgcGVuYWx0eSB3b3VsZCBiZSBhZm9yZW1lbnRpb25lZCBub24tcG9zaXgN
CndlaXJkbmVzcy4NCg0KPiANCj4gPiA+IFNldGggdGhlIGZ1bmRhbWVudGFsIHByb2JsZW0gd2l0
aCB5b3VyIHBhdGNoIHdhcyB0aGF0IHlvdSB3ZXJlDQo+ID4gPiBwYXRjaGluZw0KPiA+ID4gYSBs
b2NhdGlvbiB0aGF0IGlzIHVzZWQgZm9yIG1vcmUganVzdCBtb3VudHMuDQo+ID4gPiANCj4gPiA+
IEkgYW0gc3Ryb25nbHkgd2lzaGluZyB0aGF0IHdlIGNvdWxkIGp1c3QgY2hhbmdlIGZvbGxvd19h
dXRvbW91bnQNCj4gPiA+IGZyb206DQo+ID4gPiANCj4gPiA+IA0KPiA+ID4gCW9sZF9jcmVkID0g
b3ZlcnJpZGVfY3JlZHMoJmluaXRfY3JlZCk7DQo+ID4gPiAJbW50ID0gcGF0aC0+ZGVudHJ5LT5k
X29wLT5kX2F1dG9tb3VudChwYXRoKTsNCj4gPiA+IAlyZXZlcnRfY3JlZHMob2xkX2NyZWQpOw0K
PiA+ID4gDQo+ID4gPiB0bzoNCj4gPiA+IA0KPiA+ID4gCW9sZF9jcmVkID0gb3ZlcnJpZGVfY3Jl
ZHMocGF0aC0+bW50LT5tbnRfc2ItPnNfY3JlZCk7DQo+ID4gPiAJbW50ID0gcGF0aC0+ZGVudHJ5
LT5kX29wLT5kX2F1dG9tb3VudChwYXRoKTsNCj4gPiA+IAlyZXZlcnRfY3JlZHMob2xkX2NyZWQp
Ow0KPiA+ID4gDQo+ID4gPiBBbmQgYWxsIHdpbGwgYmUgd2VsbCB3aXRoIG5mcy7CoMKgVGhhdCBk
b2VzIHJlbWFpbiBwb3NzaWJsZS4NCj4gPiANCj4gPiBOby4gVGhhdCB3b3VsZCBicmVhayBwZXJt
aXNzaW9ucyBjaGVja2luZy4gU2VlIGFib3ZlLg0KPiANCj4gVGhlbiB3ZSBuZWVkIHRvIGxvb2sg
bXVjaCBoYXJkZXIgYXQgdGhlIHBlcm1pc3Npb24gY2hlY2tpbmcNCj4gbW9kZWwgb2YgZF9hdXRv
bW91bnQgYmVjYXVzZSB3ZSBuZWVkIHRvIHBlcm1pc3Npb24gY2hlY2sgYWdhaW5zdA0KPiBib3Ro
IHNldHMgb2YgY3JlZHMuDQo+IA0KPiA+ID4gQnV0IGxvb2tpbmcgYXQgdGhlIGNvZGUgcGF0aCB5
b3UgdG91Y2hlZCBpdCBzZWVtcyB0byBsb29rdXAgdGhlDQo+ID4gPiBjcmVkDQo+ID4gPiBiYXNl
ZCBwdXJlbHkgb24gdGhlIGxvY2FsIHVpZCwgZ2lkLCBhbmQgZ3JvdXBzLsKgwqBXaGljaCBzdWdn
ZXN0cw0KPiA+ID4gdG8NCj4gPiA+IG1lIHRoYXQgZXZlbiB0aGUgb3JpZ2luYWwgbW91bnRlcnMg
Y3JlZHMgbWF5IG5vdCBiZSBlbm91Z2ggOigNCj4gPiA+IA0KPiA+ID4gQXQgd2hpY2ggcG9pbnQg
SSBhbSBub3QgY2VydGFpbiBvZiB0aGUgc29sdXRpb24uwqDCoEJ1dCBJIGZlYXIgdGhhdA0KPiA+
ID4gbGlrZQ0KPiA+ID4gYXV0b2ZzIE5GUyBhY3R1YWxseSBjYXJlcyB3aGljaCB1c2VyIGlzIHRy
YW5zaXRpb24gdGhlIG1hZ2ljDQo+ID4gPiBtb3VudHBvaW50LA0KPiA+ID4gYW5kIG1heSByZXR1
cm4gZGlmZmVyZW50IGRhdGEgZGVwZW5kaW5nIG9uIHdobyB0cmFuc2l0aW9ucyB0aGUNCj4gPiA+
IG1vdW50cG9pbnQgZmlyc3QuwqDCoEljayHCoMKgTmFzdHkgTmFzdHkgSWNrIQ0KPiA+ID4gDQo+
ID4gDQo+ID4gVGhlIHNlY3VyaXR5IG1vZGVsIGlzIHRoZSBzYW1lIGFzIHRoYXQgb2YgYW4gb3Jk
aW5hcnkgZGlyZWN0b3J5Lg0KPiA+IFRoZQ0KPiA+IG9ubHkgZGlmZmVyZW5jZSBpcyB0aGF0IHdl
IGNyZWF0ZSBhIG5ldyBzdXBlcmJsb2NrLiBXaHkgaXMgdGhhdA0KPiA+ICJJY2siPw0KPiANCj4g
VGhlIEljayBpcyBpZiB0aGUgc3VwZXJibG9jayB0aGF0IGlzIGNyZWF0ZWQgZGVwZW5kcyBvbiB0
aGUgdXNlcg0KPiBjcm9zc2luZyB0aGUgbW91bnRwb2ludC4NCj4gDQo+IEFLQSBEbyB3ZSBnZXQg
YSBkaWZmZXJlbnQgbW91bnRwb2ludCBpZiB1c2VyIEEgdHJpZ2dlcnMgdGhlIGF1dG9tb3VudA0K
PiB2cyB1c2VyIEIgdHJpZ2dlcmluZyB0aGUgYXV0b21vdW50Pw0KDQpZb3UgbWF5IGdldCBhbiBF
QUNDRVMuIE90aGVyd2lzZSB0aGUgbW91bnRwb2ludHMgc2hvdWxkIGJlIHRoZSBzYW1lLg0KDQo+
IElmIHRoZSBwcm9ibGVtIGlzIGp1c3Qgcm9vdCBzcXVhc2ggYW5kIG5vdCBsZXR0aW5nIHJvb3Qg
dHJpZ2dlciB0aGUNCj4gYXV0b21vdW50IGl0IGlzIG11Y2ggbGVzcyBpY2suwqDCoFdlaXJkIGJ1
dCBub3QgaWNrLg0KDQpOb3QganVzdCByb290IHNxdWFzaCwgYnV0IGdlbmVyYWwgcGVybWlzc2lv
bnMgY2hlY2tpbmcuIFdlIGRvbid0IHdhbnQNCnRvIGdpdmUgdXNlciBCIHJvb3QgcHJpdmlsZWdl
cyBhbGxvd2luZyBjcm9zc2luZyBpbnRvIGEgZGlyZWN0b3J5IHRvDQp3aGljaCBzL2hlIHdvdWxk
IG9yZGluYXJpbHkgaGF2ZSBubyByaWdodHMgZWl0aGVyLg0KDQotLSANClRyb25kIE15a2xlYnVz
dA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVi
dXN0QHByaW1hcnlkYXRhLmNvbQ0K

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

On Wed, Jan 25, 2017 at 12:14:26AM +0000, Trond Myklebust wrote:
> Adding David Howells and Steve French as I believe both AFS and CIFS
> have the exact same requirements and NFS here.
> 
> On Wed, 2017-01-25 at 12:56 +1300, Eric W. Biederman wrote:
> > Trond Myklebust <trondmy@primarydata.com> writes:
> > 
> > > On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
> > > > With respect to nfs and automounts.
> > > > 
> > > > Does NFS have different automount behavior based on the user
> > > > performing the automount?
> > > > 
> > > > If NFS does not have different automount behavior depending on
> > > > the
> > > > user
> > > > we just use the creds of the original mounter of NFS?
> > > > 
> > > > If NFS does have different automount behavior depending on the
> > > > user
> > > > (ouch!) we need to go through the call path and see where it
> > > > makes
> > > > sense to over ride things and where it does not.
> > > 
> > > The reason why the NFS client creates a mountpoint is because on
> > > entering a directory, it detects that there is either a similar
> > > mountpoint on the server, or there is a referral (which acts sort
> > > of
> > > like a symlink, except it points to a path on one or more different
> > > NFS
> > > servers).
> > > Without that mountpoint, most things would work, but the user would
> > > end
> > > up seeing nasty non-posix features like duplicate inode numbers.
> > > 
> > > We do not want to use any creds other than user creds here, because
> > > as
> > > far as the security model is concerned, the process is just
> > > crossing
> > > into an existing directory.
> > 
> > But sget needs different creds.
> > 
> > Because the user who authorizes the mounting of the filesystem is
> > different than the user who is crossing into the new filesystem.
> > 
> > The local system now cares about that distinction even if the nfs
> > server
> > does not.
> 
> Why? The filesystem is already mounted. We're creating a new
> mountpoint, but we could equally well just say 'sod that' and create an
> ordinary directory. The penalty would be aforementioned non-posix
> weirdness.
> 
> > 
> > > > Seth the fundamental problem with your patch was that you were
> > > > patching
> > > > a location that is used for more just mounts.
> > > > 
> > > > I am strongly wishing that we could just change follow_automount
> > > > from:
> > > > 
> > > > 
> > > > 	old_cred = override_creds(&init_cred);
> > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > 	revert_creds(old_cred);
> > > > 
> > > > to:
> > > > 
> > > > 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
> > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > 	revert_creds(old_cred);
> > > > 
> > > > And all will be well with nfs.  That does remain possible.
> > > 
> > > No. That would break permissions checking. See above.
> > 
> > Then we need to look much harder at the permission checking
> > model of d_automount because we need to permission check against
> > both sets of creds.

How about something like this? Essentially, stash the creds used at
mount time in the super block, then create a vfs_kern_automount()
function which overrides the currend creds then calls vfs_kern_mount().

Only compile tested so far, and probably it should be split up into
several patches.

diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index 81dd075356b9..4455e64610d3 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -202,7 +202,7 @@ static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
 
 	/* try and do the mount */
 	_debug("--- attempting mount %s -o %s ---", devname, options);
-	mnt = vfs_kern_mount(&afs_fs_type, 0, devname, options);
+	mnt = vfs_kern_automount(mntpt, &afs_fs_type, 0, devname, options);
 	_debug("--- mount result %p ---", mnt);
 
 	free_page((unsigned long) devname);
diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index ec9dbbcca3b9..d378f88e8630 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -245,9 +245,10 @@ char *cifs_compose_mount_options(const char *sb_mountdata,
  * @fullpath:		full path in UNC format
  * @ref:		server's referral
  */
-static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb,
+static struct vfsmount *cifs_dfs_do_refmount(struct dentry *mntpt,
 		const char *fullpath, const struct dfs_info3_param *ref)
 {
+	struct cifs_sb_info *cifs_sb = CIFS_SB(mntpt->d_sb);
 	struct vfsmount *mnt;
 	char *mountdata;
 	char *devname = NULL;
@@ -259,7 +260,7 @@ static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb,
 	if (IS_ERR(mountdata))
 		return (struct vfsmount *)mountdata;
 
-	mnt = vfs_kern_mount(&cifs_fs_type, 0, devname, mountdata);
+	mnt = vfs_kern_automount(mntpt, &cifs_fs_type, 0, devname, mountdata);
 	kfree(mountdata);
 	kfree(devname);
 	return mnt;
@@ -334,7 +335,7 @@ static struct vfsmount *cifs_dfs_do_automount(struct dentry *mntpt)
 			mnt = ERR_PTR(-EINVAL);
 			break;
 		}
-		mnt = cifs_dfs_do_refmount(cifs_sb,
+		mnt = cifs_dfs_do_refmount(mntpt,
 				full_path, referrals + i);
 		cifs_dbg(FYI, "%s: cifs_dfs_do_refmount:%s , mnt:%p\n",
 			 __func__, referrals[i].node_name, mnt);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f17fcf89e18e..d45f131af869 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -187,9 +187,9 @@ static const struct super_operations debugfs_super_operations = {
 
 static struct vfsmount *debugfs_automount(struct path *path)
 {
-	struct vfsmount *(*f)(void *);
-	f = (struct vfsmount *(*)(void *))path->dentry->d_fsdata;
-	return f(d_inode(path->dentry)->i_private);
+	struct vfsmount *(*f)(struct dentry *, void *);
+	f = (struct vfsmount *(*)(struct dentry *, void *))path->dentry->d_fsdata;
+	return f(path->dentry, d_inode(path->dentry)->i_private);
 }
 
 static const struct dentry_operations debugfs_dops = {
@@ -504,7 +504,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
  */
 struct dentry *debugfs_create_automount(const char *name,
 					struct dentry *parent,
-					struct vfsmount *(*f)(void *),
+					struct vfsmount *(*f)(struct dentry *, void *),
 					void *data)
 {
 	struct dentry *dentry = start_creating(name, parent);
diff --git a/fs/namespace.c b/fs/namespace.c
index 487ba30bb5c6..da7e6dfa56cb 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void
 }
 EXPORT_SYMBOL_GPL(vfs_kern_mount);
 
+struct vfsmount *
+vfs_kern_automount(struct dentry *dentry, struct file_system_type *type,
+		   int flags, const char *name, void *data)
+{
+	const struct cred *old_cred;
+	struct vfsmount *mnt;
+
+	old_cred = override_creds(dentry->d_sb->s_cred);
+	mnt = vfs_kern_mount(type, flags, name, data);
+	revert_creds(old_cred);
+
+	return mnt;
+}
+EXPORT_SYMBOL_GPL(vfs_kern_automount);
+
 static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 					int flag)
 {
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 5551e8ef67fd..dce529c50cbd 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -222,11 +222,11 @@ void nfs_release_automount_timer(void)
 /*
  * Clone a mountpoint of the appropriate type
  */
-static struct vfsmount *nfs_do_clone_mount(struct nfs_server *server,
+static struct vfsmount *nfs_do_clone_mount(struct dentry *dentry,
 					   const char *devname,
 					   struct nfs_clone_mount *mountdata)
 {
-	return vfs_kern_mount(&nfs_xdev_fs_type, 0, devname, mountdata);
+	return vfs_kern_automount(dentry, &nfs_xdev_fs_type, 0, devname, mountdata);
 }
 
 /**
@@ -261,7 +261,7 @@ struct vfsmount *nfs_do_submount(struct dentry *dentry, struct nfs_fh *fh,
 	mnt = (struct vfsmount *)devname;
 	if (IS_ERR(devname))
 		goto free_page;
-	mnt = nfs_do_clone_mount(NFS_SB(dentry->d_sb), devname, &mountdata);
+	mnt = nfs_do_clone_mount(dentry, devname, &mountdata);
 free_page:
 	free_page((unsigned long)page);
 out:
diff --git a/fs/super.c b/fs/super.c
index 1709ed029a2c..b33b7d606aec 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -166,6 +166,7 @@ static void destroy_super(struct super_block *s)
 	list_lru_destroy(&s->s_inode_lru);
 	security_sb_free(s);
 	WARN_ON(!list_empty(&s->s_mounts));
+	put_cred(s->s_cred);
 	put_user_ns(s->s_user_ns);
 	kfree(s->s_subtype);
 	kfree(s->s_options);
@@ -193,6 +194,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 
 	INIT_LIST_HEAD(&s->s_mounts);
 	s->s_user_ns = get_user_ns(user_ns);
+	s->s_cred = get_current_cred();
 
 	if (security_sb_alloc(s))
 		goto fail;
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 014cc564d1c4..67ac9f10e7b7 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -99,7 +99,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 
 struct dentry *debugfs_create_automount(const char *name,
 					struct dentry *parent,
-					struct vfsmount *(*f)(void *),
+					struct vfsmount *(*f)(struct dentry *, void *),
 					void *data);
 
 void debugfs_remove(struct dentry *dentry);
@@ -211,7 +211,7 @@ static inline struct dentry *debugfs_create_symlink(const char *name,
 
 static inline struct dentry *debugfs_create_automount(const char *name,
 					struct dentry *parent,
-					struct vfsmount *(*f)(void *),
+					struct vfsmount *(*f)(struct dentry *, void *),
 					void *data)
 {
 	return ERR_PTR(-ENODEV);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba074328894..cae845944d1d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1374,6 +1374,9 @@ struct super_block {
 	 */
 	struct user_namespace *s_user_ns;
 
+	/* Credentials of the user who mounted the filesystem */
+	const struct cred *s_cred;
+
 	/*
 	 * Keep the lru lists last in the structure so they always sit on their
 	 * own individual cachelines.
diff --git a/include/linux/mount.h b/include/linux/mount.h
index c6f55158d5e5..b9cdca0c6b1a 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -90,6 +90,9 @@ struct file_system_type;
 extern struct vfsmount *vfs_kern_mount(struct file_system_type *type,
 				      int flags, const char *name,
 				      void *data);
+struct vfsmount *vfs_kern_automount(struct dentry *dentry,
+				    struct file_system_type *type,
+				    int flags, const char *name, void *data);
 
 extern void mnt_set_expiry(struct vfsmount *mnt, struct list_head *expiry_list);
 extern void mark_mounts_for_expiry(struct list_head *mounts);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d7449783987a..7bb959abd5f1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7503,7 +7503,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 	ftrace_init_tracefs(tr, d_tracer);
 }
 
-static struct vfsmount *trace_automount(void *ingore)
+static struct vfsmount *trace_automount(struct dentry *dentry, void *ingore)
 {
 	struct vfsmount *mnt;
 	struct file_system_type *type;
@@ -7516,7 +7516,7 @@ static struct vfsmount *trace_automount(void *ingore)
 	type = get_fs_type("tracefs");
 	if (!type)
 		return NULL;
-	mnt = vfs_kern_mount(type, 0, "tracefs", NULL);
+	mnt = vfs_kern_automount(dentry, type, 0, "tracefs", NULL);
 	put_filesystem(type);
 	if (IS_ERR(mnt))
 		return NULL;

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Trond Myklebust]

T24gV2VkLCAyMDE3LTAxLTI1IGF0IDA4OjUyIC0wNjAwLCBTZXRoIEZvcnNoZWUgd3JvdGU6DQo+
IE9uIFdlZCwgSmFuIDI1LCAyMDE3IGF0IDEyOjE0OjI2QU0gKzAwMDAsIFRyb25kIE15a2xlYnVz
dCB3cm90ZToNCj4gPiBBZGRpbmcgRGF2aWQgSG93ZWxscyBhbmQgU3RldmUgRnJlbmNoIGFzIEkg
YmVsaWV2ZSBib3RoIEFGUyBhbmQNCj4gPiBDSUZTDQo+ID4gaGF2ZSB0aGUgZXhhY3Qgc2FtZSBy
ZXF1aXJlbWVudHMgYW5kIE5GUyBoZXJlLg0KPiA+IA0KPiA+IE9uIFdlZCwgMjAxNy0wMS0yNSBh
dCAxMjo1NiArMTMwMCwgRXJpYyBXLiBCaWVkZXJtYW4gd3JvdGU6DQo+ID4gPiBUcm9uZCBNeWts
ZWJ1c3QgPHRyb25kbXlAcHJpbWFyeWRhdGEuY29tPiB3cml0ZXM6DQo+ID4gPiANCj4gPiA+ID4g
T24gV2VkLCAyMDE3LTAxLTI1IGF0IDEyOjI4ICsxMzAwLCBFcmljIFcuIEJpZWRlcm1hbiB3cm90
ZToNCj4gPiA+ID4gPiBXaXRoIHJlc3BlY3QgdG8gbmZzIGFuZCBhdXRvbW91bnRzLg0KPiA+ID4g
PiA+IA0KPiA+ID4gPiA+IERvZXMgTkZTIGhhdmUgZGlmZmVyZW50IGF1dG9tb3VudCBiZWhhdmlv
ciBiYXNlZCBvbiB0aGUgdXNlcg0KPiA+ID4gPiA+IHBlcmZvcm1pbmcgdGhlIGF1dG9tb3VudD8N
Cj4gPiA+ID4gPiANCj4gPiA+ID4gPiBJZiBORlMgZG9lcyBub3QgaGF2ZSBkaWZmZXJlbnQgYXV0
b21vdW50IGJlaGF2aW9yIGRlcGVuZGluZw0KPiA+ID4gPiA+IG9uDQo+ID4gPiA+ID4gdGhlDQo+
ID4gPiA+ID4gdXNlcg0KPiA+ID4gPiA+IHdlIGp1c3QgdXNlIHRoZSBjcmVkcyBvZiB0aGUgb3Jp
Z2luYWwgbW91bnRlciBvZiBORlM/DQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gSWYgTkZTIGRvZXMg
aGF2ZSBkaWZmZXJlbnQgYXV0b21vdW50IGJlaGF2aW9yIGRlcGVuZGluZyBvbg0KPiA+ID4gPiA+
IHRoZQ0KPiA+ID4gPiA+IHVzZXINCj4gPiA+ID4gPiAob3VjaCEpIHdlIG5lZWQgdG8gZ28gdGhy
b3VnaCB0aGUgY2FsbCBwYXRoIGFuZCBzZWUgd2hlcmUgaXQNCj4gPiA+ID4gPiBtYWtlcw0KPiA+
ID4gPiA+IHNlbnNlIHRvIG92ZXIgcmlkZSB0aGluZ3MgYW5kIHdoZXJlIGl0IGRvZXMgbm90Lg0K
PiA+ID4gPiANCj4gPiA+ID4gVGhlIHJlYXNvbiB3aHkgdGhlIE5GUyBjbGllbnQgY3JlYXRlcyBh
IG1vdW50cG9pbnQgaXMgYmVjYXVzZQ0KPiA+ID4gPiBvbg0KPiA+ID4gPiBlbnRlcmluZyBhIGRp
cmVjdG9yeSwgaXQgZGV0ZWN0cyB0aGF0IHRoZXJlIGlzIGVpdGhlciBhIHNpbWlsYXINCj4gPiA+
ID4gbW91bnRwb2ludCBvbiB0aGUgc2VydmVyLCBvciB0aGVyZSBpcyBhIHJlZmVycmFsICh3aGlj
aCBhY3RzDQo+ID4gPiA+IHNvcnQNCj4gPiA+ID4gb2YNCj4gPiA+ID4gbGlrZSBhIHN5bWxpbmss
IGV4Y2VwdCBpdCBwb2ludHMgdG8gYSBwYXRoIG9uIG9uZSBvciBtb3JlDQo+ID4gPiA+IGRpZmZl
cmVudA0KPiA+ID4gPiBORlMNCj4gPiA+ID4gc2VydmVycykuDQo+ID4gPiA+IFdpdGhvdXQgdGhh
dCBtb3VudHBvaW50LCBtb3N0IHRoaW5ncyB3b3VsZCB3b3JrLCBidXQgdGhlIHVzZXINCj4gPiA+
ID4gd291bGQNCj4gPiA+ID4gZW5kDQo+ID4gPiA+IHVwIHNlZWluZyBuYXN0eSBub24tcG9zaXgg
ZmVhdHVyZXMgbGlrZSBkdXBsaWNhdGUgaW5vZGUNCj4gPiA+ID4gbnVtYmVycy4NCj4gPiA+ID4g
DQo+ID4gPiA+IFdlIGRvIG5vdCB3YW50IHRvIHVzZSBhbnkgY3JlZHMgb3RoZXIgdGhhbiB1c2Vy
IGNyZWRzIGhlcmUsDQo+ID4gPiA+IGJlY2F1c2UNCj4gPiA+ID4gYXMNCj4gPiA+ID4gZmFyIGFz
IHRoZSBzZWN1cml0eSBtb2RlbCBpcyBjb25jZXJuZWQsIHRoZSBwcm9jZXNzIGlzIGp1c3QNCj4g
PiA+ID4gY3Jvc3NpbmcNCj4gPiA+ID4gaW50byBhbiBleGlzdGluZyBkaXJlY3RvcnkuDQo+ID4g
PiANCj4gPiA+IEJ1dCBzZ2V0IG5lZWRzIGRpZmZlcmVudCBjcmVkcy4NCj4gPiA+IA0KPiA+ID4g
QmVjYXVzZSB0aGUgdXNlciB3aG8gYXV0aG9yaXplcyB0aGUgbW91bnRpbmcgb2YgdGhlIGZpbGVz
eXN0ZW0gaXMNCj4gPiA+IGRpZmZlcmVudCB0aGFuIHRoZSB1c2VyIHdobyBpcyBjcm9zc2luZyBp
bnRvIHRoZSBuZXcgZmlsZXN5c3RlbS4NCj4gPiA+IA0KPiA+ID4gVGhlIGxvY2FsIHN5c3RlbSBu
b3cgY2FyZXMgYWJvdXQgdGhhdCBkaXN0aW5jdGlvbiBldmVuIGlmIHRoZSBuZnMNCj4gPiA+IHNl
cnZlcg0KPiA+ID4gZG9lcyBub3QuDQo+ID4gDQo+ID4gV2h5PyBUaGUgZmlsZXN5c3RlbSBpcyBh
bHJlYWR5IG1vdW50ZWQuIFdlJ3JlIGNyZWF0aW5nIGEgbmV3DQo+ID4gbW91bnRwb2ludCwgYnV0
IHdlIGNvdWxkIGVxdWFsbHkgd2VsbCBqdXN0IHNheSAnc29kIHRoYXQnIGFuZA0KPiA+IGNyZWF0
ZSBhbg0KPiA+IG9yZGluYXJ5IGRpcmVjdG9yeS4gVGhlIHBlbmFsdHkgd291bGQgYmUgYWZvcmVt
ZW50aW9uZWQgbm9uLXBvc2l4DQo+ID4gd2VpcmRuZXNzLg0KPiA+IA0KPiA+ID4gDQo+ID4gPiA+
ID4gU2V0aCB0aGUgZnVuZGFtZW50YWwgcHJvYmxlbSB3aXRoIHlvdXIgcGF0Y2ggd2FzIHRoYXQg
eW91DQo+ID4gPiA+ID4gd2VyZQ0KPiA+ID4gPiA+IHBhdGNoaW5nDQo+ID4gPiA+ID4gYSBsb2Nh
dGlvbiB0aGF0IGlzIHVzZWQgZm9yIG1vcmUganVzdCBtb3VudHMuDQo+ID4gPiA+ID4gDQo+ID4g
PiA+ID4gSSBhbSBzdHJvbmdseSB3aXNoaW5nIHRoYXQgd2UgY291bGQganVzdCBjaGFuZ2UNCj4g
PiA+ID4gPiBmb2xsb3dfYXV0b21vdW50DQo+ID4gPiA+ID4gZnJvbToNCj4gPiA+ID4gPiANCj4g
PiA+ID4gPiANCj4gPiA+ID4gPiAJb2xkX2NyZWQgPSBvdmVycmlkZV9jcmVkcygmaW5pdF9jcmVk
KTsNCj4gPiA+ID4gPiAJbW50ID0gcGF0aC0+ZGVudHJ5LT5kX29wLT5kX2F1dG9tb3VudChwYXRo
KTsNCj4gPiA+ID4gPiAJcmV2ZXJ0X2NyZWRzKG9sZF9jcmVkKTsNCj4gPiA+ID4gPiANCj4gPiA+
ID4gPiB0bzoNCj4gPiA+ID4gPiANCj4gPiA+ID4gPiAJb2xkX2NyZWQgPSBvdmVycmlkZV9jcmVk
cyhwYXRoLT5tbnQtPm1udF9zYi0+c19jcmVkKTsNCj4gPiA+ID4gPiAJbW50ID0gcGF0aC0+ZGVu
dHJ5LT5kX29wLT5kX2F1dG9tb3VudChwYXRoKTsNCj4gPiA+ID4gPiAJcmV2ZXJ0X2NyZWRzKG9s
ZF9jcmVkKTsNCj4gPiA+ID4gPiANCj4gPiA+ID4gPiBBbmQgYWxsIHdpbGwgYmUgd2VsbCB3aXRo
IG5mcy7CoMKgVGhhdCBkb2VzIHJlbWFpbiBwb3NzaWJsZS4NCj4gPiA+ID4gDQo+ID4gPiA+IE5v
LiBUaGF0IHdvdWxkIGJyZWFrIHBlcm1pc3Npb25zIGNoZWNraW5nLiBTZWUgYWJvdmUuDQo+ID4g
PiANCj4gPiA+IFRoZW4gd2UgbmVlZCB0byBsb29rIG11Y2ggaGFyZGVyIGF0IHRoZSBwZXJtaXNz
aW9uIGNoZWNraW5nDQo+ID4gPiBtb2RlbCBvZiBkX2F1dG9tb3VudCBiZWNhdXNlIHdlIG5lZWQg
dG8gcGVybWlzc2lvbiBjaGVjayBhZ2FpbnN0DQo+ID4gPiBib3RoIHNldHMgb2YgY3JlZHMuDQo+
IA0KPiBIb3cgYWJvdXQgc29tZXRoaW5nIGxpa2UgdGhpcz8gRXNzZW50aWFsbHksIHN0YXNoIHRo
ZSBjcmVkcyB1c2VkIGF0DQo+IG1vdW50IHRpbWUgaW4gdGhlIHN1cGVyIGJsb2NrLCB0aGVuIGNy
ZWF0ZSBhIHZmc19rZXJuX2F1dG9tb3VudCgpDQo+IGZ1bmN0aW9uIHdoaWNoIG92ZXJyaWRlcyB0
aGUgY3VycmVuZCBjcmVkcyB0aGVuIGNhbGxzDQo+IHZmc19rZXJuX21vdW50KCkuDQo+IA0KPiBP
bmx5IGNvbXBpbGUgdGVzdGVkIHNvIGZhciwgYW5kIHByb2JhYmx5IGl0IHNob3VsZCBiZSBzcGxp
dCB1cCBpbnRvDQo+IHNldmVyYWwgcGF0Y2hlcy4NCg0KPiBkaWZmIC0tZ2l0IGEvZnMvbmFtZXNw
YWNlLmMgYi9mcy9uYW1lc3BhY2UuYw0KPiBpbmRleCA0ODdiYTMwYmI1YzYuLmRhN2U2ZGZhNTZj
YiAxMDA2NDQNCj4gLS0tIGEvZnMvbmFtZXNwYWNlLmMNCj4gKysrIGIvZnMvbmFtZXNwYWNlLmMN
Cj4gQEAgLTk4OSw2ICs5ODksMjEgQEAgdmZzX2tlcm5fbW91bnQoc3RydWN0IGZpbGVfc3lzdGVt
X3R5cGUgKnR5cGUsDQo+IGludCBmbGFncywgY29uc3QgY2hhciAqbmFtZSwgdm9pZA0KPiDCoH0N
Cj4gwqBFWFBPUlRfU1lNQk9MX0dQTCh2ZnNfa2Vybl9tb3VudCk7DQo+IMKgDQo+ICtzdHJ1Y3Qg
dmZzbW91bnQgKg0KPiArdmZzX2tlcm5fYXV0b21vdW50KHN0cnVjdCBkZW50cnkgKmRlbnRyeSwg
c3RydWN0IGZpbGVfc3lzdGVtX3R5cGUNCj4gKnR5cGUsDQo+ICsJCcKgwqDCoGludCBmbGFncywg
Y29uc3QgY2hhciAqbmFtZSwgdm9pZCAqZGF0YSkNCj4gK3sNCj4gKwljb25zdCBzdHJ1Y3QgY3Jl
ZCAqb2xkX2NyZWQ7DQo+ICsJc3RydWN0IHZmc21vdW50ICptbnQ7DQo+ICsNCj4gKwlvbGRfY3Jl
ZCA9IG92ZXJyaWRlX2NyZWRzKGRlbnRyeS0+ZF9zYi0+c19jcmVkKTsNCj4gKwltbnQgPSB2ZnNf
a2Vybl9tb3VudCh0eXBlLCBmbGFncywgbmFtZSwgZGF0YSk7DQo+ICsJcmV2ZXJ0X2NyZWRzKG9s
ZF9jcmVkKTsNCj4gKw0KPiArCXJldHVybiBtbnQ7DQo+ICt9DQo+ICtFWFBPUlRfU1lNQk9MX0dQ
TCh2ZnNfa2Vybl9hdXRvbW91bnQpOw0KPiArDQoNCk5vcGUuIEFzIEkgc2FpZCwgdGhlIGNhbGwg
aW50byB0aGUgZmlsZXN5c3RlbSBuZWVkcyB0aGUgX3VzZXJfIGNyZWRzLg0KDQotLSA=

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee]

On Wed, Jan 25, 2017 at 03:51:30PM +0000, Trond Myklebust wrote:
> On Wed, 2017-01-25 at 08:52 -0600, Seth Forshee wrote:
> > On Wed, Jan 25, 2017 at 12:14:26AM +0000, Trond Myklebust wrote:
> > > Adding David Howells and Steve French as I believe both AFS and
> > > CIFS
> > > have the exact same requirements and NFS here.
> > > 
> > > On Wed, 2017-01-25 at 12:56 +1300, Eric W. Biederman wrote:
> > > > Trond Myklebust <trondmy@primarydata.com> writes:
> > > > 
> > > > > On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
> > > > > > With respect to nfs and automounts.
> > > > > > 
> > > > > > Does NFS have different automount behavior based on the user
> > > > > > performing the automount?
> > > > > > 
> > > > > > If NFS does not have different automount behavior depending
> > > > > > on
> > > > > > the
> > > > > > user
> > > > > > we just use the creds of the original mounter of NFS?
> > > > > > 
> > > > > > If NFS does have different automount behavior depending on
> > > > > > the
> > > > > > user
> > > > > > (ouch!) we need to go through the call path and see where it
> > > > > > makes
> > > > > > sense to over ride things and where it does not.
> > > > > 
> > > > > The reason why the NFS client creates a mountpoint is because
> > > > > on
> > > > > entering a directory, it detects that there is either a similar
> > > > > mountpoint on the server, or there is a referral (which acts
> > > > > sort
> > > > > of
> > > > > like a symlink, except it points to a path on one or more
> > > > > different
> > > > > NFS
> > > > > servers).
> > > > > Without that mountpoint, most things would work, but the user
> > > > > would
> > > > > end
> > > > > up seeing nasty non-posix features like duplicate inode
> > > > > numbers.
> > > > > 
> > > > > We do not want to use any creds other than user creds here,
> > > > > because
> > > > > as
> > > > > far as the security model is concerned, the process is just
> > > > > crossing
> > > > > into an existing directory.
> > > > 
> > > > But sget needs different creds.
> > > > 
> > > > Because the user who authorizes the mounting of the filesystem is
> > > > different than the user who is crossing into the new filesystem.
> > > > 
> > > > The local system now cares about that distinction even if the nfs
> > > > server
> > > > does not.
> > > 
> > > Why? The filesystem is already mounted. We're creating a new
> > > mountpoint, but we could equally well just say 'sod that' and
> > > create an
> > > ordinary directory. The penalty would be aforementioned non-posix
> > > weirdness.
> > > 
> > > > 
> > > > > > Seth the fundamental problem with your patch was that you
> > > > > > were
> > > > > > patching
> > > > > > a location that is used for more just mounts.
> > > > > > 
> > > > > > I am strongly wishing that we could just change
> > > > > > follow_automount
> > > > > > from:
> > > > > > 
> > > > > > 
> > > > > > 	old_cred = override_creds(&init_cred);
> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > > > 	revert_creds(old_cred);
> > > > > > 
> > > > > > to:
> > > > > > 
> > > > > > 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > > > 	revert_creds(old_cred);
> > > > > > 
> > > > > > And all will be well with nfs.  That does remain possible.
> > > > > 
> > > > > No. That would break permissions checking. See above.
> > > > 
> > > > Then we need to look much harder at the permission checking
> > > > model of d_automount because we need to permission check against
> > > > both sets of creds.
> > 
> > How about something like this? Essentially, stash the creds used at
> > mount time in the super block, then create a vfs_kern_automount()
> > function which overrides the currend creds then calls
> > vfs_kern_mount().
> > 
> > Only compile tested so far, and probably it should be split up into
> > several patches.
> 
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 487ba30bb5c6..da7e6dfa56cb 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type,
> > int flags, const char *name, void
> >  }
> >  EXPORT_SYMBOL_GPL(vfs_kern_mount);
> >  
> > +struct vfsmount *
> > +vfs_kern_automount(struct dentry *dentry, struct file_system_type
> > *type,
> > +		   int flags, const char *name, void *data)
> > +{
> > +	const struct cred *old_cred;
> > +	struct vfsmount *mnt;
> > +
> > +	old_cred = override_creds(dentry->d_sb->s_cred);
> > +	mnt = vfs_kern_mount(type, flags, name, data);
> > +	revert_creds(old_cred);
> > +
> > +	return mnt;
> > +}
> > +EXPORT_SYMBOL_GPL(vfs_kern_automount);
> > +
> 
> Nope. As I said, the call into the filesystem needs the _user_ creds.

Okay. I thought that perhaps the user's creds were only needed when
looking up the mountpoint, and that after that it might be okay to
switch the creds. I guess not.

Thanks,
Seth

Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Wed, Jan 25, 2017 at 03:51:30PM +0000, Trond Myklebust wrote:
>> On Wed, 2017-01-25 at 08:52 -0600, Seth Forshee wrote:
>> > On Wed, Jan 25, 2017 at 12:14:26AM +0000, Trond Myklebust wrote:
>> > > Adding David Howells and Steve French as I believe both AFS and
>> > > CIFS
>> > > have the exact same requirements and NFS here.
>> > > 
>> > > On Wed, 2017-01-25 at 12:56 +1300, Eric W. Biederman wrote:
>> > > > Trond Myklebust <trondmy@primarydata.com> writes:
>> > > > 
>> > > > > On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
>> > > > > > With respect to nfs and automounts.
>> > > > > > 
>> > > > > > Does NFS have different automount behavior based on the user
>> > > > > > performing the automount?
>> > > > > > 
>> > > > > > If NFS does not have different automount behavior depending
>> > > > > > on
>> > > > > > the
>> > > > > > user
>> > > > > > we just use the creds of the original mounter of NFS?
>> > > > > > 
>> > > > > > If NFS does have different automount behavior depending on
>> > > > > > the
>> > > > > > user
>> > > > > > (ouch!) we need to go through the call path and see where it
>> > > > > > makes
>> > > > > > sense to over ride things and where it does not.
>> > > > > 
>> > > > > The reason why the NFS client creates a mountpoint is because
>> > > > > on
>> > > > > entering a directory, it detects that there is either a similar
>> > > > > mountpoint on the server, or there is a referral (which acts
>> > > > > sort
>> > > > > of
>> > > > > like a symlink, except it points to a path on one or more
>> > > > > different
>> > > > > NFS
>> > > > > servers).
>> > > > > Without that mountpoint, most things would work, but the user
>> > > > > would
>> > > > > end
>> > > > > up seeing nasty non-posix features like duplicate inode
>> > > > > numbers.
>> > > > > 
>> > > > > We do not want to use any creds other than user creds here,
>> > > > > because
>> > > > > as
>> > > > > far as the security model is concerned, the process is just
>> > > > > crossing
>> > > > > into an existing directory.
>> > > > 
>> > > > But sget needs different creds.
>> > > > 
>> > > > Because the user who authorizes the mounting of the filesystem is
>> > > > different than the user who is crossing into the new filesystem.
>> > > > 
>> > > > The local system now cares about that distinction even if the nfs
>> > > > server
>> > > > does not.
>> > > 
>> > > Why? The filesystem is already mounted. We're creating a new
>> > > mountpoint, but we could equally well just say 'sod that' and
>> > > create an
>> > > ordinary directory. The penalty would be aforementioned non-posix
>> > > weirdness.
>> > > 
>> > > > 
>> > > > > > Seth the fundamental problem with your patch was that you
>> > > > > > were
>> > > > > > patching
>> > > > > > a location that is used for more just mounts.
>> > > > > > 
>> > > > > > I am strongly wishing that we could just change
>> > > > > > follow_automount
>> > > > > > from:
>> > > > > > 
>> > > > > > 
>> > > > > > 	old_cred = override_creds(&init_cred);
>> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
>> > > > > > 	revert_creds(old_cred);
>> > > > > > 
>> > > > > > to:
>> > > > > > 
>> > > > > > 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
>> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
>> > > > > > 	revert_creds(old_cred);
>> > > > > > 
>> > > > > > And all will be well with nfs.  That does remain possible.
>> > > > > 
>> > > > > No. That would break permissions checking. See above.
>> > > > 
>> > > > Then we need to look much harder at the permission checking
>> > > > model of d_automount because we need to permission check against
>> > > > both sets of creds.
>> > 
>> > How about something like this? Essentially, stash the creds used at
>> > mount time in the super block, then create a vfs_kern_automount()
>> > function which overrides the currend creds then calls
>> > vfs_kern_mount().
>> > 
>> > Only compile tested so far, and probably it should be split up into
>> > several patches.
>> 
>> > diff --git a/fs/namespace.c b/fs/namespace.c
>> > index 487ba30bb5c6..da7e6dfa56cb 100644
>> > --- a/fs/namespace.c
>> > +++ b/fs/namespace.c
>> > @@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type,
>> > int flags, const char *name, void
>> >  }
>> >  EXPORT_SYMBOL_GPL(vfs_kern_mount);
>> >  
>> > +struct vfsmount *
>> > +vfs_kern_automount(struct dentry *dentry, struct file_system_type
>> > *type,
>> > +		   int flags, const char *name, void *data)
>> > +{
>> > +	const struct cred *old_cred;
>> > +	struct vfsmount *mnt;
>> > +
>> > +	old_cred = override_creds(dentry->d_sb->s_cred);
>> > +	mnt = vfs_kern_mount(type, flags, name, data);
>> > +	revert_creds(old_cred);
>> > +
>> > +	return mnt;
>> > +}
>> > +EXPORT_SYMBOL_GPL(vfs_kern_automount);
>> > +
>> 
>> Nope. As I said, the call into the filesystem needs the _user_ creds.
>
> Okay. I thought that perhaps the user's creds were only needed when
> looking up the mountpoint, and that after that it might be okay to
> switch the creds. I guess not.

So I agree that for the case of submounts that we don't need an
additional permission check as the initial mount of the filesystem
is that permission check already.

I can see some interesting corner cases where a permission check might
be required at the point when we have unprivileged mounts with submounts
in the mix.  As sget might find a filesystem that was already mounted
elsewhere.  But we don't have to face that case today.

What is needs to be fixed are the permission checks cause problems for
existing filesystems with submounts.

What we need to fix those is knowledge the mount is happening for a
submount/automount.  At which point it is simple to turn off the
permission check.  Knowledge that a mount is a submount and not the
ordinary kind of mount is going to require something like Seth's patch.

So after having gone off to a corner and thought about this I believe
I have come up with a minimal patch that makes things good for everyone.

Eric

[REVIEW][PATCH] fs: Better permission checking for submounts

[Eric W. Biederman]

To support unprivileged users mounting filesystems two permission
checks have to be performed: a test to see if the user allowed to
create a mount in the mount namespace, and a test to see if
the user is allowed to access the specified filesystem.

The automount case is special in that mounting the original filesystem
grants permission to mount the sub-filesystems, to any user who
happens to stumble across the their mountpoint and satisfies the
ordinary filesystem permission checks.

Attempting to handle the automount case by using override_creds
almost works.  It preserves the idea that permission to mount
the original filesystem is permission to mount the sub-filesystem.
Unfortunately using override_creds messes up the filesystems
ordinary permission checks.

Solve this by being explicit that a mount is a submount by introducing
vfs_submount, and using it where appropriate.

vfs_submount uses a new mount internal mount flags MS_SUBMOUNT, to let
sget and friends know that a mount is a submount so they can take appropriate
action.

sget and sget_userns are modified to not perform any permission checks
on submounts.

follow_automount is modified to stop using override_creds as that
has proven problemantic.

do_mount is modified to always remove the new MS_SUBMOUNT flag so
that we know userspace will never by able to specify it.

autofs4 is modified to stop using current_real_cred that was put in
there to handle the previous version of submount permission checking.

cifs is modified to pass the mountpoint all of the way down to vfs_submount.

debugfs is modified to pass the mountpoint all of the way down to
trace_automount by adding a new parameter.  To make this change easier
a new typedef debugfs_automount_t is introduced to capture the type of
the debugfs automount function.

Cc: stable@vger.kernel.org
Fixes: 069d5ac9ae0d ("autofs:  Fix automounts by using current_real_cred()->uid")
Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/afs/mntpt.c          |  2 +-
 fs/autofs4/waitq.c      |  4 ++--
 fs/cifs/cifs_dfs_ref.c  |  7 ++++---
 fs/debugfs/inode.c      |  8 ++++----
 fs/namei.c              |  3 ---
 fs/namespace.c          | 17 ++++++++++++++++-
 fs/nfs/namespace.c      |  2 +-
 fs/nfs/nfs4namespace.c  |  2 +-
 fs/super.c              | 13 ++++++++++---
 include/linux/debugfs.h |  3 ++-
 include/linux/mount.h   |  3 +++
 include/uapi/linux/fs.h |  1 +
 kernel/trace/trace.c    |  4 ++--
 13 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index 81dd075356b9..d4fb0afc0097 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -202,7 +202,7 @@ static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
 
 	/* try and do the mount */
 	_debug("--- attempting mount %s -o %s ---", devname, options);
-	mnt = vfs_kern_mount(&afs_fs_type, 0, devname, options);
+	mnt = vfs_submount(mntpt, &afs_fs_type, devname, options);
 	_debug("--- mount result %p ---", mnt);
 
 	free_page((unsigned long) devname);
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 1278335ce366..79fbd85db4ba 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -436,8 +436,8 @@ int autofs4_wait(struct autofs_sb_info *sbi,
 		memcpy(&wq->name, &qstr, sizeof(struct qstr));
 		wq->dev = autofs4_get_dev(sbi);
 		wq->ino = autofs4_get_ino(sbi);
-		wq->uid = current_real_cred()->uid;
-		wq->gid = current_real_cred()->gid;
+		wq->uid = current_cred()->uid;
+		wq->gid = current_cred()->gid;
 		wq->pid = pid;
 		wq->tgid = tgid;
 		wq->status = -EINTR; /* Status return if interrupted */
diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index ec9dbbcca3b9..9156be545b0f 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -245,7 +245,8 @@ char *cifs_compose_mount_options(const char *sb_mountdata,
  * @fullpath:		full path in UNC format
  * @ref:		server's referral
  */
-static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb,
+static struct vfsmount *cifs_dfs_do_refmount(struct dentry *mntpt,
+		struct cifs_sb_info *cifs_sb,
 		const char *fullpath, const struct dfs_info3_param *ref)
 {
 	struct vfsmount *mnt;
@@ -259,7 +260,7 @@ static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb,
 	if (IS_ERR(mountdata))
 		return (struct vfsmount *)mountdata;
 
-	mnt = vfs_kern_mount(&cifs_fs_type, 0, devname, mountdata);
+	mnt = vfs_submount(mntpt, &cifs_fs_type, devname, mountdata);
 	kfree(mountdata);
 	kfree(devname);
 	return mnt;
@@ -334,7 +335,7 @@ static struct vfsmount *cifs_dfs_do_automount(struct dentry *mntpt)
 			mnt = ERR_PTR(-EINVAL);
 			break;
 		}
-		mnt = cifs_dfs_do_refmount(cifs_sb,
+		mnt = cifs_dfs_do_refmount(mntpt, cifs_sb,
 				full_path, referrals + i);
 		cifs_dbg(FYI, "%s: cifs_dfs_do_refmount:%s , mnt:%p\n",
 			 __func__, referrals[i].node_name, mnt);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f17fcf89e18e..1e30f74a9527 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -187,9 +187,9 @@ static const struct super_operations debugfs_super_operations = {
 
 static struct vfsmount *debugfs_automount(struct path *path)
 {
-	struct vfsmount *(*f)(void *);
-	f = (struct vfsmount *(*)(void *))path->dentry->d_fsdata;
-	return f(d_inode(path->dentry)->i_private);
+	debugfs_automount_t f;
+	f = (debugfs_automount_t)path->dentry->d_fsdata;
+	return f(path->dentry, d_inode(path->dentry)->i_private);
 }
 
 static const struct dentry_operations debugfs_dops = {
@@ -504,7 +504,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
  */
 struct dentry *debugfs_create_automount(const char *name,
 					struct dentry *parent,
-					struct vfsmount *(*f)(void *),
+					debugfs_automount_t f,
 					void *data)
 {
 	struct dentry *dentry = start_creating(name, parent);
diff --git a/fs/namei.c b/fs/namei.c
index 6fa3e9138fe4..da689c9c005e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1100,7 +1100,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			    bool *need_mntput)
 {
 	struct vfsmount *mnt;
-	const struct cred *old_cred;
 	int err;
 
 	if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
@@ -1129,9 +1128,7 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 	if (nd->total_link_count >= 40)
 		return -ELOOP;
 
-	old_cred = override_creds(&init_cred);
 	mnt = path->dentry->d_op->d_automount(path);
-	revert_creds(old_cred);
 	if (IS_ERR(mnt)) {
 		/*
 		 * The filesystem is allowed to return -EISDIR here to indicate
diff --git a/fs/namespace.c b/fs/namespace.c
index 487ba30bb5c6..089a6b23135a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void
 }
 EXPORT_SYMBOL_GPL(vfs_kern_mount);
 
+struct vfsmount *
+vfs_submount(const struct dentry *mountpoint, struct file_system_type *type,
+	     const char *name, void *data)
+{
+	/* Until it is worked out how to pass the user namespace
+	 * through from the parent mount to the submount don't support
+	 * unprivileged mounts with submounts.
+	 */
+	if (mountpoint->d_sb->s_user_ns != &init_user_ns)
+		return ERR_PTR(-EPERM);
+
+	return vfs_kern_mount(type, MS_SUBMOUNT, name, data);
+}
+EXPORT_SYMBOL_GPL(vfs_submount);
+
 static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 					int flag)
 {
@@ -2794,7 +2809,7 @@ long do_mount(const char *dev_name, const char __user *dir_name,
 
 	flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE | MS_BORN |
 		   MS_NOATIME | MS_NODIRATIME | MS_RELATIME| MS_KERNMOUNT |
-		   MS_STRICTATIME | MS_NOREMOTELOCK);
+		   MS_STRICTATIME | MS_NOREMOTELOCK | MS_SUBMOUNT);
 
 	if (flags & MS_REMOUNT)
 		retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 5551e8ef67fd..e49d831c4e85 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -226,7 +226,7 @@ static struct vfsmount *nfs_do_clone_mount(struct nfs_server *server,
 					   const char *devname,
 					   struct nfs_clone_mount *mountdata)
 {
-	return vfs_kern_mount(&nfs_xdev_fs_type, 0, devname, mountdata);
+	return vfs_submount(mountdata->dentry, &nfs_xdev_fs_type, devname, mountdata);
 }
 
 /**
diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
index d21104912676..d8b040bd9814 100644
--- a/fs/nfs/nfs4namespace.c
+++ b/fs/nfs/nfs4namespace.c
@@ -279,7 +279,7 @@ static struct vfsmount *try_location(struct nfs_clone_mount *mountdata,
 				mountdata->hostname,
 				mountdata->mnt_path);
 
-		mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0, page, mountdata);
+		mnt = vfs_submount(mountdata->dentry, &nfs4_referral_fs_type, page, mountdata);
 		if (!IS_ERR(mnt))
 			break;
 	}
diff --git a/fs/super.c b/fs/super.c
index 1709ed029a2c..4185844f7a12 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -469,7 +469,7 @@ struct super_block *sget_userns(struct file_system_type *type,
 	struct super_block *old;
 	int err;
 
-	if (!(flags & MS_KERNMOUNT) &&
+	if (!(flags & (MS_KERNMOUNT|MS_SUBMOUNT)) &&
 	    !(type->fs_flags & FS_USERNS_MOUNT) &&
 	    !capable(CAP_SYS_ADMIN))
 		return ERR_PTR(-EPERM);
@@ -499,7 +499,7 @@ struct super_block *sget_userns(struct file_system_type *type,
 	}
 	if (!s) {
 		spin_unlock(&sb_lock);
-		s = alloc_super(type, flags, user_ns);
+		s = alloc_super(type, (flags & ~MS_SUBMOUNT), user_ns);
 		if (!s)
 			return ERR_PTR(-ENOMEM);
 		goto retry;
@@ -540,8 +540,15 @@ struct super_block *sget(struct file_system_type *type,
 {
 	struct user_namespace *user_ns = current_user_ns();
 
+	/* We don't yet pass the user namespace of the parent
+	 * mount through to here so always use &init_user_ns
+	 * until that changes.
+	 */
+	if (flags & MS_SUBMOUNT)
+		user_ns = &init_user_ns;
+
 	/* Ensure the requestor has permissions over the target filesystem */
-	if (!(flags & MS_KERNMOUNT) && !ns_capable(user_ns, CAP_SYS_ADMIN))
+	if (!(flags & (MS_KERNMOUNT|MS_SUBMOUNT)) && !ns_capable(user_ns, CAP_SYS_ADMIN))
 		return ERR_PTR(-EPERM);
 
 	return sget_userns(type, test, set, flags, user_ns, data);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 014cc564d1c4..233006be30aa 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -97,9 +97,10 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);
 struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *dest);
 
+typedef struct vfsmount *(*debugfs_automount_t)(struct dentry *, void *);
 struct dentry *debugfs_create_automount(const char *name,
 					struct dentry *parent,
-					struct vfsmount *(*f)(void *),
+					debugfs_automount_t f,
 					void *data);
 
 void debugfs_remove(struct dentry *dentry);
diff --git a/include/linux/mount.h b/include/linux/mount.h
index c6f55158d5e5..8e0352af06b7 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -90,6 +90,9 @@ struct file_system_type;
 extern struct vfsmount *vfs_kern_mount(struct file_system_type *type,
 				      int flags, const char *name,
 				      void *data);
+extern struct vfsmount *vfs_submount(const struct dentry *mountpoint,
+				     struct file_system_type *type,
+				     const char *name, void *data);
 
 extern void mnt_set_expiry(struct vfsmount *mnt, struct list_head *expiry_list);
 extern void mark_mounts_for_expiry(struct list_head *mounts);
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 36da93fbf188..048a85e9f017 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -132,6 +132,7 @@ struct inodes_stat_t {
 #define MS_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
 
 /* These sb flags are internal to the kernel */
+#define MS_SUBMOUNT     (1<<26)
 #define MS_NOREMOTELOCK	(1<<27)
 #define MS_NOSEC	(1<<28)
 #define MS_BORN		(1<<29)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d7449783987a..310f0ea0d1a2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7503,7 +7503,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
 	ftrace_init_tracefs(tr, d_tracer);
 }
 
-static struct vfsmount *trace_automount(void *ingore)
+static struct vfsmount *trace_automount(struct dentry *mntpt, void *ingore)
 {
 	struct vfsmount *mnt;
 	struct file_system_type *type;
@@ -7516,7 +7516,7 @@ static struct vfsmount *trace_automount(void *ingore)
 	type = get_fs_type("tracefs");
 	if (!type)
 		return NULL;
-	mnt = vfs_kern_mount(type, 0, "tracefs", NULL);
+	mnt = vfs_submount(mntpt, type, "tracefs", NULL);
 	put_filesystem(type);
 	if (IS_ERR(mnt))
 		return NULL;
-- 
2.10.1

Re: [REVIEW][PATCH] fs: Better permission checking for submounts

[Trond Myklebust]

On Wed, 2017-02-01 at 19:38 +1300, Eric W. Biederman wrote:
> To support unprivileged users mounting filesystems two permission
> checks have to be performed: a test to see if the user allowed to
> create a mount in the mount namespace, and a test to see if
> the user is allowed to access the specified filesystem.
> 
> The automount case is special in that mounting the original
> filesystem
> grants permission to mount the sub-filesystems, to any user who
> happens to stumble across the their mountpoint and satisfies the
> ordinary filesystem permission checks.
> 
> Attempting to handle the automount case by using override_creds
> almost works.  It preserves the idea that permission to mount
> the original filesystem is permission to mount the sub-filesystem.
> Unfortunately using override_creds messes up the filesystems
> ordinary permission checks.
> 
> Solve this by being explicit that a mount is a submount by
> introducing
> vfs_submount, and using it where appropriate.
> 
> vfs_submount uses a new mount internal mount flags MS_SUBMOUNT, to
> let
> sget and friends know that a mount is a submount so they can take
> appropriate
> action.
> 
> sget and sget_userns are modified to not perform any permission
> checks
> on submounts.
> 
> follow_automount is modified to stop using override_creds as that
> has proven problemantic.
> 
> do_mount is modified to always remove the new MS_SUBMOUNT flag so
> that we know userspace will never by able to specify it.
> 
> autofs4 is modified to stop using current_real_cred that was put in
> there to handle the previous version of submount permission checking.
> 
> cifs is modified to pass the mountpoint all of the way down to
> vfs_submount.
> 
> debugfs is modified to pass the mountpoint all of the way down to
> trace_automount by adding a new parameter.  To make this change
> easier
> a new typedef debugfs_automount_t is introduced to capture the type
> of
> the debugfs automount function.
> 
> Cc: stable@vger.kernel.org
> Fixes: 069d5ac9ae0d ("autofs:  Fix automounts by using
> current_real_cred()->uid")
> Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> creds")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/afs/mntpt.c          |  2 +-
>  fs/autofs4/waitq.c      |  4 ++--
>  fs/cifs/cifs_dfs_ref.c  |  7 ++++---
>  fs/debugfs/inode.c      |  8 ++++----
>  fs/namei.c              |  3 ---
>  fs/namespace.c          | 17 ++++++++++++++++-
>  fs/nfs/namespace.c      |  2 +-
>  fs/nfs/nfs4namespace.c  |  2 +-
>  fs/super.c              | 13 ++++++++++---
>  include/linux/debugfs.h |  3 ++-
>  include/linux/mount.h   |  3 +++
>  include/uapi/linux/fs.h |  1 +
>  kernel/trace/trace.c    |  4 ++--
>  13 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
> index 81dd075356b9..d4fb0afc0097 100644
> --- a/fs/afs/mntpt.c
> +++ b/fs/afs/mntpt.c
> @@ -202,7 +202,7 @@ static struct vfsmount
> *afs_mntpt_do_automount(struct dentry *mntpt)
>  
>  	/* try and do the mount */
>  	_debug("--- attempting mount %s -o %s ---", devname,
> options);
> -	mnt = vfs_kern_mount(&afs_fs_type, 0, devname, options);
> +	mnt = vfs_submount(mntpt, &afs_fs_type, devname, options);
>  	_debug("--- mount result %p ---", mnt);
>  
>  	free_page((unsigned long) devname);
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index 1278335ce366..79fbd85db4ba 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -436,8 +436,8 @@ int autofs4_wait(struct autofs_sb_info *sbi,
>  		memcpy(&wq->name, &qstr, sizeof(struct qstr));
>  		wq->dev = autofs4_get_dev(sbi);
>  		wq->ino = autofs4_get_ino(sbi);
> -		wq->uid = current_real_cred()->uid;
> -		wq->gid = current_real_cred()->gid;
> +		wq->uid = current_cred()->uid;
> +		wq->gid = current_cred()->gid;
>  		wq->pid = pid;
>  		wq->tgid = tgid;
>  		wq->status = -EINTR; /* Status return if interrupted
> */
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index ec9dbbcca3b9..9156be545b0f 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -245,7 +245,8 @@ char *cifs_compose_mount_options(const char
> *sb_mountdata,
>   * @fullpath:		full path in UNC format
>   * @ref:		server's referral
>   */
> -static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info
> *cifs_sb,
> +static struct vfsmount *cifs_dfs_do_refmount(struct dentry *mntpt,
> +		struct cifs_sb_info *cifs_sb,
>  		const char *fullpath, const struct dfs_info3_param
> *ref)
>  {
>  	struct vfsmount *mnt;
> @@ -259,7 +260,7 @@ static struct vfsmount
> *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb,
>  	if (IS_ERR(mountdata))
>  		return (struct vfsmount *)mountdata;
>  
> -	mnt = vfs_kern_mount(&cifs_fs_type, 0, devname, mountdata);
> +	mnt = vfs_submount(mntpt, &cifs_fs_type, devname,
> mountdata);
>  	kfree(mountdata);
>  	kfree(devname);
>  	return mnt;
> @@ -334,7 +335,7 @@ static struct vfsmount
> *cifs_dfs_do_automount(struct dentry *mntpt)
>  			mnt = ERR_PTR(-EINVAL);
>  			break;
>  		}
> -		mnt = cifs_dfs_do_refmount(cifs_sb,
> +		mnt = cifs_dfs_do_refmount(mntpt, cifs_sb,
>  				full_path, referrals + i);
>  		cifs_dbg(FYI, "%s: cifs_dfs_do_refmount:%s ,
> mnt:%p\n",
>  			 __func__, referrals[i].node_name, mnt);
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index f17fcf89e18e..1e30f74a9527 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -187,9 +187,9 @@ static const struct super_operations
> debugfs_super_operations = {
>  
>  static struct vfsmount *debugfs_automount(struct path *path)
>  {
> -	struct vfsmount *(*f)(void *);
> -	f = (struct vfsmount *(*)(void *))path->dentry->d_fsdata;
> -	return f(d_inode(path->dentry)->i_private);
> +	debugfs_automount_t f;
> +	f = (debugfs_automount_t)path->dentry->d_fsdata;
> +	return f(path->dentry, d_inode(path->dentry)->i_private);
>  }
>  
>  static const struct dentry_operations debugfs_dops = {
> @@ -504,7 +504,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
>   */
>  struct dentry *debugfs_create_automount(const char *name,
>  					struct dentry *parent,
> -					struct vfsmount *(*f)(void
> *),
> +					debugfs_automount_t f,
>  					void *data)
>  {
>  	struct dentry *dentry = start_creating(name, parent);
> diff --git a/fs/namei.c b/fs/namei.c
> index 6fa3e9138fe4..da689c9c005e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1100,7 +1100,6 @@ static int follow_automount(struct path *path,
> struct nameidata *nd,
>  			    bool *need_mntput)
>  {
>  	struct vfsmount *mnt;
> -	const struct cred *old_cred;
>  	int err;
>  
>  	if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
> @@ -1129,9 +1128,7 @@ static int follow_automount(struct path *path,
> struct nameidata *nd,
>  	if (nd->total_link_count >= 40)
>  		return -ELOOP;
>  
> -	old_cred = override_creds(&init_cred);
>  	mnt = path->dentry->d_op->d_automount(path);
> -	revert_creds(old_cred);
>  	if (IS_ERR(mnt)) {
>  		/*
>  		 * The filesystem is allowed to return -EISDIR here
> to indicate
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 487ba30bb5c6..089a6b23135a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type,
> int flags, const char *name, void
>  }
>  EXPORT_SYMBOL_GPL(vfs_kern_mount);
>  
> +struct vfsmount *
> +vfs_submount(const struct dentry *mountpoint, struct
> file_system_type *type,
> +	     const char *name, void *data)
> +{
> +	/* Until it is worked out how to pass the user namespace
> +	 * through from the parent mount to the submount don't
> support
> +	 * unprivileged mounts with submounts.
> +	 */
> +	if (mountpoint->d_sb->s_user_ns != &init_user_ns)
> +		return ERR_PTR(-EPERM);
> +
> +	return vfs_kern_mount(type, MS_SUBMOUNT, name, data);
> +}
> +EXPORT_SYMBOL_GPL(vfs_submount);
> +
>  static struct mount *clone_mnt(struct mount *old, struct dentry
> *root,
>  					int flag)
>  {
> @@ -2794,7 +2809,7 @@ long do_mount(const char *dev_name, const char
> __user *dir_name,
>  
>  	flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE |
> MS_BORN |
>  		   MS_NOATIME | MS_NODIRATIME | MS_RELATIME|
> MS_KERNMOUNT |
> -		   MS_STRICTATIME | MS_NOREMOTELOCK);
> +		   MS_STRICTATIME | MS_NOREMOTELOCK | MS_SUBMOUNT);
>  
>  	if (flags & MS_REMOUNT)
>  		retval = do_remount(&path, flags & ~MS_REMOUNT,
> mnt_flags,
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index 5551e8ef67fd..e49d831c4e85 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -226,7 +226,7 @@ static struct vfsmount *nfs_do_clone_mount(struct
> nfs_server *server,
>  					   const char *devname,
>  					   struct nfs_clone_mount
> *mountdata)
>  {
> -	return vfs_kern_mount(&nfs_xdev_fs_type, 0, devname,
> mountdata);
> +	return vfs_submount(mountdata->dentry, &nfs_xdev_fs_type,
> devname, mountdata);
>  }
>  
>  /**
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index d21104912676..d8b040bd9814 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -279,7 +279,7 @@ static struct vfsmount *try_location(struct
> nfs_clone_mount *mountdata,
>  				mountdata->hostname,
>  				mountdata->mnt_path);
>  
> -		mnt = vfs_kern_mount(&nfs4_referral_fs_type, 0,
> page, mountdata);
> +		mnt = vfs_submount(mountdata->dentry,
> &nfs4_referral_fs_type, page, mountdata);
>  		if (!IS_ERR(mnt))
>  			break;
>  	}
> diff --git a/fs/super.c b/fs/super.c
> index 1709ed029a2c..4185844f7a12 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -469,7 +469,7 @@ struct super_block *sget_userns(struct
> file_system_type *type,
>  	struct super_block *old;
>  	int err;
>  
> -	if (!(flags & MS_KERNMOUNT) &&
> +	if (!(flags & (MS_KERNMOUNT|MS_SUBMOUNT)) &&
>  	    !(type->fs_flags & FS_USERNS_MOUNT) &&
>  	    !capable(CAP_SYS_ADMIN))
>  		return ERR_PTR(-EPERM);
> @@ -499,7 +499,7 @@ struct super_block *sget_userns(struct
> file_system_type *type,
>  	}
>  	if (!s) {
>  		spin_unlock(&sb_lock);
> -		s = alloc_super(type, flags, user_ns);
> +		s = alloc_super(type, (flags & ~MS_SUBMOUNT),
> user_ns);
>  		if (!s)
>  			return ERR_PTR(-ENOMEM);
>  		goto retry;
> @@ -540,8 +540,15 @@ struct super_block *sget(struct file_system_type
> *type,
>  {
>  	struct user_namespace *user_ns = current_user_ns();
>  
> +	/* We don't yet pass the user namespace of the parent
> +	 * mount through to here so always use &init_user_ns
> +	 * until that changes.
> +	 */
> +	if (flags & MS_SUBMOUNT)
> +		user_ns = &init_user_ns;
> +
>  	/* Ensure the requestor has permissions over the target
> filesystem */
> -	if (!(flags & MS_KERNMOUNT) && !ns_capable(user_ns,
> CAP_SYS_ADMIN))
> +	if (!(flags & (MS_KERNMOUNT|MS_SUBMOUNT)) &&
> !ns_capable(user_ns, CAP_SYS_ADMIN))
>  		return ERR_PTR(-EPERM);
>  
>  	return sget_userns(type, test, set, flags, user_ns, data);
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 014cc564d1c4..233006be30aa 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -97,9 +97,10 @@ struct dentry *debugfs_create_dir(const char
> *name, struct dentry *parent);
>  struct dentry *debugfs_create_symlink(const char *name, struct
> dentry *parent,
>  				      const char *dest);
>  
> +typedef struct vfsmount *(*debugfs_automount_t)(struct dentry *,
> void *);
>  struct dentry *debugfs_create_automount(const char *name,
>  					struct dentry *parent,
> -					struct vfsmount *(*f)(void
> *),
> +					debugfs_automount_t f,
>  					void *data);
>  
>  void debugfs_remove(struct dentry *dentry);
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index c6f55158d5e5..8e0352af06b7 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -90,6 +90,9 @@ struct file_system_type;
>  extern struct vfsmount *vfs_kern_mount(struct file_system_type
> *type,
>  				      int flags, const char *name,
>  				      void *data);
> +extern struct vfsmount *vfs_submount(const struct dentry
> *mountpoint,
> +				     struct file_system_type *type,
> +				     const char *name, void *data);
>  
>  extern void mnt_set_expiry(struct vfsmount *mnt, struct list_head
> *expiry_list);
>  extern void mark_mounts_for_expiry(struct list_head *mounts);
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 36da93fbf188..048a85e9f017 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -132,6 +132,7 @@ struct inodes_stat_t {
>  #define MS_LAZYTIME	(1<<25) /* Update the on-disk [acm]times
> lazily */
>  
>  /* These sb flags are internal to the kernel */
> +#define MS_SUBMOUNT     (1<<26)
>  #define MS_NOREMOTELOCK	(1<<27)
>  #define MS_NOSEC	(1<<28)
>  #define MS_BORN		(1<<29)
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d7449783987a..310f0ea0d1a2 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7503,7 +7503,7 @@ init_tracer_tracefs(struct trace_array *tr,
> struct dentry *d_tracer)
>  	ftrace_init_tracefs(tr, d_tracer);
>  }
>  
> -static struct vfsmount *trace_automount(void *ingore)
> +static struct vfsmount *trace_automount(struct dentry *mntpt, void
> *ingore)
>  {
>  	struct vfsmount *mnt;
>  	struct file_system_type *type;
> @@ -7516,7 +7516,7 @@ static struct vfsmount *trace_automount(void
> *ingore)
>  	type = get_fs_type("tracefs");
>  	if (!type)
>  		return NULL;
> -	mnt = vfs_kern_mount(type, 0, "tracefs", NULL);
> +	mnt = vfs_submount(mntpt, type, "tracefs", NULL);
>  	put_filesystem(type);
>  	if (IS_ERR(mnt))
>  		return NULL;

Yes. That looks like it might be workable.

Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com>
-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: [REVIEW][PATCH] fs: Better permission checking for submounts

[Trond Myklebust]

T24gV2VkLCAyMDE3LTAyLTAxIGF0IDE5OjM4ICsxMzAwLCBFcmljIFcuIEJpZWRlcm1hbiB3cm90
ZToNCj4gVG8gc3VwcG9ydCB1bnByaXZpbGVnZWQgdXNlcnMgbW91bnRpbmcgZmlsZXN5c3RlbXMg
dHdvIHBlcm1pc3Npb24NCj4gY2hlY2tzIGhhdmUgdG8gYmUgcGVyZm9ybWVkOiBhIHRlc3QgdG8g
c2VlIGlmIHRoZSB1c2VyIGFsbG93ZWQgdG8NCj4gY3JlYXRlIGEgbW91bnQgaW4gdGhlIG1vdW50
IG5hbWVzcGFjZSwgYW5kIGEgdGVzdCB0byBzZWUgaWYNCj4gdGhlIHVzZXIgaXMgYWxsb3dlZCB0
byBhY2Nlc3MgdGhlIHNwZWNpZmllZCBmaWxlc3lzdGVtLg0KPiANCj4gVGhlIGF1dG9tb3VudCBj
YXNlIGlzIHNwZWNpYWwgaW4gdGhhdCBtb3VudGluZyB0aGUgb3JpZ2luYWwNCj4gZmlsZXN5c3Rl
bQ0KPiBncmFudHMgcGVybWlzc2lvbiB0byBtb3VudCB0aGUgc3ViLWZpbGVzeXN0ZW1zLCB0byBh
bnkgdXNlciB3aG8NCj4gaGFwcGVucyB0byBzdHVtYmxlIGFjcm9zcyB0aGUgdGhlaXIgbW91bnRw
b2ludCBhbmQgc2F0aXNmaWVzIHRoZQ0KPiBvcmRpbmFyeSBmaWxlc3lzdGVtIHBlcm1pc3Npb24g
Y2hlY2tzLg0KPiANCj4gQXR0ZW1wdGluZyB0byBoYW5kbGUgdGhlIGF1dG9tb3VudCBjYXNlIGJ5
IHVzaW5nIG92ZXJyaWRlX2NyZWRzDQo+IGFsbW9zdCB3b3Jrcy7CoMKgSXQgcHJlc2VydmVzIHRo
ZSBpZGVhIHRoYXQgcGVybWlzc2lvbiB0byBtb3VudA0KPiB0aGUgb3JpZ2luYWwgZmlsZXN5c3Rl
bSBpcyBwZXJtaXNzaW9uIHRvIG1vdW50IHRoZSBzdWItZmlsZXN5c3RlbS4NCj4gVW5mb3J0dW5h
dGVseSB1c2luZyBvdmVycmlkZV9jcmVkcyBtZXNzZXMgdXAgdGhlIGZpbGVzeXN0ZW1zDQo+IG9y
ZGluYXJ5IHBlcm1pc3Npb24gY2hlY2tzLg0KPiANCj4gU29sdmUgdGhpcyBieSBiZWluZyBleHBs
aWNpdCB0aGF0IGEgbW91bnQgaXMgYSBzdWJtb3VudCBieQ0KPiBpbnRyb2R1Y2luZw0KPiB2ZnNf
c3VibW91bnQsIGFuZCB1c2luZyBpdCB3aGVyZSBhcHByb3ByaWF0ZS4NCj4gDQo+IHZmc19zdWJt
b3VudCB1c2VzIGEgbmV3IG1vdW50IGludGVybmFsIG1vdW50IGZsYWdzIE1TX1NVQk1PVU5ULCB0
bw0KPiBsZXQNCj4gc2dldCBhbmQgZnJpZW5kcyBrbm93IHRoYXQgYSBtb3VudCBpcyBhIHN1Ym1v
dW50IHNvIHRoZXkgY2FuIHRha2UNCj4gYXBwcm9wcmlhdGUNCj4gYWN0aW9uLg0KPiANCj4gc2dl
dCBhbmQgc2dldF91c2VybnMgYXJlIG1vZGlmaWVkIHRvIG5vdCBwZXJmb3JtIGFueSBwZXJtaXNz
aW9uDQo+IGNoZWNrcw0KPiBvbiBzdWJtb3VudHMuDQo+IA0KPiBmb2xsb3dfYXV0b21vdW50IGlz
IG1vZGlmaWVkIHRvIHN0b3AgdXNpbmcgb3ZlcnJpZGVfY3JlZHMgYXMgdGhhdA0KPiBoYXMgcHJv
dmVuIHByb2JsZW1hbnRpYy4NCj4gDQo+IGRvX21vdW50IGlzIG1vZGlmaWVkIHRvIGFsd2F5cyBy
ZW1vdmUgdGhlIG5ldyBNU19TVUJNT1VOVCBmbGFnIHNvDQo+IHRoYXQgd2Uga25vdyB1c2Vyc3Bh
Y2Ugd2lsbCBuZXZlciBieSBhYmxlIHRvIHNwZWNpZnkgaXQuDQo+IA0KPiBhdXRvZnM0IGlzIG1v
ZGlmaWVkIHRvIHN0b3AgdXNpbmcgY3VycmVudF9yZWFsX2NyZWQgdGhhdCB3YXMgcHV0IGluDQo+
IHRoZXJlIHRvIGhhbmRsZSB0aGUgcHJldmlvdXMgdmVyc2lvbiBvZiBzdWJtb3VudCBwZXJtaXNz
aW9uIGNoZWNraW5nLg0KPiANCj4gY2lmcyBpcyBtb2RpZmllZCB0byBwYXNzIHRoZSBtb3VudHBv
aW50IGFsbCBvZiB0aGUgd2F5IGRvd24gdG8NCj4gdmZzX3N1Ym1vdW50Lg0KPiANCj4gZGVidWdm
cyBpcyBtb2RpZmllZCB0byBwYXNzIHRoZSBtb3VudHBvaW50IGFsbCBvZiB0aGUgd2F5IGRvd24g
dG8NCj4gdHJhY2VfYXV0b21vdW50IGJ5IGFkZGluZyBhIG5ldyBwYXJhbWV0ZXIuwqDCoFRvIG1h
a2UgdGhpcyBjaGFuZ2UNCj4gZWFzaWVyDQo+IGEgbmV3IHR5cGVkZWYgZGVidWdmc19hdXRvbW91
bnRfdCBpcyBpbnRyb2R1Y2VkIHRvIGNhcHR1cmUgdGhlIHR5cGUNCj4gb2YNCj4gdGhlIGRlYnVn
ZnMgYXV0b21vdW50IGZ1bmN0aW9uLg0KPiANCj4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcN
Cj4gRml4ZXM6IDA2OWQ1YWM5YWUwZCAoImF1dG9mczrCoMKgRml4IGF1dG9tb3VudHMgYnkgdXNp
bmcNCj4gY3VycmVudF9yZWFsX2NyZWQoKS0+dWlkIikNCj4gRml4ZXM6IGFlYWE0YTc5ZmY2YSAo
ImZzOiBDYWxsIGRfYXV0b21vdW50IHdpdGggdGhlIGZpbGVzeXN0ZW1zDQo+IGNyZWRzIikNCj4g
U2lnbmVkLW9mZi1ieTogIkVyaWMgVy4gQmllZGVybWFuIiA8ZWJpZWRlcm1AeG1pc3Npb24uY29t
Pg0KPiAtLS0NCj4gwqBmcy9hZnMvbW50cHQuY8KgwqDCoMKgwqDCoMKgwqDCoMKgfMKgwqAyICst
DQo+IMKgZnMvYXV0b2ZzNC93YWl0cS5jwqDCoMKgwqDCoMKgfMKgwqA0ICsrLS0NCj4gwqBmcy9j
aWZzL2NpZnNfZGZzX3JlZi5jwqDCoHzCoMKgNyArKysrLS0tDQo+IMKgZnMvZGVidWdmcy9pbm9k
ZS5jwqDCoMKgwqDCoMKgfMKgwqA4ICsrKystLS0tDQo+IMKgZnMvbmFtZWkuY8KgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqB8wqDCoDMgLS0tDQo+IMKgZnMvbmFtZXNwYWNlLmPCoMKgwqDCoMKg
wqDCoMKgwqDCoHwgMTcgKysrKysrKysrKysrKysrKy0NCj4gwqBmcy9uZnMvbmFtZXNwYWNlLmPC
oMKgwqDCoMKgwqB8wqDCoDIgKy0NCj4gwqBmcy9uZnMvbmZzNG5hbWVzcGFjZS5jwqDCoHzCoMKg
MiArLQ0KPiDCoGZzL3N1cGVyLmPCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgfCAxMyArKysr
KysrKysrLS0tDQo+IMKgaW5jbHVkZS9saW51eC9kZWJ1Z2ZzLmggfMKgwqAzICsrLQ0KPiDCoGlu
Y2x1ZGUvbGludXgvbW91bnQuaMKgwqDCoHzCoMKgMyArKysNCj4gwqBpbmNsdWRlL3VhcGkvbGlu
dXgvZnMuaCB8wqDCoDEgKw0KPiDCoGtlcm5lbC90cmFjZS90cmFjZS5jwqDCoMKgwqB8wqDCoDQg
KystLQ0KPiDCoDEzIGZpbGVzIGNoYW5nZWQsIDQ3IGluc2VydGlvbnMoKyksIDIyIGRlbGV0aW9u
cygtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2ZzL2Fmcy9tbnRwdC5jIGIvZnMvYWZzL21udHB0LmMN
Cj4gaW5kZXggODFkZDA3NTM1NmI5Li5kNGZiMGFmYzAwOTcgMTAwNjQ0DQo+IC0tLSBhL2ZzL2Fm
cy9tbnRwdC5jDQo+ICsrKyBiL2ZzL2Fmcy9tbnRwdC5jDQo+IEBAIC0yMDIsNyArMjAyLDcgQEAg
c3RhdGljIHN0cnVjdCB2ZnNtb3VudA0KPiAqYWZzX21udHB0X2RvX2F1dG9tb3VudChzdHJ1Y3Qg
ZGVudHJ5ICptbnRwdCkNCj4gwqANCj4gwqAJLyogdHJ5IGFuZCBkbyB0aGUgbW91bnQgKi8NCj4g
wqAJX2RlYnVnKCItLS0gYXR0ZW1wdGluZyBtb3VudCAlcyAtbyAlcyAtLS0iLCBkZXZuYW1lLA0K
PiBvcHRpb25zKTsNCj4gLQltbnQgPSB2ZnNfa2Vybl9tb3VudCgmYWZzX2ZzX3R5cGUsIDAsIGRl
dm5hbWUsIG9wdGlvbnMpOw0KPiArCW1udCA9IHZmc19zdWJtb3VudChtbnRwdCwgJmFmc19mc190
eXBlLCBkZXZuYW1lLCBvcHRpb25zKTsNCj4gwqAJX2RlYnVnKCItLS0gbW91bnQgcmVzdWx0ICVw
IC0tLSIsIG1udCk7DQo+IMKgDQo+IMKgCWZyZWVfcGFnZSgodW5zaWduZWQgbG9uZykgZGV2bmFt
ZSk7DQo+IGRpZmYgLS1naXQgYS9mcy9hdXRvZnM0L3dhaXRxLmMgYi9mcy9hdXRvZnM0L3dhaXRx
LmMNCj4gaW5kZXggMTI3ODMzNWNlMzY2Li43OWZiZDg1ZGI0YmEgMTAwNjQ0DQo+IC0tLSBhL2Zz
L2F1dG9mczQvd2FpdHEuYw0KPiArKysgYi9mcy9hdXRvZnM0L3dhaXRxLmMNCj4gQEAgLTQzNiw4
ICs0MzYsOCBAQCBpbnQgYXV0b2ZzNF93YWl0KHN0cnVjdCBhdXRvZnNfc2JfaW5mbyAqc2JpLA0K
PiDCoAkJbWVtY3B5KCZ3cS0+bmFtZSwgJnFzdHIsIHNpemVvZihzdHJ1Y3QgcXN0cikpOw0KPiDC
oAkJd3EtPmRldiA9IGF1dG9mczRfZ2V0X2RldihzYmkpOw0KPiDCoAkJd3EtPmlubyA9IGF1dG9m
czRfZ2V0X2lubyhzYmkpOw0KPiAtCQl3cS0+dWlkID0gY3VycmVudF9yZWFsX2NyZWQoKS0+dWlk
Ow0KPiAtCQl3cS0+Z2lkID0gY3VycmVudF9yZWFsX2NyZWQoKS0+Z2lkOw0KPiArCQl3cS0+dWlk
ID0gY3VycmVudF9jcmVkKCktPnVpZDsNCj4gKwkJd3EtPmdpZCA9IGN1cnJlbnRfY3JlZCgpLT5n
aWQ7DQo+IMKgCQl3cS0+cGlkID0gcGlkOw0KPiDCoAkJd3EtPnRnaWQgPSB0Z2lkOw0KPiDCoAkJ
d3EtPnN0YXR1cyA9IC1FSU5UUjsgLyogU3RhdHVzIHJldHVybiBpZiBpbnRlcnJ1cHRlZA0KPiAq
Lw0KPiBkaWZmIC0tZ2l0IGEvZnMvY2lmcy9jaWZzX2Rmc19yZWYuYyBiL2ZzL2NpZnMvY2lmc19k
ZnNfcmVmLmMNCj4gaW5kZXggZWM5ZGJiY2NhM2I5Li45MTU2YmU1NDViMGYgMTAwNjQ0DQo+IC0t
LSBhL2ZzL2NpZnMvY2lmc19kZnNfcmVmLmMNCj4gKysrIGIvZnMvY2lmcy9jaWZzX2Rmc19yZWYu
Yw0KPiBAQCAtMjQ1LDcgKzI0NSw4IEBAIGNoYXIgKmNpZnNfY29tcG9zZV9tb3VudF9vcHRpb25z
KGNvbnN0IGNoYXINCj4gKnNiX21vdW50ZGF0YSwNCj4gwqAgKiBAZnVsbHBhdGg6CQlmdWxsIHBh
dGggaW4gVU5DIGZvcm1hdA0KPiDCoCAqIEByZWY6CQlzZXJ2ZXIncyByZWZlcnJhbA0KPiDCoCAq
Lw0KPiAtc3RhdGljIHN0cnVjdCB2ZnNtb3VudCAqY2lmc19kZnNfZG9fcmVmbW91bnQoc3RydWN0
IGNpZnNfc2JfaW5mbw0KPiAqY2lmc19zYiwNCj4gK3N0YXRpYyBzdHJ1Y3QgdmZzbW91bnQgKmNp
ZnNfZGZzX2RvX3JlZm1vdW50KHN0cnVjdCBkZW50cnkgKm1udHB0LA0KPiArCQlzdHJ1Y3QgY2lm
c19zYl9pbmZvICpjaWZzX3NiLA0KPiDCoAkJY29uc3QgY2hhciAqZnVsbHBhdGgsIGNvbnN0IHN0
cnVjdCBkZnNfaW5mbzNfcGFyYW0NCj4gKnJlZikNCj4gwqB7DQo+IMKgCXN0cnVjdCB2ZnNtb3Vu
dCAqbW50Ow0KPiBAQCAtMjU5LDcgKzI2MCw3IEBAIHN0YXRpYyBzdHJ1Y3QgdmZzbW91bnQNCj4g
KmNpZnNfZGZzX2RvX3JlZm1vdW50KHN0cnVjdCBjaWZzX3NiX2luZm8gKmNpZnNfc2IsDQo+IMKg
CWlmIChJU19FUlIobW91bnRkYXRhKSkNCj4gwqAJCXJldHVybiAoc3RydWN0IHZmc21vdW50ICop
bW91bnRkYXRhOw0KPiDCoA0KPiAtCW1udCA9IHZmc19rZXJuX21vdW50KCZjaWZzX2ZzX3R5cGUs
IDAsIGRldm5hbWUsIG1vdW50ZGF0YSk7DQo+ICsJbW50ID0gdmZzX3N1Ym1vdW50KG1udHB0LCAm
Y2lmc19mc190eXBlLCBkZXZuYW1lLA0KPiBtb3VudGRhdGEpOw0KPiDCoAlrZnJlZShtb3VudGRh
dGEpOw0KPiDCoAlrZnJlZShkZXZuYW1lKTsNCj4gwqAJcmV0dXJuIG1udDsNCj4gQEAgLTMzNCw3
ICszMzUsNyBAQCBzdGF0aWMgc3RydWN0IHZmc21vdW50DQo+ICpjaWZzX2Rmc19kb19hdXRvbW91
bnQoc3RydWN0IGRlbnRyeSAqbW50cHQpDQo+IMKgCQkJbW50ID0gRVJSX1BUUigtRUlOVkFMKTsN
Cj4gwqAJCQlicmVhazsNCj4gwqAJCX0NCj4gLQkJbW50ID0gY2lmc19kZnNfZG9fcmVmbW91bnQo
Y2lmc19zYiwNCj4gKwkJbW50ID0gY2lmc19kZnNfZG9fcmVmbW91bnQobW50cHQsIGNpZnNfc2Is
DQo+IMKgCQkJCWZ1bGxfcGF0aCwgcmVmZXJyYWxzICsgaSk7DQo+IMKgCQljaWZzX2RiZyhGWUks
ICIlczogY2lmc19kZnNfZG9fcmVmbW91bnQ6JXMgLA0KPiBtbnQ6JXBcbiIsDQo+IMKgCQkJwqBf
X2Z1bmNfXywgcmVmZXJyYWxzW2ldLm5vZGVfbmFtZSwgbW50KTsNCj4gZGlmZiAtLWdpdCBhL2Zz
L2RlYnVnZnMvaW5vZGUuYyBiL2ZzL2RlYnVnZnMvaW5vZGUuYw0KPiBpbmRleCBmMTdmY2Y4OWUx
OGUuLjFlMzBmNzRhOTUyNyAxMDA2NDQNCj4gLS0tIGEvZnMvZGVidWdmcy9pbm9kZS5jDQo+ICsr
KyBiL2ZzL2RlYnVnZnMvaW5vZGUuYw0KPiBAQCAtMTg3LDkgKzE4Nyw5IEBAIHN0YXRpYyBjb25z
dCBzdHJ1Y3Qgc3VwZXJfb3BlcmF0aW9ucw0KPiBkZWJ1Z2ZzX3N1cGVyX29wZXJhdGlvbnMgPSB7
DQo+IMKgDQo+IMKgc3RhdGljIHN0cnVjdCB2ZnNtb3VudCAqZGVidWdmc19hdXRvbW91bnQoc3Ry
dWN0IHBhdGggKnBhdGgpDQo+IMKgew0KPiAtCXN0cnVjdCB2ZnNtb3VudCAqKCpmKSh2b2lkICop
Ow0KPiAtCWYgPSAoc3RydWN0IHZmc21vdW50ICooKikodm9pZCAqKSlwYXRoLT5kZW50cnktPmRf
ZnNkYXRhOw0KPiAtCXJldHVybiBmKGRfaW5vZGUocGF0aC0+ZGVudHJ5KS0+aV9wcml2YXRlKTsN
Cj4gKwlkZWJ1Z2ZzX2F1dG9tb3VudF90IGY7DQo+ICsJZiA9IChkZWJ1Z2ZzX2F1dG9tb3VudF90
KXBhdGgtPmRlbnRyeS0+ZF9mc2RhdGE7DQo+ICsJcmV0dXJuIGYocGF0aC0+ZGVudHJ5LCBkX2lu
b2RlKHBhdGgtPmRlbnRyeSktPmlfcHJpdmF0ZSk7DQo+IMKgfQ0KPiDCoA0KPiDCoHN0YXRpYyBj
b25zdCBzdHJ1Y3QgZGVudHJ5X29wZXJhdGlvbnMgZGVidWdmc19kb3BzID0gew0KPiBAQCAtNTA0
LDcgKzUwNCw3IEBAIEVYUE9SVF9TWU1CT0xfR1BMKGRlYnVnZnNfY3JlYXRlX2Rpcik7DQo+IMKg
ICovDQo+IMKgc3RydWN0IGRlbnRyeSAqZGVidWdmc19jcmVhdGVfYXV0b21vdW50KGNvbnN0IGNo
YXIgKm5hbWUsDQo+IMKgCQkJCQlzdHJ1Y3QgZGVudHJ5ICpwYXJlbnQsDQo+IC0JCQkJCXN0cnVj
dCB2ZnNtb3VudCAqKCpmKSh2b2lkDQo+ICopLA0KPiArCQkJCQlkZWJ1Z2ZzX2F1dG9tb3VudF90
IGYsDQo+IMKgCQkJCQl2b2lkICpkYXRhKQ0KPiDCoHsNCj4gwqAJc3RydWN0IGRlbnRyeSAqZGVu
dHJ5ID0gc3RhcnRfY3JlYXRpbmcobmFtZSwgcGFyZW50KTsNCj4gZGlmZiAtLWdpdCBhL2ZzL25h
bWVpLmMgYi9mcy9uYW1laS5jDQo+IGluZGV4IDZmYTNlOTEzOGZlNC4uZGE2ODljOWMwMDVlIDEw
MDY0NA0KPiAtLS0gYS9mcy9uYW1laS5jDQo+ICsrKyBiL2ZzL25hbWVpLmMNCj4gQEAgLTExMDAs
NyArMTEwMCw2IEBAIHN0YXRpYyBpbnQgZm9sbG93X2F1dG9tb3VudChzdHJ1Y3QgcGF0aCAqcGF0
aCwNCj4gc3RydWN0IG5hbWVpZGF0YSAqbmQsDQo+IMKgCQkJwqDCoMKgwqBib29sICpuZWVkX21u
dHB1dCkNCj4gwqB7DQo+IMKgCXN0cnVjdCB2ZnNtb3VudCAqbW50Ow0KPiAtCWNvbnN0IHN0cnVj
dCBjcmVkICpvbGRfY3JlZDsNCj4gwqAJaW50IGVycjsNCj4gwqANCj4gwqAJaWYgKCFwYXRoLT5k
ZW50cnktPmRfb3AgfHwgIXBhdGgtPmRlbnRyeS0+ZF9vcC0+ZF9hdXRvbW91bnQpDQo+IEBAIC0x
MTI5LDkgKzExMjgsNyBAQCBzdGF0aWMgaW50IGZvbGxvd19hdXRvbW91bnQoc3RydWN0IHBhdGgg
KnBhdGgsDQo+IHN0cnVjdCBuYW1laWRhdGEgKm5kLA0KPiDCoAlpZiAobmQtPnRvdGFsX2xpbmtf
Y291bnQgPj0gNDApDQo+IMKgCQlyZXR1cm4gLUVMT09QOw0KPiDCoA0KPiAtCW9sZF9jcmVkID0g
b3ZlcnJpZGVfY3JlZHMoJmluaXRfY3JlZCk7DQo+IMKgCW1udCA9IHBhdGgtPmRlbnRyeS0+ZF9v
cC0+ZF9hdXRvbW91bnQocGF0aCk7DQo+IC0JcmV2ZXJ0X2NyZWRzKG9sZF9jcmVkKTsNCj4gwqAJ
aWYgKElTX0VSUihtbnQpKSB7DQo+IMKgCQkvKg0KPiDCoAkJwqAqIFRoZSBmaWxlc3lzdGVtIGlz
IGFsbG93ZWQgdG8gcmV0dXJuIC1FSVNESVIgaGVyZQ0KPiB0byBpbmRpY2F0ZQ0KPiBkaWZmIC0t
Z2l0IGEvZnMvbmFtZXNwYWNlLmMgYi9mcy9uYW1lc3BhY2UuYw0KPiBpbmRleCA0ODdiYTMwYmI1
YzYuLjA4OWE2YjIzMTM1YSAxMDA2NDQNCj4gLS0tIGEvZnMvbmFtZXNwYWNlLmMNCj4gKysrIGIv
ZnMvbmFtZXNwYWNlLmMNCj4gQEAgLTk4OSw2ICs5ODksMjEgQEAgdmZzX2tlcm5fbW91bnQoc3Ry
dWN0IGZpbGVfc3lzdGVtX3R5cGUgKnR5cGUsDQo+IGludCBmbGFncywgY29uc3QgY2hhciAqbmFt
ZSwgdm9pZA0KPiDCoH0NCj4gwqBFWFBPUlRfU1lNQk9MX0dQTCh2ZnNfa2Vybl9tb3VudCk7DQo+
IMKgDQo+ICtzdHJ1Y3QgdmZzbW91bnQgKg0KPiArdmZzX3N1Ym1vdW50KGNvbnN0IHN0cnVjdCBk
ZW50cnkgKm1vdW50cG9pbnQsIHN0cnVjdA0KPiBmaWxlX3N5c3RlbV90eXBlICp0eXBlLA0KPiAr
CcKgwqDCoMKgwqBjb25zdCBjaGFyICpuYW1lLCB2b2lkICpkYXRhKQ0KPiArew0KPiArCS8qIFVu
dGlsIGl0IGlzIHdvcmtlZCBvdXQgaG93IHRvIHBhc3MgdGhlIHVzZXIgbmFtZXNwYWNlDQo+ICsJ
wqAqIHRocm91Z2ggZnJvbSB0aGUgcGFyZW50IG1vdW50IHRvIHRoZSBzdWJtb3VudCBkb24ndA0K
PiBzdXBwb3J0DQo+ICsJwqAqIHVucHJpdmlsZWdlZCBtb3VudHMgd2l0aCBzdWJtb3VudHMuDQo+
ICsJwqAqLw0KPiArCWlmIChtb3VudHBvaW50LT5kX3NiLT5zX3VzZXJfbnMgIT0gJmluaXRfdXNl
cl9ucykNCj4gKwkJcmV0dXJuIEVSUl9QVFIoLUVQRVJNKTsNCj4gKw0KPiArCXJldHVybiB2ZnNf
a2Vybl9tb3VudCh0eXBlLCBNU19TVUJNT1VOVCwgbmFtZSwgZGF0YSk7DQo+ICt9DQo+ICtFWFBP
UlRfU1lNQk9MX0dQTCh2ZnNfc3VibW91bnQpOw0KPiArDQo+IMKgc3RhdGljIHN0cnVjdCBtb3Vu
dCAqY2xvbmVfbW50KHN0cnVjdCBtb3VudCAqb2xkLCBzdHJ1Y3QgZGVudHJ5DQo+ICpyb290LA0K
PiDCoAkJCQkJaW50IGZsYWcpDQo+IMKgew0KPiBAQCAtMjc5NCw3ICsyODA5LDcgQEAgbG9uZyBk
b19tb3VudChjb25zdCBjaGFyICpkZXZfbmFtZSwgY29uc3QgY2hhcg0KPiBfX3VzZXIgKmRpcl9u
YW1lLA0KPiDCoA0KPiDCoAlmbGFncyAmPSB+KE1TX05PU1VJRCB8IE1TX05PRVhFQyB8IE1TX05P
REVWIHwgTVNfQUNUSVZFIHwNCj4gTVNfQk9STiB8DQo+IMKgCQnCoMKgwqBNU19OT0FUSU1FIHwg
TVNfTk9ESVJBVElNRSB8IE1TX1JFTEFUSU1FfA0KPiBNU19LRVJOTU9VTlQgfA0KPiAtCQnCoMKg
wqBNU19TVFJJQ1RBVElNRSB8IE1TX05PUkVNT1RFTE9DSyk7DQo+ICsJCcKgwqDCoE1TX1NUUklD
VEFUSU1FIHwgTVNfTk9SRU1PVEVMT0NLIHwgTVNfU1VCTU9VTlQpOw0KPiDCoA0KPiDCoAlpZiAo
ZmxhZ3MgJiBNU19SRU1PVU5UKQ0KPiDCoAkJcmV0dmFsID0gZG9fcmVtb3VudCgmcGF0aCwgZmxh
Z3MgJiB+TVNfUkVNT1VOVCwNCj4gbW50X2ZsYWdzLA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25h
bWVzcGFjZS5jIGIvZnMvbmZzL25hbWVzcGFjZS5jDQo+IGluZGV4IDU1NTFlOGVmNjdmZC4uZTQ5
ZDgzMWM0ZTg1IDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmFtZXNwYWNlLmMNCj4gKysrIGIvZnMv
bmZzL25hbWVzcGFjZS5jDQo+IEBAIC0yMjYsNyArMjI2LDcgQEAgc3RhdGljIHN0cnVjdCB2ZnNt
b3VudCAqbmZzX2RvX2Nsb25lX21vdW50KHN0cnVjdA0KPiBuZnNfc2VydmVyICpzZXJ2ZXIsDQo+
IMKgCQkJCQnCoMKgwqBjb25zdCBjaGFyICpkZXZuYW1lLA0KPiDCoAkJCQkJwqDCoMKgc3RydWN0
IG5mc19jbG9uZV9tb3VudA0KPiAqbW91bnRkYXRhKQ0KPiDCoHsNCj4gLQlyZXR1cm4gdmZzX2tl
cm5fbW91bnQoJm5mc194ZGV2X2ZzX3R5cGUsIDAsIGRldm5hbWUsDQo+IG1vdW50ZGF0YSk7DQo+
ICsJcmV0dXJuIHZmc19zdWJtb3VudChtb3VudGRhdGEtPmRlbnRyeSwgJm5mc194ZGV2X2ZzX3R5
cGUsDQo+IGRldm5hbWUsIG1vdW50ZGF0YSk7DQo+IMKgfQ0KPiDCoA0KPiDCoC8qKg0KPiBkaWZm
IC0tZ2l0IGEvZnMvbmZzL25mczRuYW1lc3BhY2UuYyBiL2ZzL25mcy9uZnM0bmFtZXNwYWNlLmMN
Cj4gaW5kZXggZDIxMTA0OTEyNjc2Li5kOGIwNDBiZDk4MTQgMTAwNjQ0DQo+IC0tLSBhL2ZzL25m
cy9uZnM0bmFtZXNwYWNlLmMNCj4gKysrIGIvZnMvbmZzL25mczRuYW1lc3BhY2UuYw0KPiBAQCAt
Mjc5LDcgKzI3OSw3IEBAIHN0YXRpYyBzdHJ1Y3QgdmZzbW91bnQgKnRyeV9sb2NhdGlvbihzdHJ1
Y3QNCj4gbmZzX2Nsb25lX21vdW50ICptb3VudGRhdGEsDQo+IMKgCQkJCW1vdW50ZGF0YS0+aG9z
dG5hbWUsDQo+IMKgCQkJCW1vdW50ZGF0YS0+bW50X3BhdGgpOw0KPiDCoA0KPiAtCQltbnQgPSB2
ZnNfa2Vybl9tb3VudCgmbmZzNF9yZWZlcnJhbF9mc190eXBlLCAwLA0KPiBwYWdlLCBtb3VudGRh
dGEpOw0KPiArCQltbnQgPSB2ZnNfc3VibW91bnQobW91bnRkYXRhLT5kZW50cnksDQo+ICZuZnM0
X3JlZmVycmFsX2ZzX3R5cGUsIHBhZ2UsIG1vdW50ZGF0YSk7DQo+IMKgCQlpZiAoIUlTX0VSUiht
bnQpKQ0KPiDCoAkJCWJyZWFrOw0KPiDCoAl9DQo+IGRpZmYgLS1naXQgYS9mcy9zdXBlci5jIGIv
ZnMvc3VwZXIuYw0KPiBpbmRleCAxNzA5ZWQwMjlhMmMuLjQxODU4NDRmN2ExMiAxMDA2NDQNCj4g
LS0tIGEvZnMvc3VwZXIuYw0KPiArKysgYi9mcy9zdXBlci5jDQo+IEBAIC00NjksNyArNDY5LDcg
QEAgc3RydWN0IHN1cGVyX2Jsb2NrICpzZ2V0X3VzZXJucyhzdHJ1Y3QNCj4gZmlsZV9zeXN0ZW1f
dHlwZSAqdHlwZSwNCj4gwqAJc3RydWN0IHN1cGVyX2Jsb2NrICpvbGQ7DQo+IMKgCWludCBlcnI7
DQo+IMKgDQo+IC0JaWYgKCEoZmxhZ3MgJiBNU19LRVJOTU9VTlQpICYmDQo+ICsJaWYgKCEoZmxh
Z3MgJiAoTVNfS0VSTk1PVU5UfE1TX1NVQk1PVU5UKSkgJiYNCj4gwqAJwqDCoMKgwqAhKHR5cGUt
PmZzX2ZsYWdzICYgRlNfVVNFUk5TX01PVU5UKSAmJg0KPiDCoAnCoMKgwqDCoCFjYXBhYmxlKENB
UF9TWVNfQURNSU4pKQ0KPiDCoAkJcmV0dXJuIEVSUl9QVFIoLUVQRVJNKTsNCj4gQEAgLTQ5OSw3
ICs0OTksNyBAQCBzdHJ1Y3Qgc3VwZXJfYmxvY2sgKnNnZXRfdXNlcm5zKHN0cnVjdA0KPiBmaWxl
X3N5c3RlbV90eXBlICp0eXBlLA0KPiDCoAl9DQo+IMKgCWlmICghcykgew0KPiDCoAkJc3Bpbl91
bmxvY2soJnNiX2xvY2spOw0KPiAtCQlzID0gYWxsb2Nfc3VwZXIodHlwZSwgZmxhZ3MsIHVzZXJf
bnMpOw0KPiArCQlzID0gYWxsb2Nfc3VwZXIodHlwZSwgKGZsYWdzICYgfk1TX1NVQk1PVU5UKSwN
Cj4gdXNlcl9ucyk7DQo+IMKgCQlpZiAoIXMpDQo+IMKgCQkJcmV0dXJuIEVSUl9QVFIoLUVOT01F
TSk7DQo+IMKgCQlnb3RvIHJldHJ5Ow0KPiBAQCAtNTQwLDggKzU0MCwxNSBAQCBzdHJ1Y3Qgc3Vw
ZXJfYmxvY2sgKnNnZXQoc3RydWN0IGZpbGVfc3lzdGVtX3R5cGUNCj4gKnR5cGUsDQo+IMKgew0K
PiDCoAlzdHJ1Y3QgdXNlcl9uYW1lc3BhY2UgKnVzZXJfbnMgPSBjdXJyZW50X3VzZXJfbnMoKTsN
Cj4gwqANCj4gKwkvKiBXZSBkb24ndCB5ZXQgcGFzcyB0aGUgdXNlciBuYW1lc3BhY2Ugb2YgdGhl
IHBhcmVudA0KPiArCcKgKiBtb3VudCB0aHJvdWdoIHRvIGhlcmUgc28gYWx3YXlzIHVzZSAmaW5p
dF91c2VyX25zDQo+ICsJwqAqIHVudGlsIHRoYXQgY2hhbmdlcy4NCj4gKwnCoCovDQo+ICsJaWYg
KGZsYWdzICYgTVNfU1VCTU9VTlQpDQo+ICsJCXVzZXJfbnMgPSAmaW5pdF91c2VyX25zOw0KPiAr
DQo+IMKgCS8qIEVuc3VyZSB0aGUgcmVxdWVzdG9yIGhhcyBwZXJtaXNzaW9ucyBvdmVyIHRoZSB0
YXJnZXQNCj4gZmlsZXN5c3RlbSAqLw0KPiAtCWlmICghKGZsYWdzICYgTVNfS0VSTk1PVU5UKSAm
JiAhbnNfY2FwYWJsZSh1c2VyX25zLA0KPiBDQVBfU1lTX0FETUlOKSkNCj4gKwlpZiAoIShmbGFn
cyAmIChNU19LRVJOTU9VTlR8TVNfU1VCTU9VTlQpKSAmJg0KPiAhbnNfY2FwYWJsZSh1c2VyX25z
LCBDQVBfU1lTX0FETUlOKSkNCj4gwqAJCXJldHVybiBFUlJfUFRSKC1FUEVSTSk7DQo+IMKgDQo+
IMKgCXJldHVybiBzZ2V0X3VzZXJucyh0eXBlLCB0ZXN0LCBzZXQsIGZsYWdzLCB1c2VyX25zLCBk
YXRhKTsNCj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvZGVidWdmcy5oIGIvaW5jbHVkZS9s
aW51eC9kZWJ1Z2ZzLmgNCj4gaW5kZXggMDE0Y2M1NjRkMWM0Li4yMzMwMDZiZTMwYWEgMTAwNjQ0
DQo+IC0tLSBhL2luY2x1ZGUvbGludXgvZGVidWdmcy5oDQo+ICsrKyBiL2luY2x1ZGUvbGludXgv
ZGVidWdmcy5oDQo+IEBAIC05Nyw5ICs5NywxMCBAQCBzdHJ1Y3QgZGVudHJ5ICpkZWJ1Z2ZzX2Ny
ZWF0ZV9kaXIoY29uc3QgY2hhcg0KPiAqbmFtZSwgc3RydWN0IGRlbnRyeSAqcGFyZW50KTsNCj4g
wqBzdHJ1Y3QgZGVudHJ5ICpkZWJ1Z2ZzX2NyZWF0ZV9zeW1saW5rKGNvbnN0IGNoYXIgKm5hbWUs
IHN0cnVjdA0KPiBkZW50cnkgKnBhcmVudCwNCj4gwqAJCQkJwqDCoMKgwqDCoMKgY29uc3QgY2hh
ciAqZGVzdCk7DQo+IMKgDQo+ICt0eXBlZGVmIHN0cnVjdCB2ZnNtb3VudCAqKCpkZWJ1Z2ZzX2F1
dG9tb3VudF90KShzdHJ1Y3QgZGVudHJ5ICosDQo+IHZvaWQgKik7DQo+IMKgc3RydWN0IGRlbnRy
eSAqZGVidWdmc19jcmVhdGVfYXV0b21vdW50KGNvbnN0IGNoYXIgKm5hbWUsDQo+IMKgCQkJCQlz
dHJ1Y3QgZGVudHJ5ICpwYXJlbnQsDQo+IC0JCQkJCXN0cnVjdCB2ZnNtb3VudCAqKCpmKSh2b2lk
DQo+ICopLA0KPiArCQkJCQlkZWJ1Z2ZzX2F1dG9tb3VudF90IGYsDQo+IMKgCQkJCQl2b2lkICpk
YXRhKTsNCj4gwqANCj4gwqB2b2lkIGRlYnVnZnNfcmVtb3ZlKHN0cnVjdCBkZW50cnkgKmRlbnRy
eSk7DQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L21vdW50LmggYi9pbmNsdWRlL2xpbnV4
L21vdW50LmgNCj4gaW5kZXggYzZmNTUxNThkNWU1Li44ZTAzNTJhZjA2YjcgMTAwNjQ0DQo+IC0t
LSBhL2luY2x1ZGUvbGludXgvbW91bnQuaA0KPiArKysgYi9pbmNsdWRlL2xpbnV4L21vdW50LmgN
Cj4gQEAgLTkwLDYgKzkwLDkgQEAgc3RydWN0IGZpbGVfc3lzdGVtX3R5cGU7DQo+IMKgZXh0ZXJu
IHN0cnVjdCB2ZnNtb3VudCAqdmZzX2tlcm5fbW91bnQoc3RydWN0IGZpbGVfc3lzdGVtX3R5cGUN
Cj4gKnR5cGUsDQo+IMKgCQkJCcKgwqDCoMKgwqDCoGludCBmbGFncywgY29uc3QgY2hhciAqbmFt
ZSwNCj4gwqAJCQkJwqDCoMKgwqDCoMKgdm9pZCAqZGF0YSk7DQo+ICtleHRlcm4gc3RydWN0IHZm
c21vdW50ICp2ZnNfc3VibW91bnQoY29uc3Qgc3RydWN0IGRlbnRyeQ0KPiAqbW91bnRwb2ludCwN
Cj4gKwkJCQnCoMKgwqDCoMKgc3RydWN0IGZpbGVfc3lzdGVtX3R5cGUgKnR5cGUsDQo+ICsJCQkJ
wqDCoMKgwqDCoGNvbnN0IGNoYXIgKm5hbWUsIHZvaWQgKmRhdGEpOw0KPiDCoA0KPiDCoGV4dGVy
biB2b2lkIG1udF9zZXRfZXhwaXJ5KHN0cnVjdCB2ZnNtb3VudCAqbW50LCBzdHJ1Y3QgbGlzdF9o
ZWFkDQo+ICpleHBpcnlfbGlzdCk7DQo+IMKgZXh0ZXJuIHZvaWQgbWFya19tb3VudHNfZm9yX2V4
cGlyeShzdHJ1Y3QgbGlzdF9oZWFkICptb3VudHMpOw0KPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS91
YXBpL2xpbnV4L2ZzLmggYi9pbmNsdWRlL3VhcGkvbGludXgvZnMuaA0KPiBpbmRleCAzNmRhOTNm
YmYxODguLjA0OGE4NWU5ZjAxNyAxMDA2NDQNCj4gLS0tIGEvaW5jbHVkZS91YXBpL2xpbnV4L2Zz
LmgNCj4gKysrIGIvaW5jbHVkZS91YXBpL2xpbnV4L2ZzLmgNCj4gQEAgLTEzMiw2ICsxMzIsNyBA
QCBzdHJ1Y3QgaW5vZGVzX3N0YXRfdCB7DQo+IMKgI2RlZmluZSBNU19MQVpZVElNRQkoMTw8MjUp
IC8qIFVwZGF0ZSB0aGUgb24tZGlzayBbYWNtXXRpbWVzDQo+IGxhemlseSAqLw0KPiDCoA0KPiDC
oC8qIFRoZXNlIHNiIGZsYWdzIGFyZSBpbnRlcm5hbCB0byB0aGUga2VybmVsICovDQo+ICsjZGVm
aW5lIE1TX1NVQk1PVU5UwqDCoMKgwqDCoCgxPDwyNikNCj4gwqAjZGVmaW5lIE1TX05PUkVNT1RF
TE9DSwkoMTw8MjcpDQo+IMKgI2RlZmluZSBNU19OT1NFQwkoMTw8MjgpDQo+IMKgI2RlZmluZSBN
U19CT1JOCQkoMTw8MjkpDQo+IGRpZmYgLS1naXQgYS9rZXJuZWwvdHJhY2UvdHJhY2UuYyBiL2tl
cm5lbC90cmFjZS90cmFjZS5jDQo+IGluZGV4IGQ3NDQ5NzgzOTg3YS4uMzEwZjBlYTBkMWEyIDEw
MDY0NA0KPiAtLS0gYS9rZXJuZWwvdHJhY2UvdHJhY2UuYw0KPiArKysgYi9rZXJuZWwvdHJhY2Uv
dHJhY2UuYw0KPiBAQCAtNzUwMyw3ICs3NTAzLDcgQEAgaW5pdF90cmFjZXJfdHJhY2VmcyhzdHJ1
Y3QgdHJhY2VfYXJyYXkgKnRyLA0KPiBzdHJ1Y3QgZGVudHJ5ICpkX3RyYWNlcikNCj4gwqAJZnRy
YWNlX2luaXRfdHJhY2Vmcyh0ciwgZF90cmFjZXIpOw0KPiDCoH0NCj4gwqANCj4gLXN0YXRpYyBz
dHJ1Y3QgdmZzbW91bnQgKnRyYWNlX2F1dG9tb3VudCh2b2lkICppbmdvcmUpDQo+ICtzdGF0aWMg
c3RydWN0IHZmc21vdW50ICp0cmFjZV9hdXRvbW91bnQoc3RydWN0IGRlbnRyeSAqbW50cHQsIHZv
aWQNCj4gKmluZ29yZSkNCj4gwqB7DQo+IMKgCXN0cnVjdCB2ZnNtb3VudCAqbW50Ow0KPiDCoAlz
dHJ1Y3QgZmlsZV9zeXN0ZW1fdHlwZSAqdHlwZTsNCj4gQEAgLTc1MTYsNyArNzUxNiw3IEBAIHN0
YXRpYyBzdHJ1Y3QgdmZzbW91bnQgKnRyYWNlX2F1dG9tb3VudCh2b2lkDQo+ICppbmdvcmUpDQo+
IMKgCXR5cGUgPSBnZXRfZnNfdHlwZSgidHJhY2VmcyIpOw0KPiDCoAlpZiAoIXR5cGUpDQo+IMKg
CQlyZXR1cm4gTlVMTDsNCj4gLQltbnQgPSB2ZnNfa2Vybl9tb3VudCh0eXBlLCAwLCAidHJhY2Vm
cyIsIE5VTEwpOw0KPiArCW1udCA9IHZmc19zdWJtb3VudChtbnRwdCwgdHlwZSwgInRyYWNlZnMi
LCBOVUxMKTsNCj4gwqAJcHV0X2ZpbGVzeXN0ZW0odHlwZSk7DQo+IMKgCWlmIChJU19FUlIobW50
KSkNCj4gwqAJCXJldHVybiBOVUxMOw0KDQpZZXMuIFRoYXQgbG9va3MgbGlrZSBpdCBtaWdodCBi
ZSB3b3JrYWJsZS4NCg0KUmV2aWV3ZWQtYnk6IFRyb25kIE15a2xlYnVzdCA8dHJvbmQubXlrbGVi
dXN0QHByaW1hcnlkYXRhLmNvbT4NCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xp
ZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEu
Y29tDQo=

Re: [REVIEW][PATCH] fs: Better permission checking for submounts

[Seth Forshee]

On Wed, Feb 01, 2017 at 07:38:17PM +1300, Eric W. Biederman wrote:
> 
> To support unprivileged users mounting filesystems two permission
> checks have to be performed: a test to see if the user allowed to
> create a mount in the mount namespace, and a test to see if
> the user is allowed to access the specified filesystem.
> 
> The automount case is special in that mounting the original filesystem
> grants permission to mount the sub-filesystems, to any user who
> happens to stumble across the their mountpoint and satisfies the
> ordinary filesystem permission checks.
> 
> Attempting to handle the automount case by using override_creds
> almost works.  It preserves the idea that permission to mount
> the original filesystem is permission to mount the sub-filesystem.
> Unfortunately using override_creds messes up the filesystems
> ordinary permission checks.
> 
> Solve this by being explicit that a mount is a submount by introducing
> vfs_submount, and using it where appropriate.
> 
> vfs_submount uses a new mount internal mount flags MS_SUBMOUNT, to let
> sget and friends know that a mount is a submount so they can take appropriate
> action.
> 
> sget and sget_userns are modified to not perform any permission checks
> on submounts.
> 
> follow_automount is modified to stop using override_creds as that
> has proven problemantic.
> 
> do_mount is modified to always remove the new MS_SUBMOUNT flag so
> that we know userspace will never by able to specify it.
> 
> autofs4 is modified to stop using current_real_cred that was put in
> there to handle the previous version of submount permission checking.
> 
> cifs is modified to pass the mountpoint all of the way down to vfs_submount.
> 
> debugfs is modified to pass the mountpoint all of the way down to
> trace_automount by adding a new parameter.  To make this change easier
> a new typedef debugfs_automount_t is introduced to capture the type of
> the debugfs automount function.
> 
> Cc: stable@vger.kernel.org
> Fixes: 069d5ac9ae0d ("autofs:  Fix automounts by using current_real_cred()->uid")
> Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Looks good to me. I also got testing from the user who reported the bug
to us, and it does fix his nfs submount problem.

Reviewed-by: Seth Forshee <seth.forshee@canonical.com>