Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448123 > unrolled thread

Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM Oleg Drokin <green@linuxhacker.ru> - 2016-07-21 22:40 +0200
    Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of  EPERM "J. Bruce Fields" <bfields@fieldses.org> - 2016-07-22 04:00 +0200
      Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM Oleg Drokin <green@linuxhacker.ru> - 2016-07-22 08:40 +0200
        Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of  EPERM "J. Bruce Fields" <bfields@fieldses.org> - 2016-07-22 13:00 +0200
          Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM Oleg Drokin <green@linuxhacker.ru> - 2016-07-22 17:20 +0200
            Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM "J. Bruce Fields" <bfields@redhat.com> - 2016-07-22 20:00 +0200
              [PATCH 6/7] nfsd: clean up bad-type check in nfsd_create_locked "J. Bruce Fields" <bfields@redhat.com> - 2016-07-22 20:00 +0200
              [PATCH 1/7] nfsd: Make creates return EEXIST instead of EACCES "J. Bruce Fields" <bfields@redhat.com> - 2016-07-22 20:00 +0200
              [PATCH 7/7] nfsd: drop unnecessary MAY_EXEC check from create "J. Bruce Fields" <bfields@redhat.com> - 2016-07-22 20:00 +0200

#1448123 — Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM

On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:

> On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
>> 
>> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
>> 
>>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
>>>> It looks like we are bit overzealous about failing mkdir/create/mknod
>>>> with permission denied if the parent dir is not writeable.
>>>> Need to make sure the name does not exist first, because we need to
>>>> return EEXIST in that case.
>>>> 
>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>> ---
>>>> A very similar problem exists with symlinks, but the patch is more
>>>> involved, so assuming this one is ok, I'll send a symlink one separately.
>>>> fs/nfsd/nfs4proc.c |  6 +++++-
>>>> fs/nfsd/vfs.c      | 11 ++++++++++-
>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index de1ff1d..0067520 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>> 
>>>> 	fh_init(&resfh, NFS4_FHSIZE);
>>>> 
>>>> +	/*
>>>> +	 * We just check thta parent is accessible here, nfsd_* do their
>>>> +	 * own access permission checks
>>>> +	 */
>>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
>>>> -			   NFSD_MAY_CREATE);
>>>> +			   NFSD_MAY_EXEC);
>>>> 	if (status)
>>>> 		return status;
>>>> 
>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>> index 6fbd81e..6a45ec6 100644
>>>> --- a/fs/nfsd/vfs.c
>>>> +++ b/fs/nfsd/vfs.c
>>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>> 	if (isdotent(fname, flen))
>>>> 		goto out;
>>>> 
>>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
>>>> +	/*
>>>> +	 * Even though it is a create, first we see if we are even allowed
>>>> +	 * to peek inside the parent
>>>> +	 */
>>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
>>> 
>>> Looks like in the v3 case we haven't actually locked the directory yet
>>> at this point so this check is a little race-prone.
>> 
>> In reality this check is not really needed, I suspect.
>> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
>> anyway so if there was a race and somebody changed dir access in the middle,
>> there's going to be another check anyway and it would be caught.
>> Unless there's some weird server-side permission wiggling as well that makes it
>> ineffective, but I imagine that one cannot really change in a racy way?
> 
> Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
> We still need the fh_verify there since it's also what does the
> filehandle->dentry translation, but we don't need permission checking
> here yet.

This will likely need an extra test to ensure that when you
do mkdir where you do not have exec permissions, you would get EACCES instead
of EEXIST, otherwise that would be information leakage, no?
Or do you think the second time we do nfsd_permission, that would be covered?

> Applying with that one change.  (And I'll followup with some additional
> minor cleanup of the create code.)
> 
> --b.
> 
>> 
>>> I wonder why the code's structured that way--it's confusing.
>> 
>> Probably years of accumulated "damage" ;)
>> 
>>> --b.
>>> 
>>>> 	if (err)
>>>> 		goto out;
>>>> 
>>>> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>> 		goto out; 
>>>> 	}
>>>> 
>>>> +	/* Now let's see if we actually have permissions to create */
>>>> +	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
>>>> +	if (err)
>>>> +		goto out;
>>>> +
>>>> 	if (!(iap->ia_valid & ATTR_MODE))
>>>> 		iap->ia_mode = 0;
>>>> 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
>>>> -- 
>>>> 2.7.4

[toc] | [next] | [standalone]

