diff mbox

nfsd: EACCES vs EPERM on utime()/utimes() calls

Message ID 557047E2.10804@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kinglong Mee June 4, 2015, 12:43 p.m. UTC 
On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>> Hello.
>>>
>>> As the man page for utime/utimes states [1], EPERM is returned if
>>> the second argument of utime/utimes is not NULL and:
>>>   * the caller's effective user id does not match the owner of the file
>>>   * the caller does not have write access to the file
>>>   * the caller is not privileged
>>>
>>> However, I don't see this behavior with NFS, I see EACCES is
>>> generated instead.
>>
>> Agreed that it's probably a server bug.  (Have you run across a case
>> where this makes a difference?)
> 
> Thank you.
> 
> No, I've not seen such a real-word scenario.
> 
> I have these LTP test cases failing:
> 
> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
> 
> and it makes me a bit nervous :)
> 
>>
>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>> which is probably doing the right thing.  But before that there's an
>> fh_verify()--looks like that is expected to fail in your case.  I bet
>> that's the cause.

Yes, it is.

nfsd do the permission checking before notify_change() as,

/* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));

return -EACCES for non-owner user.

> 
> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
> 
> Anyway, if nobody is interested, I'll give it a try, but later.

Here is a diff patch for this problem, please try testing.
If okay, I will send an official patch.

Note: must apply the following patch first in the url,
http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a

thanks,
Kinglong Mee
-------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

J. Bruce Fields June 4, 2015, 8:27 p.m. UTC | #1 
On Thu, Jun 04, 2015 at 08:43:14PM +0800, Kinglong Mee wrote:
> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
> > On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
> >> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
> >>> Hello.
> >>>
> >>> As the man page for utime/utimes states [1], EPERM is returned if
> >>> the second argument of utime/utimes is not NULL and:
> >>>   * the caller's effective user id does not match the owner of the file
> >>>   * the caller does not have write access to the file
> >>>   * the caller is not privileged
> >>>
> >>> However, I don't see this behavior with NFS, I see EACCES is
> >>> generated instead.
> >>
> >> Agreed that it's probably a server bug.  (Have you run across a case
> >> where this makes a difference?)
> > 
> > Thank you.
> > 
> > No, I've not seen such a real-word scenario.
> > 
> > I have these LTP test cases failing:
> > 
> > * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
> > * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
> > 
> > and it makes me a bit nervous :)
> > 
> >>
> >> Looking at nfsd_setattr()....  The main work is done by notify_change(),
> >> which is probably doing the right thing.  But before that there's an
> >> fh_verify()--looks like that is expected to fail in your case.  I bet
> >> that's the cause.
> 
> Yes, it is.
> 
> nfsd do the permission checking before notify_change() as,
> 
> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
> 
> return -EACCES for non-owner user.
> 
> > 
> > I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
> > 
> > Anyway, if nobody is interested, I'll give it a try, but later.
> 
> Here is a diff patch for this problem, please try testing.
> If okay, I will send an official patch.
> 
> Note: must apply the following patch first in the url,
> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a

We could do that if we have to.

I wonder if we could instead skip the fh_verify's inode_permission call
entirely?  Most callers of fh_verify don't need the inode_permission
call at all as far as I can tell, because the following vfs operation
does permission checking already.

We still need some nfs-specific checking, I guess (e.g. to make sure we
aren't allowing setattr on a read-only export of a writeable mount), but
the inode_permission itself I think we can usually skip.

Andreas Gruenbacher has also been complaining that the redundant
inode_permission calls make the richacl work more difficult, I can't
remember why exactly.

--b.

> 
> thanks,
> Kinglong Mee
> -------------------------------------------------------------
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 84d770b..2533088 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -407,10 +371,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>  	bool		get_write_count;
>  	int		size_change = 0;
>  
> -	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> +	if (iap->ia_valid & ATTR_SIZE) {
>  		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> -	if (iap->ia_valid & ATTR_SIZE)
>  		ftype = S_IFREG;
> +	}
> +
> +	/*
> +	 * According to utimes_common(),
> +	 *
> +	 * If times is NULL (or both times are UTIME_NOW),
> +	 * then we need to check permissions, because
> +	 * inode_change_ok() won't do it.
> +	 */
> +	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
> +		accmode |= NFSD_MAY_OWNER_OVERRIDE;
> +		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
> +			accmode |= NFSD_MAY_WRITE;
> +	}
>  
>  	/* Callers that do fh_verify should do the fh_want_write: */
>  	get_write_count = !fhp->fh_dentry;
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro June 5, 2015, 12:50 a.m. UTC | #2 
On Thu, Jun 04, 2015 at 04:27:25PM -0400, J. Bruce Fields wrote:

