* FAILED: patch "[PATCH] nfsd: check permissions when setting ACLs" failed to apply to 3.14-stable tree
@ 2016-07-11 22:42 gregkh
2016-07-12 20:20 ` J. Bruce Fields
0 siblings, 1 reply; 2+ messages in thread
From: gregkh @ 2016-07-11 22:42 UTC (permalink / raw)
To: ben, bfields, david, hch, viro; +Cc: stable
The patch below does not apply to the 3.14-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From 999653786df6954a31044528ac3f7a5dadca08f4 Mon Sep 17 00:00:00 2001
From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 22 Jun 2016 19:43:35 +0100
Subject: [PATCH] nfsd: check permissions when setting ACLs
Use set_posix_acl, which includes proper permission checks, instead of
calling ->set_acl directly. Without this anyone may be able to grant
themselves permissions to a file by setting the ACL.
Lock the inode to make the new checks atomic with respect to set_acl.
(Also, nfsd was the only caller of set_acl not locking the inode, so I
suspect this may fix other races.)
This also simplifies the code, and ensures our ACLs are checked by
posix_acl_valid.
The permission checks and the inode locking were lost with commit
4ac7249e, which changed nfsd to use the set_acl inode operation directly
instead of going through xattr handlers.
Reported-by: David Sinquin <david@sinquin.eu>
[agreunba@redhat.com: use set_posix_acl]
Fixes: 4ac7249e
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: stable@vger.kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index 1580ea6fd64d..d08cd88155c7 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -104,22 +104,21 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
goto out;
inode = d_inode(fh->fh_dentry);
- if (!IS_POSIXACL(inode) || !inode->i_op->set_acl) {
- error = -EOPNOTSUPP;
- goto out_errno;
- }
error = fh_want_write(fh);
if (error)
goto out_errno;
- error = inode->i_op->set_acl(inode, argp->acl_access, ACL_TYPE_ACCESS);
+ fh_lock(fh);
+
+ error = set_posix_acl(inode, ACL_TYPE_ACCESS, argp->acl_access);
if (error)
- goto out_drop_write;
- error = inode->i_op->set_acl(inode, argp->acl_default,
- ACL_TYPE_DEFAULT);
+ goto out_drop_lock;
+ error = set_posix_acl(inode, ACL_TYPE_DEFAULT, argp->acl_default);
if (error)
- goto out_drop_write;
+ goto out_drop_lock;
+
+ fh_unlock(fh);
fh_drop_write(fh);
@@ -131,7 +130,8 @@ out:
posix_acl_release(argp->acl_access);
posix_acl_release(argp->acl_default);
return nfserr;
-out_drop_write:
+out_drop_lock:
+ fh_unlock(fh);
fh_drop_write(fh);
out_errno:
nfserr = nfserrno(error);
diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index 01df4cd7c753..0c890347cde3 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -95,22 +95,20 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst * rqstp,
goto out;
inode = d_inode(fh->fh_dentry);
- if (!IS_POSIXACL(inode) || !inode->i_op->set_acl) {
- error = -EOPNOTSUPP;
- goto out_errno;
- }
error = fh_want_write(fh);
if (error)
goto out_errno;
- error = inode->i_op->set_acl(inode, argp->acl_access, ACL_TYPE_ACCESS);
+ fh_lock(fh);
+
+ error = set_posix_acl(inode, ACL_TYPE_ACCESS, argp->acl_access);
if (error)
- goto out_drop_write;
- error = inode->i_op->set_acl(inode, argp->acl_default,
- ACL_TYPE_DEFAULT);
+ goto out_drop_lock;
+ error = set_posix_acl(inode, ACL_TYPE_DEFAULT, argp->acl_default);
-out_drop_write:
+out_drop_lock:
+ fh_unlock(fh);
fh_drop_write(fh);
out_errno:
nfserr = nfserrno(error);
diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index 6adabd6049b7..71292a0d6f09 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -770,9 +770,6 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
dentry = fhp->fh_dentry;
inode = d_inode(dentry);
- if (!inode->i_op->set_acl || !IS_POSIXACL(inode))
- return nfserr_attrnotsupp;
-
if (S_ISDIR(inode->i_mode))
flags = NFS4_ACL_DIR;
@@ -782,16 +779,19 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
if (host_error < 0)
goto out_nfserr;
- host_error = inode->i_op->set_acl(inode, pacl, ACL_TYPE_ACCESS);
+ fh_lock(fhp);
+
+ host_error = set_posix_acl(inode, ACL_TYPE_ACCESS, pacl);
if (host_error < 0)
- goto out_release;
+ goto out_drop_lock;
if (S_ISDIR(inode->i_mode)) {
- host_error = inode->i_op->set_acl(inode, dpacl,
- ACL_TYPE_DEFAULT);
+ host_error = set_posix_acl(inode, ACL_TYPE_DEFAULT, dpacl);
}
-out_release:
+out_drop_lock:
+ fh_unlock(fhp);
+
posix_acl_release(pacl);
posix_acl_release(dpacl);
out_nfserr:
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: FAILED: patch "[PATCH] nfsd: check permissions when setting ACLs" failed to apply to 3.14-stable tree
2016-07-11 22:42 FAILED: patch "[PATCH] nfsd: check permissions when setting ACLs" failed to apply to 3.14-stable tree gregkh
@ 2016-07-12 20:20 ` J. Bruce Fields
0 siblings, 0 replies; 2+ messages in thread
From: J. Bruce Fields @ 2016-07-12 20:20 UTC (permalink / raw)
To: gregkh; +Cc: ben, david, hch, viro, stable
On Mon, Jul 11, 2016 at 03:42:18PM -0700, gregkh@linuxfoundation.org wrote:
>
> The patch below does not apply to the 3.14-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
Thanks, looks like only minor fixups are required, I'll send backports.
--b.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From 999653786df6954a31044528ac3f7a5dadca08f4 Mon Sep 17 00:00:00 2001
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Wed, 22 Jun 2016 19:43:35 +0100
> Subject: [PATCH] nfsd: check permissions when setting ACLs
>
> Use set_posix_acl, which includes proper permission checks, instead of
> calling ->set_acl directly. Without this anyone may be able to grant
> themselves permissions to a file by setting the ACL.
>
> Lock the inode to make the new checks atomic with respect to set_acl.
> (Also, nfsd was the only caller of set_acl not locking the inode, so I
> suspect this may fix other races.)
>
> This also simplifies the code, and ensures our ACLs are checked by
> posix_acl_valid.
>
> The permission checks and the inode locking were lost with commit
> 4ac7249e, which changed nfsd to use the set_acl inode operation directly
> instead of going through xattr handlers.
>
> Reported-by: David Sinquin <david@sinquin.eu>
> [agreunba@redhat.com: use set_posix_acl]
> Fixes: 4ac7249e
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> index 1580ea6fd64d..d08cd88155c7 100644
> --- a/fs/nfsd/nfs2acl.c
> +++ b/fs/nfsd/nfs2acl.c
> @@ -104,22 +104,21 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
> goto out;
>
> inode = d_inode(fh->fh_dentry);
> - if (!IS_POSIXACL(inode) || !inode->i_op->set_acl) {
> - error = -EOPNOTSUPP;
> - goto out_errno;
> - }
>
> error = fh_want_write(fh);
> if (error)
> goto out_errno;
>
> - error = inode->i_op->set_acl(inode, argp->acl_access, ACL_TYPE_ACCESS);
> + fh_lock(fh);
> +
> + error = set_posix_acl(inode, ACL_TYPE_ACCESS, argp->acl_access);
> if (error)
> - goto out_drop_write;
> - error = inode->i_op->set_acl(inode, argp->acl_default,
> - ACL_TYPE_DEFAULT);
> + goto out_drop_lock;
> + error = set_posix_acl(inode, ACL_TYPE_DEFAULT, argp->acl_default);
> if (error)
> - goto out_drop_write;
> + goto out_drop_lock;
> +
> + fh_unlock(fh);
>
> fh_drop_write(fh);
>
> @@ -131,7 +130,8 @@ out:
> posix_acl_release(argp->acl_access);
> posix_acl_release(argp->acl_default);
> return nfserr;
> -out_drop_write:
> +out_drop_lock:
> + fh_unlock(fh);
> fh_drop_write(fh);
> out_errno:
> nfserr = nfserrno(error);
> diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> index 01df4cd7c753..0c890347cde3 100644
> --- a/fs/nfsd/nfs3acl.c
> +++ b/fs/nfsd/nfs3acl.c
> @@ -95,22 +95,20 @@ static __be32 nfsd3_proc_setacl(struct svc_rqst * rqstp,
> goto out;
>
> inode = d_inode(fh->fh_dentry);
> - if (!IS_POSIXACL(inode) || !inode->i_op->set_acl) {
> - error = -EOPNOTSUPP;
> - goto out_errno;
> - }
>
> error = fh_want_write(fh);
> if (error)
> goto out_errno;
>
> - error = inode->i_op->set_acl(inode, argp->acl_access, ACL_TYPE_ACCESS);
> + fh_lock(fh);
> +
> + error = set_posix_acl(inode, ACL_TYPE_ACCESS, argp->acl_access);
> if (error)
> - goto out_drop_write;
> - error = inode->i_op->set_acl(inode, argp->acl_default,
> - ACL_TYPE_DEFAULT);
> + goto out_drop_lock;
> + error = set_posix_acl(inode, ACL_TYPE_DEFAULT, argp->acl_default);
>
> -out_drop_write:
> +out_drop_lock:
> + fh_unlock(fh);
> fh_drop_write(fh);
> out_errno:
> nfserr = nfserrno(error);
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index 6adabd6049b7..71292a0d6f09 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -770,9 +770,6 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> dentry = fhp->fh_dentry;
> inode = d_inode(dentry);
>
> - if (!inode->i_op->set_acl || !IS_POSIXACL(inode))
> - return nfserr_attrnotsupp;
> -
> if (S_ISDIR(inode->i_mode))
> flags = NFS4_ACL_DIR;
>
> @@ -782,16 +779,19 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (host_error < 0)
> goto out_nfserr;
>
> - host_error = inode->i_op->set_acl(inode, pacl, ACL_TYPE_ACCESS);
> + fh_lock(fhp);
> +
> + host_error = set_posix_acl(inode, ACL_TYPE_ACCESS, pacl);
> if (host_error < 0)
> - goto out_release;
> + goto out_drop_lock;
>
> if (S_ISDIR(inode->i_mode)) {
> - host_error = inode->i_op->set_acl(inode, dpacl,
> - ACL_TYPE_DEFAULT);
> + host_error = set_posix_acl(inode, ACL_TYPE_DEFAULT, dpacl);
> }
>
> -out_release:
> +out_drop_lock:
> + fh_unlock(fhp);
> +
> posix_acl_release(pacl);
> posix_acl_release(dpacl);
> out_nfserr:
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-07-12 20:20 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-11 22:42 FAILED: patch "[PATCH] nfsd: check permissions when setting ACLs" failed to apply to 3.14-stable tree gregkh
2016-07-12 20:20 ` J. Bruce Fields
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).