#1448300 — Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM

On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote:
> 
> On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:
> 
> > On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
> >> 
> >> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
> >> 
> >>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
> >>>> It looks like we are bit overzealous about failing mkdir/create/mknod
> >>>> with permission denied if the parent dir is not writeable.
> >>>> Need to make sure the name does not exist first, because we need to
> >>>> return EEXIST in that case.
> >>>> 
> >>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> >>>> ---
> >>>> A very similar problem exists with symlinks, but the patch is more
> >>>> involved, so assuming this one is ok, I'll send a symlink one separately.
> >>>> fs/nfsd/nfs4proc.c |  6 +++++-
> >>>> fs/nfsd/vfs.c      | 11 ++++++++++-
> >>>> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>> index de1ff1d..0067520 100644
> >>>> --- a/fs/nfsd/nfs4proc.c
> >>>> +++ b/fs/nfsd/nfs4proc.c
> >>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>> 
> >>>> 	fh_init(&resfh, NFS4_FHSIZE);
> >>>> 
> >>>> +	/*
> >>>> +	 * We just check thta parent is accessible here, nfsd_* do their
> >>>> +	 * own access permission checks
> >>>> +	 */
> >>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
> >>>> -			   NFSD_MAY_CREATE);
> >>>> +			   NFSD_MAY_EXEC);
> >>>> 	if (status)
> >>>> 		return status;
> >>>> 
> >>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>>> index 6fbd81e..6a45ec6 100644
> >>>> --- a/fs/nfsd/vfs.c
> >>>> +++ b/fs/nfsd/vfs.c
> >>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >>>> 	if (isdotent(fname, flen))
> >>>> 		goto out;
> >>>> 
> >>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> >>>> +	/*
> >>>> +	 * Even though it is a create, first we see if we are even allowed
> >>>> +	 * to peek inside the parent
> >>>> +	 */
> >>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
> >>> 
> >>> Looks like in the v3 case we haven't actually locked the directory yet
> >>> at this point so this check is a little race-prone.
> >> 
> >> In reality this check is not really needed, I suspect.
> >> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
> >> anyway so if there was a race and somebody changed dir access in the middle,
> >> there's going to be another check anyway and it would be caught.
> >> Unless there's some weird server-side permission wiggling as well that makes it
> >> ineffective, but I imagine that one cannot really change in a racy way?
> > 
> > Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
> > We still need the fh_verify there since it's also what does the
> > filehandle->dentry translation, but we don't need permission checking
> > here yet.
> 
> This will likely need an extra test to ensure that when you
> do mkdir where you do not have exec permissions, you would get EACCES instead
> of EEXIST, otherwise that would be information leakage, no?
> Or do you think the second time we do nfsd_permission, that would be covered?

No, you're right, for some reason I thought that the check for a
positive inode didn't happen till later.  But actually the logic is
basically:

	lock inode
	lookup_one_len
	return nfserr_exist if looked up dentry is positive.
	check for create permission
	vfs_create

So, yes, the initial MAY_EXEC test's needed to prevent that information
leak.

That said... I wonder why it's done that way?  Seems to me we could just
tremove that nfserr_exist check and the vfs would handle it for us....
I'll try that.

--b.

> 
> > Applying with that one change.  (And I'll followup with some additional
> > minor cleanup of the create code.)
> > 
> > --b.
> > 
> >> 
> >>> I wonder why the code's structured that way--it's confusing.
> >> 
> >> Probably years of accumulated "damage" ;)
> >> 
> >>> --b.
> >>> 
> >>>> 	if (err)
> >>>> 		goto out;
> >>>> 
> >>>> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >>>> 		goto out; 
> >>>> 	}
> >>>> 
> >>>> +	/* Now let's see if we actually have permissions to create */
> >>>> +	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
> >>>> +	if (err)
> >>>> +		goto out;
> >>>> +
> >>>> 	if (!(iap->ia_valid & ATTR_MODE))
> >>>> 		iap->ia_mode = 0;
> >>>> 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
> >>>> -- 
> >>>> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1448413

On Jul 21, 2016, at 9:57 PM, J. Bruce Fields wrote:

