| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 3 Jan 2013 17:46:56 -0500
From: "J. Bruce Fields" <bfields@...ldses.org>
To: Pawel Dziepak <pdziepak@...rnos.org>
Cc: linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH] nfsd: Don't overuse NFS?ERR_PERM error code
On Tue, Dec 25, 2012 at 12:30:42AM +0100, Pawel Dziepak wrote:
> From: Pawel Dziepak <pdziepak@...rnos.org>
>
> Unlike EPERM, NFS?ERR_PERM error code applies only when the operation could
> not be completed because the caller is either not the owner or not a
> privileged user. Actually, only OPEN, CREATE and SETATTR are supposed to
> ever return such error code when a problem occurs when changing or setting
> access permissions.
>
> Generally, this patch replaces NFS?ERR_PERM with NFS?ERR_ACCESS which is a more
> appropriate error code indicating that the caller is not allowed to perform the
> requested operation.
>
> In case of incorrect {file,directory} names either NFS?ERR_INVAL or
> NFS4ERR_BADNAME is returned.
>
> Opening for write files with append-only bit set or mandatory locking enabled
> results in NFS4ERR_SHARE_DENIED.
>
> Signed-off-by: Pawel Dziepak <pdziepak@...rnos.org>
> ---
> fs/nfsd/nfsfh.c | 2 +-
> fs/nfsd/vfs.c | 28 ++++++++++++++++------------
> 2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 814afaa..b18382c 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -91,7 +91,7 @@ static __be32 nfsd_setuser_and_check_port(struct
> svc_rqst *rqstp,
> dprintk(KERN_WARNING
> "nfsd: request from insecure port %s!\n",
> svc_print_addr(rqstp, buf, sizeof(buf)));
> - return nfserr_perm;
> + return nfserr_acces;
Why is eaccess better in this particular case?
> }
>
> /* Set user creds for this exportpoint */
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index d586117..a9964de 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -777,7 +777,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh
> *fhp, umode_t type,
> /* Disallow write access to files with the append-only bit set
> * or any access when mandatory locking enabled
> */
> - err = nfserr_perm;
> + err = nfserr_share_denied;
This results in returning a v4-only error on some v2 and v3
routines--definitely not OK.
> if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE))
> goto out;
> /*
> @@ -924,8 +924,6 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct
> svc_fh *fhp, struct file *file,
> __be32 err;
> int host_err;
>
> - err = nfserr_perm;
> -
Hm, yeah there's no way out of this routine without clobbering this
value later, I don't know why we have this assignment. OK.
> if (file->f_op->splice_read && rqstp->rq_splice_ok) {
> struct splice_desc sd = {
> .len = 0,
> @@ -1246,7 +1244,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> __be32 err2;
> int host_err;
>
> - err = nfserr_perm;
> + err = nfserr_inval;
> if (!flen)
> goto out;
OK, I guess. I'm curious though why you care which error you get when
operating on a zero-length filehandle.
Is there a practical problem you're trying to solve here?
> err = nfserr_exist;
> @@ -1387,7 +1385,7 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> int host_err;
> __u32 v_mtime=0, v_atime=0;
>
> - err = nfserr_perm;
> + err = nfserr_inval;
> if (!flen)
> goto out;
> err = nfserr_exist;
> @@ -1675,7 +1673,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> err = nfserr_isdir;
> if (S_ISDIR(tfhp->fh_dentry->d_inode->i_mode))
> goto out;
> - err = nfserr_perm;
> + err = nfserr_inval;
> if (!len)
> goto out;
> err = nfserr_exist;
> @@ -1761,8 +1759,11 @@ nfsd_rename(struct svc_rqst *rqstp, struct
> svc_fh *ffhp, char *fname, int flen,
> if (ffhp->fh_export != tfhp->fh_export)
> goto out;
>
> - err = nfserr_perm;
> - if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
> + err = nfserr_inval;
> + if (!flen || !tlen)
> + goto out;
> + err = nfsd_v4client(rqstp) ? nfserr_badname : nfserr_inval;
> + if (isdotent(fname, flen) || isdotent(tname, tlen))
> goto out;
>
> host_err = fh_want_write(ffhp);
> @@ -1850,8 +1851,11 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> svc_fh *fhp, int type,
> __be32 err;
> int host_err;
>
> - err = nfserr_acces;
> - if (!flen || isdotent(fname, flen))
> + err = nfserr_inval;
> + if (!flen)
> + goto out;
> + err = nfsd_v4client(rqstp) ? nfserr_badname : nfserr_inval;
> + if (isdotent(fname, flen))
> goto out;
> err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_REMOVE);
> if (err)
> @@ -2120,10 +2124,10 @@ nfsd_permission(struct svc_rqst *rqstp, struct
> svc_export *exp,
> __mnt_is_readonly(exp->ex_path.mnt))
> return nfserr_rofs;
> if (/* (acc & NFSD_MAY_WRITE) && */ IS_IMMUTABLE(inode))
> - return nfserr_perm;
> + return nfserr_acces;
I'm confused about that case. This is consistent with
fs/namei.c:__inode_permission, but not with any other IS_IMMUTABLE
checks that I can find.
I would have thought this sort of "nobody can do this" case would be
EPERM.
> }
> if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
> - return nfserr_perm;
> + return nfserr_acces;
Ditto, this is inconsistent with other uses.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists