diff options
author | J. Bruce Fields <bfields@citi.umich.edu> | 2009-11-25 17:42:05 -0500 |
---|---|---|
committer | J. Bruce Fields <bfields@citi.umich.edu> | 2009-11-25 17:55:46 -0500 |
commit | 864f0f61f829bac5f150a903aad9619322a25424 (patch) | |
tree | 77a864ab5538255dfba454d13f67de60807f2973 /fs | |
parent | 9b8b317d58084b9a44f6f33b355c4278d9f841fb (diff) |
nfsd: simplify fh_verify access checks
All nfsd security depends on the security checks in fh_verify, and
especially on nfsd_setuser().
It therefore bothers me that the nfsd_setuser call may be made from
three different places, depending on whether the filehandle has already
been mapped to a dentry, and on whether subtreechecking is in force.
Instead, make an unconditional call in fh_verify(), so it's trivial to
verify that the call always occurs.
That leaves us with a redundant nfsd_setuser() call in the subtreecheck
case--it needs the correct user set earlier in order to check execute
permissions on the path to this filehandle--but I'm willing to accept
that minor inefficiency in the subtreecheck case in return for more
straightforward permission checking.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Diffstat (limited to 'fs')
-rw-r--r-- | fs/nfsd/nfsfh.c | 50 |
1 files changed, 21 insertions, 29 deletions
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index d0d8a217a3ea..a77efb8c2243 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -233,14 +233,6 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) goto out; } - if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) { - error = nfsd_setuser_and_check_port(rqstp, exp); - if (error) { - dput(dentry); - goto out; - } - } - if (S_ISDIR(dentry->d_inode->i_mode) && (dentry->d_flags & DCACHE_DISCONNECTED)) { printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %s/%s\n", @@ -295,28 +287,28 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int access) error = nfsd_set_fh_dentry(rqstp, fhp); if (error) goto out; - dentry = fhp->fh_dentry; - exp = fhp->fh_export; - } else { - /* - * just rechecking permissions - * (e.g. nfsproc_create calls fh_verify, then nfsd_create - * does as well) - */ - dprintk("nfsd: fh_verify - just checking\n"); - dentry = fhp->fh_dentry; - exp = fhp->fh_export; - /* - * Set user creds for this exportpoint; necessary even - * in the "just checking" case because this may be a - * filehandle that was created by fh_compose, and that - * is about to be used in another nfsv4 compound - * operation. - */ - error = nfsd_setuser_and_check_port(rqstp, exp); - if (error) - goto out; } + dentry = fhp->fh_dentry; + exp = fhp->fh_export; + /* + * We still have to do all these permission checks, even when + * fh_dentry is already set: + * - fh_verify may be called multiple times with different + * "access" arguments (e.g. nfsd_proc_create calls + * fh_verify(...,NFSD_MAY_EXEC) first, then later (in + * nfsd_create) calls fh_verify(...,NFSD_MAY_CREATE). + * - in the NFSv4 case, the filehandle may have been filled + * in by fh_compose, and given a dentry, but further + * compound operations performed with that filehandle + * still need permissions checks. In the worst case, a + * mountpoint crossing may have changed the export + * options, and we may now need to use a different uid + * (for example, if different id-squashing options are in + * effect on the new filesystem). + */ + error = nfsd_setuser_and_check_port(rqstp, exp); + if (error) + goto out; error = nfsd_mode_check(rqstp, dentry->d_inode->i_mode, type); if (error) |