> On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote:
>> 
>> On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:
>> 
>>> On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
>>>> 
>>>> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
>>>> 
>>>>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
>>>>>> It looks like we are bit overzealous about failing mkdir/create/mknod
>>>>>> with permission denied if the parent dir is not writeable.
>>>>>> Need to make sure the name does not exist first, because we need to
>>>>>> return EEXIST in that case.
>>>>>> 
>>>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>>>> ---
>>>>>> A very similar problem exists with symlinks, but the patch is more
>>>>>> involved, so assuming this one is ok, I'll send a symlink one separately.
>>>>>> fs/nfsd/nfs4proc.c |  6 +++++-
>>>>>> fs/nfsd/vfs.c      | 11 ++++++++++-
>>>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>> index de1ff1d..0067520 100644
>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>>> 
>>>>>> 	fh_init(&resfh, NFS4_FHSIZE);
>>>>>> 
>>>>>> +	/*
>>>>>> +	 * We just check thta parent is accessible here, nfsd_* do their
>>>>>> +	 * own access permission checks
>>>>>> +	 */
>>>>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
>>>>>> -			   NFSD_MAY_CREATE);
>>>>>> +			   NFSD_MAY_EXEC);
>>>>>> 	if (status)
>>>>>> 		return status;
>>>>>> 
>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>> index 6fbd81e..6a45ec6 100644
>>>>>> --- a/fs/nfsd/vfs.c
>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>>>> 	if (isdotent(fname, flen))
>>>>>> 		goto out;
>>>>>> 
>>>>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
>>>>>> +	/*
>>>>>> +	 * Even though it is a create, first we see if we are even allowed
>>>>>> +	 * to peek inside the parent
>>>>>> +	 */
>>>>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
>>>>> 
>>>>> Looks like in the v3 case we haven't actually locked the directory yet
>>>>> at this point so this check is a little race-prone.
>>>> 
>>>> In reality this check is not really needed, I suspect.
>>>> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
>>>> anyway so if there was a race and somebody changed dir access in the middle,
>>>> there's going to be another check anyway and it would be caught.
>>>> Unless there's some weird server-side permission wiggling as well that makes it
>>>> ineffective, but I imagine that one cannot really change in a racy way?
>>> 
>>> Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
>>> We still need the fh_verify there since it's also what does the
>>> filehandle->dentry translation, but we don't need permission checking
>>> here yet.
>> 
>> This will likely need an extra test to ensure that when you
>> do mkdir where you do not have exec permissions, you would get EACCES instead
>> of EEXIST, otherwise that would be information leakage, no?
>> Or do you think the second time we do nfsd_permission, that would be covered?
> 
> No, you're right, for some reason I thought that the check for a
> positive inode didn't happen till later.  But actually the logic is
> basically:
> 
> 	lock inode
> 	lookup_one_len
> 	return nfserr_exist if looked up dentry is positive.
> 	check for create permission
> 	vfs_create
> 
> So, yes, the initial MAY_EXEC test's needed to prevent that information
> leak.
> 
> That said... I wonder why it's done that way?  Seems to me we could just
> tremove that nfserr_exist check and the vfs would handle it for us....
> I'll try that.

It won't work because the very first thing vfs_create does is may_create(),
and so you get EACCES right there instead of the EEXIST.

> 
> --b.
> 
>> 
>>> Applying with that one change.  (And I'll followup with some additional
>>> minor cleanup of the create code.)
>>> 
>>> --b.
>>> 
>>>> 
>>>>> I wonder why the code's structured that way--it's confusing.
>>>> 
>>>> Probably years of accumulated "damage" ;)
>>>> 
>>>>> --b.
>>>>> 
>>>>>> 	if (err)
>>>>>> 		goto out;
>>>>>> 
>>>>>> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>>>> 		goto out; 
>>>>>> 	}
>>>>>> 
>>>>>> +	/* Now let's see if we actually have permissions to create */
>>>>>> +	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
>>>>>> +	if (err)
>>>>>> +		goto out;
>>>>>> +
>>>>>> 	if (!(iap->ia_valid & ATTR_MODE))
>>>>>> 		iap->ia_mode = 0;
>>>>>> 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
>>>>>> -- 
>>>>>> 2.7.4

[toc] | [prev] | [next] | [standalone]

#1448533 — Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM

