Hi,
I've recently encountered a problem which could be a bug in the nfs implementation.
It could be illustrated with the following small program,
#include <fcntl.h>
#include <unistd.h>
main()
{
int fd;
fd = open("abc", O_WRONLY | O_CREAT, 0644);
if (fd < 0) {
perror("open");
exit(-1);
}
write(fd, "test\n", 5);
setuid(65534);
if (ftruncate(fd, 3) < 0)
perror("ftruncate");
close(fd);
}
Compile and run it as root on an NFS mount without root squash, ftruncate() would
return an EACCESS error. On a local disk, it would complete successfully, leaving
behind a file "abc" with the string "tes". It would also be successful on NFS
if you change the mode from 0644 to 0666 (make sure to set your umask to 0).
I'm not familiar with linux nfs code, but it seems to me that the nfs code does
an additional access mode check in ftruncate/setattr, which is not done on a local
fs. I've checked on freebsd, the program works fine on both local and nfs.
Could someone more familiar with the nfs code take a look? I'm running 2.6.9-42.Elsmp
64-bit, nfsv3 mount. For nfs server, I've tried linux/freebsd and a commercial one
with a proprietary OS.
Thanks
-luoqi
PS: I'm not a subscriber of the linux kernel mailing list, I'd appreciate if any
response could be send to me directly.
On Tue, 2008-06-10 at 15:05 -0700, Luoqi Chen wrote:
> Hi,
>
> I've recently encountered a problem which could be a bug in the nfs implementation.
> It could be illustrated with the following small program,
>
> #include <fcntl.h>
> #include <unistd.h>
>
> main()
> {
> int fd;
>
> fd = open("abc", O_WRONLY | O_CREAT, 0644);
> if (fd < 0) {
> perror("open");
> exit(-1);
> }
>
> write(fd, "test\n", 5);
>
> setuid(65534);
>
> if (ftruncate(fd, 3) < 0)
> perror("ftruncate");
>
> close(fd);
> }
>
> Compile and run it as root on an NFS mount without root squash, ftruncate() would
> return an EACCESS error. On a local disk, it would complete successfully, leaving
> behind a file "abc" with the string "tes". It would also be successful on NFS
> if you change the mode from 0644 to 0666 (make sure to set your umask to 0).
Known problem. I suspect that the following untested patch (against
2.6.26-rc5) should fix it.
Cheers
Trond
-------------------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
Date: Tue, 10 Jun 2008 19:39:41 -0400
NFS: Fix the ftruncate() credential problem
ftruncate() access checking is supposed to be performed at open() time,
just like reads and writes.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/inode.c | 10 +++++++---
fs/nfs/nfs3proc.c | 7 ++++---
fs/nfs/nfs4proc.c | 47 +++++++++++++++++++++++++++--------------------
fs/nfs/proc.c | 5 +++--
include/linux/nfs_xdr.h | 4 ++--
5 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 596c5d8..072ee6c 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -347,13 +347,14 @@ out_no_inode:
goto out;
}
-#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET)
+#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET|ATTR_FILE)
int
nfs_setattr(struct dentry *dentry, struct iattr *attr)
{
struct inode *inode = dentry->d_inode;
struct nfs_fattr fattr;
+ struct rpc_cred *cred = NULL;
int error;
nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
@@ -369,9 +370,12 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
/* Optimization: if the end result is no change, don't RPC */
attr->ia_valid &= NFS_VALID_ATTRS;
- if (attr->ia_valid == 0)
+ if ((attr->ia_valid & ~ATTR_FILE) == 0)
return 0;
+ if (attr->ia_valid & ATTR_FILE)
+ cred = nfs_file_cred(attr->ia_file);
+
lock_kernel();
/* Write all dirty data */
if (S_ISREG(inode->i_mode)) {
@@ -383,7 +387,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
*/
if ((attr->ia_valid & (ATTR_MODE|ATTR_UID|ATTR_GID)) != 0)
nfs_inode_return_delegation(inode);
- error = NFS_PROTO(inode)->setattr(dentry, &fattr, attr);
+ error = NFS_PROTO(inode)->setattr(dentry, cred, &fattr, attr);
if (error == 0)
nfs_refresh_inode(inode, &fattr);
unlock_kernel();
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index c3523ad..48359c6 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -113,8 +113,8 @@ nfs3_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
}
static int
-nfs3_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
- struct iattr *sattr)
+nfs3_proc_setattr(struct dentry *dentry, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr)
{
struct inode *inode = dentry->d_inode;
struct nfs3_sattrargs arg = {
@@ -125,6 +125,7 @@ nfs3_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
.rpc_proc = &nfs3_procedures[NFS3PROC_SETATTR],
.rpc_argp = &arg,
.rpc_resp = fattr,
+ .rpc_cred = cred,
};
int status;
@@ -330,7 +331,7 @@ again:
/* Note: we could use a guarded setattr here, but I'm
* not sure this buys us anything (and I'd have
* to revamp the NFSv3 XDR code) */
- status = nfs3_proc_setattr(dentry, &fattr, sattr);
+ status = nfs3_proc_setattr(dentry, NULL, &fattr, sattr);
nfs_post_op_update_inode(dentry->d_inode, &fattr);
dprintk("NFS reply setattr (post-create): %d\n", status);
}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1293e0a..4583649 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1139,8 +1139,9 @@ static struct nfs4_state *nfs4_do_open(struct inode *dir, struct path *path, int
return res;
}
-static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
- struct iattr *sattr, struct nfs4_state *state)
+static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr,
+ struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(inode);
struct nfs_setattrargs arg = {
@@ -1157,6 +1158,7 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
.rpc_argp = &arg,
.rpc_resp = &res,
+ .rpc_cred = cred,
};
unsigned long timestamp = jiffies;
int status;
@@ -1166,7 +1168,6 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
if (nfs4_copy_delegation_stateid(&arg.stateid, inode)) {
/* Use that stateid */
} else if (state != NULL) {
- msg.rpc_cred = state->owner->so_cred;
nfs4_copy_stateid(&arg.stateid, state, current->files);
} else
memcpy(&arg.stateid, &zero_stateid, sizeof(arg.stateid));
@@ -1177,15 +1178,16 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
return status;
}
-static int nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
- struct iattr *sattr, struct nfs4_state *state)
+static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr,
+ struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(inode);
struct nfs4_exception exception = { };
int err;
do {
err = nfs4_handle_exception(server,
- _nfs4_do_setattr(inode, fattr, sattr, state),
+ _nfs4_do_setattr(inode, cred, fattr, sattr, state),
&exception);
} while (exception.retry);
return err;
@@ -1644,27 +1646,31 @@ static int nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
* This will be fixed with VFS changes (lookup-intent).
*/
static int
-nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
- struct iattr *sattr)
+nfs4_proc_setattr(struct dentry *dentry, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr)
{
- struct rpc_cred *cred;
struct inode *inode = dentry->d_inode;
- struct nfs_open_context *ctx;
+ struct nfs_open_context *ctx = NULL;
struct nfs4_state *state = NULL;
int status;
nfs_fattr_init(fattr);
- cred = rpc_lookup_cred();
- if (IS_ERR(cred))
- return PTR_ERR(cred);
+ if (cred == NULL) {
+ cred = rpc_lookup_cred();
+ if (IS_ERR(cred))
+ return PTR_ERR(cred);
+ } else
+ get_rpccred(cred);
/* Search for an existing open(O_WRITE) file */
- ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
- if (ctx != NULL)
- state = ctx->state;
+ if (sattr->ia_valid && ATTR_FILE) {
+ ctx = nfs_file_open_context(sattr->ia_file);
+ if (ctx != NULL)
+ state = ctx->state;
+ }
- status = nfs4_do_setattr(inode, fattr, sattr, state);
+ status = nfs4_do_setattr(inode, cred, fattr, sattr, state);
if (status == 0)
nfs_setattr_update_inode(inode, sattr);
if (ctx != NULL)
@@ -1897,17 +1903,16 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
goto out;
}
state = nfs4_do_open(dir, &path, flags, sattr, cred);
- put_rpccred(cred);
d_drop(dentry);
if (IS_ERR(state)) {
status = PTR_ERR(state);
- goto out;
+ goto out_putcred;
}
d_add(dentry, igrab(state->inode));
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
if (flags & O_EXCL) {
struct nfs_fattr fattr;
- status = nfs4_do_setattr(state->inode, &fattr, sattr, state);
+ status = nfs4_do_setattr(state->inode, cred, &fattr, sattr, state);
if (status == 0)
nfs_setattr_update_inode(state->inode, sattr);
nfs_post_op_update_inode(state->inode, &fattr);
@@ -1916,6 +1921,8 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
status = nfs4_intent_set_file(nd, &path, state);
else
nfs4_close_sync(&path, state, flags);
+out_putcred:
+ put_rpccred(cred);
out:
return status;
}
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 5c35b02..e203f7e 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -110,8 +110,8 @@ nfs_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
}
static int
-nfs_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
- struct iattr *sattr)
+nfs_proc_setattr(struct dentry *dentry, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr)
{
struct inode *inode = dentry->d_inode;
struct nfs_sattrargs arg = {
@@ -122,6 +122,7 @@ nfs_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
.rpc_proc = &nfs_procedures[NFSPROC_SETATTR],
.rpc_argp = &arg,
.rpc_resp = fattr,
+ .rpc_cred = cred,
};
int status;
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 8d780de..af6661e 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -792,8 +792,8 @@ struct nfs_rpc_ops {
struct nfs_fattr *);
int (*getattr) (struct nfs_server *, struct nfs_fh *,
struct nfs_fattr *);
- int (*setattr) (struct dentry *, struct nfs_fattr *,
- struct iattr *);
+ int (*setattr) (struct dentry *, struct rpc_cred *,
+ struct nfs_fattr *, struct iattr *);
int (*lookup) (struct inode *, struct qstr *,
struct nfs_fh *, struct nfs_fattr *);
int (*access) (struct inode *, struct nfs_access_entry *);
> From: Trond Myklebust <[email protected]>
> Date: Tue, 10 Jun 2008 19:39:41 -0400
> NFS: Fix the ftruncate() credential problem
>
> ftruncate() access checking is supposed to be performed at
> open() time,
> just like reads and writes.
>
Thanks, Trond. Is there any chance this patch could be
included in the linux kernel in the near future? For now,
I guess I'll workaround this problem by moving the ftruncate()
to before setuid().
-luoqi
PS: I haven't tried the patch, just browsing through, and I noticed
a typo, pointing out here to save some trouble for anyone who
wants to give it a try (I guess gcc would issue a warning too),
/* Search for an existing open(O_WRITE) file */
- ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
- if (ctx != NULL)
- state = ctx->state;
+ if (sattr->ia_valid && ATTR_FILE) { <=== && should be &
+ ctx = nfs_file_open_context(sattr->ia_file);
+ if (ctx != NULL)
+ state = ctx->state;
+ }
On Tue, 2008-06-10 at 22:24 -0700, Luoqi Chen wrote:
> > From: Trond Myklebust <[email protected]>
> > Date: Tue, 10 Jun 2008 19:39:41 -0400
> > NFS: Fix the ftruncate() credential problem
> >
> > ftruncate() access checking is supposed to be performed at
> > open() time,
> > just like reads and writes.
> >
> Thanks, Trond. Is there any chance this patch could be
> included in the linux kernel in the near future? For now,
> I guess I'll workaround this problem by moving the ftruncate()
> to before setuid().
>
> -luoqi
>
> PS: I haven't tried the patch, just browsing through, and I noticed
> a typo, pointing out here to save some trouble for anyone who
> wants to give it a try (I guess gcc would issue a warning too),
>
> /* Search for an existing open(O_WRITE) file */
> - ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
> - if (ctx != NULL)
> - state = ctx->state;
> + if (sattr->ia_valid && ATTR_FILE) { <=== && should be &
> + ctx = nfs_file_open_context(sattr->ia_file);
> + if (ctx != NULL)
> + state = ctx->state;
> + }
Thanks for testing! I've refined the patch a bit so that it is less
intrusive w.r.t. common NFS code changes, and fixed a couple more bugs.
As long as the stability proves to be good, I see no reason why we can't
merge this once the 2.6.27 merge window opens.
Cheers
Trond
-----------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
Date: Tue, 10 Jun 2008 19:39:41 -0400
NFS: Fix the ftruncate() credential problem
ftruncate() access checking is supposed to be performed at open() time,
just like reads and writes.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/inode.c | 4 ++--
fs/nfs/nfs3proc.c | 2 ++
fs/nfs/nfs4proc.c | 47 +++++++++++++++++++++++------------------------
fs/nfs/proc.c | 2 ++
4 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 596c5d8..2e4ab4a 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -347,7 +347,7 @@ out_no_inode:
goto out;
}
-#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET)
+#define NFS_VALID_ATTRS (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_SIZE|ATTR_ATIME|ATTR_ATIME_SET|ATTR_MTIME|ATTR_MTIME_SET|ATTR_FILE)
int
nfs_setattr(struct dentry *dentry, struct iattr *attr)
@@ -369,7 +369,7 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
/* Optimization: if the end result is no change, don't RPC */
attr->ia_valid &= NFS_VALID_ATTRS;
- if (attr->ia_valid == 0)
+ if ((attr->ia_valid & ~ATTR_FILE) == 0)
return 0;
lock_kernel();
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index c3523ad..b14b378 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -129,6 +129,8 @@ nfs3_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
int status;
dprintk("NFS call setattr\n");
+ if (sattr->ia_valid & ATTR_FILE)
+ msg.rpc_cred = nfs_file_cred(sattr->ia_file);
nfs_fattr_init(fattr);
status = rpc_call_sync(NFS_CLIENT(inode), &msg, 0);
if (status == 0)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1293e0a..15084a0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1139,8 +1139,9 @@ static struct nfs4_state *nfs4_do_open(struct inode *dir, struct path *path, int
return res;
}
-static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
- struct iattr *sattr, struct nfs4_state *state)
+static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr,
+ struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(inode);
struct nfs_setattrargs arg = {
@@ -1154,9 +1155,10 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
.server = server,
};
struct rpc_message msg = {
- .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
- .rpc_argp = &arg,
- .rpc_resp = &res,
+ .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
+ .rpc_argp = &arg,
+ .rpc_resp = &res,
+ .rpc_cred = cred,
};
unsigned long timestamp = jiffies;
int status;
@@ -1166,7 +1168,6 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
if (nfs4_copy_delegation_stateid(&arg.stateid, inode)) {
/* Use that stateid */
} else if (state != NULL) {
- msg.rpc_cred = state->owner->so_cred;
nfs4_copy_stateid(&arg.stateid, state, current->files);
} else
memcpy(&arg.stateid, &zero_stateid, sizeof(arg.stateid));
@@ -1177,15 +1178,16 @@ static int _nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
return status;
}
-static int nfs4_do_setattr(struct inode *inode, struct nfs_fattr *fattr,
- struct iattr *sattr, struct nfs4_state *state)
+static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
+ struct nfs_fattr *fattr, struct iattr *sattr,
+ struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(inode);
struct nfs4_exception exception = { };
int err;
do {
err = nfs4_handle_exception(server,
- _nfs4_do_setattr(inode, fattr, sattr, state),
+ _nfs4_do_setattr(inode, cred, fattr, sattr, state),
&exception);
} while (exception.retry);
return err;
@@ -1647,29 +1649,25 @@ static int
nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
struct iattr *sattr)
{
- struct rpc_cred *cred;
struct inode *inode = dentry->d_inode;
- struct nfs_open_context *ctx;
+ struct rpc_cred *cred = NULL;
struct nfs4_state *state = NULL;
int status;
nfs_fattr_init(fattr);
- cred = rpc_lookup_cred();
- if (IS_ERR(cred))
- return PTR_ERR(cred);
-
/* Search for an existing open(O_WRITE) file */
- ctx = nfs_find_open_context(inode, cred, FMODE_WRITE);
- if (ctx != NULL)
+ if (sattr->ia_valid & ATTR_FILE) {
+ struct nfs_open_context *ctx;
+
+ ctx = nfs_file_open_context(sattr->ia_file);
+ cred = ctx->cred;
state = ctx->state;
+ }
- status = nfs4_do_setattr(inode, fattr, sattr, state);
+ status = nfs4_do_setattr(inode, cred, fattr, sattr, state);
if (status == 0)
nfs_setattr_update_inode(inode, sattr);
- if (ctx != NULL)
- put_nfs_open_context(ctx);
- put_rpccred(cred);
return status;
}
@@ -1897,17 +1895,16 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
goto out;
}
state = nfs4_do_open(dir, &path, flags, sattr, cred);
- put_rpccred(cred);
d_drop(dentry);
if (IS_ERR(state)) {
status = PTR_ERR(state);
- goto out;
+ goto out_putcred;
}
d_add(dentry, igrab(state->inode));
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
if (flags & O_EXCL) {
struct nfs_fattr fattr;
- status = nfs4_do_setattr(state->inode, &fattr, sattr, state);
+ status = nfs4_do_setattr(state->inode, cred, &fattr, sattr, state);
if (status == 0)
nfs_setattr_update_inode(state->inode, sattr);
nfs_post_op_update_inode(state->inode, &fattr);
@@ -1916,6 +1913,8 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
status = nfs4_intent_set_file(nd, &path, state);
else
nfs4_close_sync(&path, state, flags);
+out_putcred:
+ put_rpccred(cred);
out:
return status;
}
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 5c35b02..c760558 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -129,6 +129,8 @@ nfs_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
sattr->ia_mode &= S_IALLUGO;
dprintk("NFS call setattr\n");
+ if (sattr->ia_valid & ATTR_FILE)
+ msg.rpc_cred = nfs_file_cred(sattr->ia_file);
nfs_fattr_init(fattr);
status = rpc_call_sync(NFS_CLIENT(inode), &msg, 0);
if (status == 0)