> I wonder if we could instead skip the fh_verify's inode_permission call
> entirely?  Most callers of fh_verify don't need the inode_permission
> call at all as far as I can tell, because the following vfs operation
> does permission checking already.

In case of notify_change() we only pass a dentry, so anything mount-dependent
must be done in callers...  Actually, I wonder how does tomoyo and its ilk
interact with nfsd?
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanislav Kholmanskikh June 5, 2015, 3:30 p.m. UTC | #3 
Hi.

On 06/04/2015 03:43 PM, Kinglong Mee wrote:
> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote:
>> On 06/02/2015 12:23 AM, bfields@fieldses.org wrote:
>>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote:
>>>> Hello.
>>>>
>>>> As the man page for utime/utimes states [1], EPERM is returned if
>>>> the second argument of utime/utimes is not NULL and:
>>>>    * the caller's effective user id does not match the owner of the file
>>>>    * the caller does not have write access to the file
>>>>    * the caller is not privileged
>>>>
>>>> However, I don't see this behavior with NFS, I see EACCES is
>>>> generated instead.
>>>
>>> Agreed that it's probably a server bug.  (Have you run across a case
>>> where this makes a difference?)
>>
>> Thank you.
>>
>> No, I've not seen such a real-word scenario.
>>
>> I have these LTP test cases failing:
>>
>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c
>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c
>>
>> and it makes me a bit nervous :)
>>
>>>
>>> Looking at nfsd_setattr()....  The main work is done by notify_change(),
>>> which is probably doing the right thing.  But before that there's an
>>> fh_verify()--looks like that is expected to fail in your case.  I bet
>>> that's the cause.
>
> Yes, it is.
>
> nfsd do the permission checking before notify_change() as,
>
> /* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>
> return -EACCES for non-owner user.
>
>>
>> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :)
>>
>> Anyway, if nobody is interested, I'll give it a try, but later.
>
> Here is a diff patch for this problem, please try testing.
> If okay, I will send an official patch.
>
> Note: must apply the following patch first in the url,
> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a
>
> thanks,
> Kinglong Mee
> -------------------------------------------------------------
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 84d770b..2533088 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -407,10 +371,23 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
>   	bool		get_write_count;
>   	int		size_change = 0;
>
> -	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> +	if (iap->ia_valid & ATTR_SIZE) {
>   		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> -	if (iap->ia_valid & ATTR_SIZE)
>   		ftype = S_IFREG;
> +	}
> +
> +	/*
> +	 * According to utimes_common(),
> +	 *
> +	 * If times is NULL (or both times are UTIME_NOW),
> +	 * then we need to check permissions, because
> +	 * inode_change_ok() won't do it.
> +	 */
> +	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
> +		accmode |= NFSD_MAY_OWNER_OVERRIDE;
> +		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
> +			accmode |= NFSD_MAY_WRITE;
> +	}
>
>   	/* Callers that do fh_verify should do the fh_want_write: */
>   	get_write_count = !fhp->fh_dentry;
>

Tested with v4.1-rc6 plust the above two patches.

The issue is fixed.

My utime_test  now reports EPERM. LTP's utime06, utimes01 now pass, 
other LTP syscall test cases don't show any regression either.

Thank you!
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 84d770b..2533088 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -407,10 +371,23 @@  nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	bool		get_write_count;
 	int		size_change = 0;
 
-	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
+	if (iap->ia_valid & ATTR_SIZE) {
 		accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
-	if (iap->ia_valid & ATTR_SIZE)
 		ftype = S_IFREG;
+	}
+
+	/*
+	 * According to utimes_common(),
+	 *
+	 * If times is NULL (or both times are UTIME_NOW),
+	 * then we need to check permissions, because
+	 * inode_change_ok() won't do it.
+	 */
+	if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME)) {
+		accmode |= NFSD_MAY_OWNER_OVERRIDE;
+		if (!(iap->ia_valid & (ATTR_ATIME_SET | ATTR_MTIME_SET)))
+			accmode |= NFSD_MAY_WRITE;
+	}
 
 	/* Callers that do fh_verify should do the fh_want_write: */
 	get_write_count = !fhp->fh_dentry;