On Fri, Jul 22, 2016 at 02:35:26AM -0400, Oleg Drokin wrote:
> 
> On Jul 21, 2016, at 9:57 PM, J. Bruce Fields wrote:
> 
> > On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote:
> >> 
> >> On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:
> >> 
> >>> On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
> >>>> 
> >>>> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
> >>>> 
> >>>>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
> >>>>>> It looks like we are bit overzealous about failing mkdir/create/mknod
> >>>>>> with permission denied if the parent dir is not writeable.
> >>>>>> Need to make sure the name does not exist first, because we need to
> >>>>>> return EEXIST in that case.
> >>>>>> 
> >>>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> >>>>>> ---
> >>>>>> A very similar problem exists with symlinks, but the patch is more
> >>>>>> involved, so assuming this one is ok, I'll send a symlink one separately.
> >>>>>> fs/nfsd/nfs4proc.c |  6 +++++-
> >>>>>> fs/nfsd/vfs.c      | 11 ++++++++++-
> >>>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>>>> index de1ff1d..0067520 100644
> >>>>>> --- a/fs/nfsd/nfs4proc.c
> >>>>>> +++ b/fs/nfsd/nfs4proc.c
> >>>>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>>> 
> >>>>>> 	fh_init(&resfh, NFS4_FHSIZE);
> >>>>>> 
> >>>>>> +	/*
> >>>>>> +	 * We just check thta parent is accessible here, nfsd_* do their
> >>>>>> +	 * own access permission checks
> >>>>>> +	 */
> >>>>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
> >>>>>> -			   NFSD_MAY_CREATE);
> >>>>>> +			   NFSD_MAY_EXEC);
> >>>>>> 	if (status)
> >>>>>> 		return status;
> >>>>>> 
> >>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>>>>> index 6fbd81e..6a45ec6 100644
> >>>>>> --- a/fs/nfsd/vfs.c
> >>>>>> +++ b/fs/nfsd/vfs.c
> >>>>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >>>>>> 	if (isdotent(fname, flen))
> >>>>>> 		goto out;
> >>>>>> 
> >>>>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> >>>>>> +	/*
> >>>>>> +	 * Even though it is a create, first we see if we are even allowed
> >>>>>> +	 * to peek inside the parent
> >>>>>> +	 */
> >>>>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
> >>>>> 
> >>>>> Looks like in the v3 case we haven't actually locked the directory yet
> >>>>> at this point so this check is a little race-prone.
> >>>> 
> >>>> In reality this check is not really needed, I suspect.
> >>>> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
> >>>> anyway so if there was a race and somebody changed dir access in the middle,
> >>>> there's going to be another check anyway and it would be caught.
> >>>> Unless there's some weird server-side permission wiggling as well that makes it
> >>>> ineffective, but I imagine that one cannot really change in a racy way?
> >>> 
> >>> Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
> >>> We still need the fh_verify there since it's also what does the
> >>> filehandle->dentry translation, but we don't need permission checking
> >>> here yet.
> >> 
> >> This will likely need an extra test to ensure that when you
> >> do mkdir where you do not have exec permissions, you would get EACCES instead
> >> of EEXIST, otherwise that would be information leakage, no?
> >> Or do you think the second time we do nfsd_permission, that would be covered?
> > 
> > No, you're right, for some reason I thought that the check for a
> > positive inode didn't happen till later.  But actually the logic is
> > basically:
> > 
> > 	lock inode
> > 	lookup_one_len
> > 	return nfserr_exist if looked up dentry is positive.
> > 	check for create permission
> > 	vfs_create
> > 
> > So, yes, the initial MAY_EXEC test's needed to prevent that information
> > leak.
> > 
> > That said... I wonder why it's done that way?  Seems to me we could just
> > tremove that nfserr_exist check and the vfs would handle it for us....
> > I'll try that.
> 
> It won't work because the very first thing vfs_create does is may_create(),
> and so you get EACCES right there instead of the EEXIST.

static inline int may_create(struct inode *dir, struct dentry *child)
{
        audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
        if (child->d_inode)
                return -EEXIST;
	...

So it looks OK to me.

--b.

[toc] | [prev] | [next] | [standalone]

#1448625

On Jul 22, 2016, at 6:55 AM, J. Bruce Fields wrote:

> On Fri, Jul 22, 2016 at 02:35:26AM -0400, Oleg Drokin wrote:
>> 
>> On Jul 21, 2016, at 9:57 PM, J. Bruce Fields wrote:
>> 
>>> On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote:
>>>> 
>>>> On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:
>>>> 
>>>>> On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
>>>>>> 
>>>>>> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
>>>>>> 
>>>>>>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
>>>>>>>> It looks like we are bit overzealous about failing mkdir/create/mknod
>>>>>>>> with permission denied if the parent dir is not writeable.
>>>>>>>> Need to make sure the name does not exist first, because we need to
>>>>>>>> return EEXIST in that case.
>>>>>>>> 
>>>>>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>>>>>> ---
>>>>>>>> A very similar problem exists with symlinks, but the patch is more
>>>>>>>> involved, so assuming this one is ok, I'll send a symlink one separately.
>>>>>>>> fs/nfsd/nfs4proc.c |  6 +++++-
>>>>>>>> fs/nfsd/vfs.c      | 11 ++++++++++-
>>>>>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>> index de1ff1d..0067520 100644
>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>>>>> 
>>>>>>>> 	fh_init(&resfh, NFS4_FHSIZE);
>>>>>>>> 
>>>>>>>> +	/*
>>>>>>>> +	 * We just check thta parent is accessible here, nfsd_* do their
>>>>>>>> +	 * own access permission checks
>>>>>>>> +	 */
>>>>>>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
>>>>>>>> -			   NFSD_MAY_CREATE);
>>>>>>>> +			   NFSD_MAY_EXEC);
>>>>>>>> 	if (status)
>>>>>>>> 		return status;
>>>>>>>> 
>>>>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>>>>> index 6fbd81e..6a45ec6 100644
>>>>>>>> --- a/fs/nfsd/vfs.c
>>>>>>>> +++ b/fs/nfsd/vfs.c
>>>>>>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>>>>>> 	if (isdotent(fname, flen))
>>>>>>>> 		goto out;
>>>>>>>> 
>>>>>>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
>>>>>>>> +	/*
>>>>>>>> +	 * Even though it is a create, first we see if we are even allowed
>>>>>>>> +	 * to peek inside the parent
>>>>>>>> +	 */
>>>>>>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
>>>>>>> 
>>>>>>> Looks like in the v3 case we haven't actually locked the directory yet
>>>>>>> at this point so this check is a little race-prone.
>>>>>> 
>>>>>> In reality this check is not really needed, I suspect.
>>>>>> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
>>>>>> anyway so if there was a race and somebody changed dir access in the middle,
>>>>>> there's going to be another check anyway and it would be caught.
>>>>>> Unless there's some weird server-side permission wiggling as well that makes it
>>>>>> ineffective, but I imagine that one cannot really change in a racy way?
>>>>> 
>>>>> Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
>>>>> We still need the fh_verify there since it's also what does the
>>>>> filehandle->dentry translation, but we don't need permission checking
>>>>> here yet.
>>>> 
>>>> This will likely need an extra test to ensure that when you
>>>> do mkdir where you do not have exec permissions, you would get EACCES instead
>>>> of EEXIST, otherwise that would be information leakage, no?
>>>> Or do you think the second time we do nfsd_permission, that would be covered?
>>> 
>>> No, you're right, for some reason I thought that the check for a
>>> positive inode didn't happen till later.  But actually the logic is
>>> basically:
>>> 
>>> 	lock inode
>>> 	lookup_one_len
>>> 	return nfserr_exist if looked up dentry is positive.
>>> 	check for create permission
>>> 	vfs_create
>>> 
>>> So, yes, the initial MAY_EXEC test's needed to prevent that information
>>> leak.
>>> 
>>> That said... I wonder why it's done that way?  Seems to me we could just
>>> tremove that nfserr_exist check and the vfs would handle it for us....
>>> I'll try that.
>> 
>> It won't work because the very first thing vfs_create does is may_create(),
>> and so you get EACCES right there instead of the EEXIST.
> 
> static inline int may_create(struct inode *dir, struct dentry *child)
> {
>        audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
>        if (child->d_inode)
>                return -EEXIST;
> 	...
> 
> So it looks OK to me.

Hm, in fact indeed. I was just too worked up about the client side, but on the
server side there was a real lookup already, so it does look workable.

> 
> --b.

[toc] | [prev] | [next] | [standalone]

#1448693

From: "J. Bruce Fields" <bfields@redhat.com>

On Fri, Jul 22, 2016 at 11:13:20AM -0400, Oleg Drokin wrote:
> Hm, in fact indeed. I was just too worked up about the client side,
> but on the server side there was a real lookup already, so it does
> look workable.

So I end up with the following.  This is all (after your patch) pretty
trivial cleanup, but I think it's overdue for that code.

--b.


J. Bruce Fields (6):
  nfsd: remove redundant zero-length check from create
  nfsd: remove redundant i_lookup check
  nfsd: reorganize nfsd_create
  nfsd: remove unnecessary positive-dentry check
  nfsd: clean up bad-type check in nfsd_create_locked
  nfsd: drop unnecessary MAY_EXEC check from create

Oleg Drokin (1):
  nfsd: Make creates return EEXIST instead of EACCES

 fs/nfsd/nfs4proc.c |   3 +-
 fs/nfsd/nfsproc.c  |   7 +--
 fs/nfsd/vfs.c      | 131 ++++++++++++++++++++++++-----------------------------
 fs/nfsd/vfs.h      |   3 ++
 4 files changed, 66 insertions(+), 78 deletions(-)

-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1448694 — [PATCH 6/7] nfsd: clean up bad-type check in nfsd_create_locked

From: "J. Bruce Fields" <bfields@redhat.com>

Minor cleanup, no change in behavior.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/vfs.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index d45b39b408a1..cd06c6511cfc 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1166,13 +1166,6 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		iap->ia_mode = 0;
 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
 
-	err = nfserr_inval;
-	if (!S_ISREG(type) && !S_ISDIR(type) && !special_file(type)) {
-		printk(KERN_WARNING "nfsd: bad file type %o in nfsd_create\n",
-		       type);
-		goto out;
-	}
-
 	err = 0;
 	host_err = 0;
 	switch (type) {
@@ -1190,6 +1183,10 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	case S_IFSOCK:
 		host_err = vfs_mknod(dirp, dchild, iap->ia_mode, rdev);
 		break;
+	default:
+		printk(KERN_WARNING "nfsd: bad file type %o in nfsd_create\n",
+		       type);
+		host_err = -EINVAL;
 	}
 	if (host_err < 0)
 		goto out_nfserr;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1448698 — [PATCH 1/7] nfsd: Make creates return EEXIST instead of EACCES

From: Oleg Drokin <green@linuxhacker.ru>

When doing a create (mkdir/mknod) on a name, it's worth
checking the name exists first before returning EACCES in case
the directory is not writeable by the user.
This makes return values on the client more consistent
regardless of whenever the entry there is cached in the local
cache or not.
Another positive side effect is certain programs only expect
EEXIST in that case even despite POSIX allowing any valid
error to be returned.

Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c |  6 +++++-
 fs/nfsd/vfs.c      | 11 ++++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e0c15f879d89..9d7e1edf0cca 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	fh_init(&resfh, NFS4_FHSIZE);
 
+	/*
+	 * We just check that parent is accessible here, nfsd_* do their
+	 * own access permission checks
+	 */
 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
-			   NFSD_MAY_CREATE);
+			   NFSD_MAY_EXEC);
 	if (status)
 		return status;
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 6fbd81ecb410..fda4f86161f8 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (isdotent(fname, flen))
 		goto out;
 
-	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
+	/*
+	 * Even though it is a create, first let's see if we are even allowed
+	 * to peek inside the parent
+	 */
+	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
 	if (err)
 		goto out;
 
@@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out; 
 	}
 
+	/* Now let's see if we actually have permissions to create */
+	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
+	if (err)
+		goto out;
+
 	if (!(iap->ia_valid & ATTR_MODE))
 		iap->ia_mode = 0;
 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1448699 — [PATCH 7/7] nfsd: drop unnecessary MAY_EXEC check from create

From: "J. Bruce Fields" <bfields@redhat.com>

We need an fh_verify to make sure we at least have a dentry, but actual
permission checks happen later.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c | 7 +------
 fs/nfsd/vfs.c      | 6 +-----
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9d7e1edf0cca..1fb222752b2b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -605,12 +605,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	fh_init(&resfh, NFS4_FHSIZE);
 
-	/*
-	 * We just check that parent is accessible here, nfsd_* do their
-	 * own access permission checks
-	 */
-	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
-			   NFSD_MAY_EXEC);
+	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR, NFSD_MAY_NOP);
 	if (status)
 		return status;
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index cd06c6511cfc..c844fd601381 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1236,11 +1236,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (isdotent(fname, flen))
 		return nfserr_exist;
 
-	/*
-	 * Even though it is a create, first let's see if we are even allowed
-	 * to peek inside the parent
-	 */
-	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
+	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_NOP);
 	if (err)
 		return err;
 
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web