[PATCH] nfsd: zero out umask if the client didn't provide one

[PATCH] nfsd: zero out umask if the client didn't provide one

[Lucas Stach]

When the server is serving a mixed set of clients where some support the
NFS 4.2 umask attribute and some don't, we need to make sure to reset the
nfd thread umask to 0 when serving a client that isn't providing the umask,
otherwise a stale umask from a previous request will be applied.

This was only done in the nfsd4_decode_open() callsite, but not in
nfsd4_decode_create() leading to newly created directories ending up with
wrong permissions in some cases. To avoid any such inconsistency in the
future, reset the umask in nfsd4_decode_fattr() where we know if the
client did provide a umask.

Fixes: 47057abde515 (nfsd: add support for the umask attribute)
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 fs/nfsd/nfs4xdr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e502fd16246b..1eb33b34c51b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -486,6 +486,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		dummy32 = be32_to_cpup(p++);
 		*umask = dummy32 & S_IRWXUGO;
 		iattr->ia_valid |= ATTR_MODE;
+	} else if (umask) {
+		*umask = 0;
 	}
 	if (len != expected_len)
 		goto xdr_error;
@@ -927,7 +929,6 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 	case NFS4_OPEN_NOCREATE:
 		break;
 	case NFS4_OPEN_CREATE:
-		current->fs->umask = 0;
 		READ_BUF(4);
 		open->op_createmode = be32_to_cpup(p++);
 		switch (open->op_createmode) {
-- 
2.16.1

Re: [PATCH] nfsd: zero out umask if the client didn't provide one

[J . Bruce Fields]

On Wed, Mar 21, 2018 at 08:52:42PM +0100, Lucas Stach wrote:
> When the server is serving a mixed set of clients where some support the
> NFS 4.2 umask attribute and some don't, we need to make sure to reset the
> nfd thread umask to 0 when serving a client that isn't providing the umask,
> otherwise a stale umask from a previous request will be applied.
> 
> This was only done in the nfsd4_decode_open() callsite, but not in
> nfsd4_decode_create() leading to newly created directories ending up with
> wrong permissions in some cases. To avoid any such inconsistency in the
> future, reset the umask in nfsd4_decode_fattr() where we know if the
> client did provide a umask.

Thanks for catching this!  (Is it because you were seeing directories
created with incorrect permissions in production?)

If the next rpc handled by this thread doesn't include a file attribute,
or isn't an NFSv4 request (maybe it's NFSv3), then it won't hit this
code and will still use the non-zero umask.

I was thinking of initializing it in nfsd_setuser, or maybe on each pass
through the loop that processes each op of a compound in
nfsd4_proc_compound....  But I think that would clear umask too
often--it'd clear the umask before it was actually used.

Actually I don't think there's any correct time to clear the umask, the
problem is where we're setting it.

We decode the whole compound in one pass, then process each op of an
NFSv4 compound.  That means the last op containing a set of the umask
will determine the umask used for the whole compound.  That seems wrong.

I mean, in practice no real client sends compounds with multiple create
operations, so maybe this is all academic, but still I think the correct
thing to do is stop setting current->fs->umask in the xdr decoder and
instead pass the umask value in the compound op arguments and set it
later in the proc code.

--b.

> 
> Fixes: 47057abde515 (nfsd: add support for the umask attribute)
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  fs/nfsd/nfs4xdr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e502fd16246b..1eb33b34c51b 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -486,6 +486,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
>  		dummy32 = be32_to_cpup(p++);
>  		*umask = dummy32 & S_IRWXUGO;
>  		iattr->ia_valid |= ATTR_MODE;
> +	} else if (umask) {
> +		*umask = 0;
>  	}
>  	if (len != expected_len)
>  		goto xdr_error;
> @@ -927,7 +929,6 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
>  	case NFS4_OPEN_NOCREATE:
>  		break;
>  	case NFS4_OPEN_CREATE:
> -		current->fs->umask = 0;
>  		READ_BUF(4);
>  		open->op_createmode = be32_to_cpup(p++);
>  		switch (open->op_createmode) {
> -- 
> 2.16.1

Re: [PATCH] nfsd: zero out umask if the client didn't provide one

[Lucas Stach]

Am Mittwoch, den 21.03.2018, 17:35 -0400 schrieb J . Bruce Fields:
> On Wed, Mar 21, 2018 at 08:52:42PM +0100, Lucas Stach wrote:
> > When the server is serving a mixed set of clients where some support the
> > NFS 4.2 umask attribute and some don't, we need to make sure to reset the
> > nfd thread umask to 0 when serving a client that isn't providing the umask,
> > otherwise a stale umask from a previous request will be applied.
> > 
> > This was only done in the nfsd4_decode_open() callsite, but not in
> > nfsd4_decode_create() leading to newly created directories ending up with
> > wrong permissions in some cases. To avoid any such inconsistency in the
> > future, reset the umask in nfsd4_decode_fattr() where we know if the
> > client did provide a umask.
> 
> Thanks for catching this!  (Is it because you were seeing directories
> created with incorrect permissions in production?)

Yes, we have hit this in our internal infrastructure.

> If the next rpc handled by this thread doesn't include a file attribute,
> or isn't an NFSv4 request (maybe it's NFSv3), then it won't hit this
> code and will still use the non-zero umask.

The thread umask seem to be only relevant for the open/create RPC
calls, but you are right about the NFS3 case. I didn't think of that.

> I was thinking of initializing it in nfsd_setuser, or maybe on each pass
> through the loop that processes each op of a compound in
> nfsd4_proc_compound....  But I think that would clear umask too
> often--it'd clear the umask before it was actually used.
> 
> Actually I don't think there's any correct time to clear the umask, the
> problem is where we're setting it.
> 
> We decode the whole compound in one pass, then process each op of an
> NFSv4 compound.  That means the last op containing a set of the umask
> will determine the umask used for the whole compound.  That seems wrong.
> 
> I mean, in practice no real client sends compounds with multiple create
> operations, so maybe this is all academic, but still I think the correct
> thing to do is stop setting current->fs->umask in the xdr decoder and
> instead pass the umask value in the compound op arguments and set it
> later in the proc code.

Yes, this seems like a better way to deal with this. I'll respin this
patch.

Thanks,
Lucas

Re: [PATCH] nfsd: zero out umask if the client didn't provide one

[J . Bruce Fields]

This is what I'm planning to apply (pending some testing).

--b.

commit 880a3a532548
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Mar 21 17:19:02 2018 -0400

    nfsd: fix incorrect umasks
    
    We're neglecting to clear the umask after it's set, which can cause a
    later unrelated rpc to (incorrectly) use the same umask if it happens to
    be processed by the same thread.
    
    There's a more subtle problem here too:
    
    An NFSv4 compound request is decoded all in one pass before any
    operations are executed.
    
    Currently we're setting current->fs->umask at the time we decode the
    compound.  In theory a single compound could contain multiple creates
    each setting a umask.  In that case we'd end up using whichever umask
    was passed in the *last* operation as the umask for all the creates,
    whether that was correct or not.
    
    So, we should just be saving the umask at decode time and waiting to set
    it until we actually process the corresponding operation.
    
    In practice it's unlikely any client would do multiple creates in a
    single compound.  And even if it did they'd likely be from the same
    process (hence carry the same umask).  So this is a little academic, but
    we should get it right anyway.
    
    Fixes: 47057abde515 (nfsd: add support for the umask attribute)
    Cc: stable@vger.kernel.org
    Reported-by: Lucash Stach <l.stach@pengutronix.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index c6157ece46b1..5d99e8810b85 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -32,6 +32,7 @@
  *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
+#include <linux/fs_struct.h>
 #include <linux/file.h>
 #include <linux/falloc.h>
 #include <linux/slab.h>
@@ -252,11 +253,13 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		 * Note: create modes (UNCHECKED,GUARDED...) are the same
 		 * in NFSv4 as in v3 except EXCLUSIVE4_1.
 		 */
+		current->fs->umask = open->op_umask;
 		status = do_nfsd_create(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
 					*resfh, open->op_createmode,
 					(u32 *)open->op_verf.data,
 					&open->op_truncate, &open->op_created);
+		current->fs->umask = 0;
 
 		if (!status && open->op_label.len)
 			nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
@@ -603,6 +606,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		return status;
 
+	current->fs->umask = create->cr_umask;
 	switch (create->cr_type) {
 	case NF4LNK:
 		status = nfsd_symlink(rqstp, &cstate->current_fh,
@@ -611,20 +615,22 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		break;
 
 	case NF4BLK:
+		status = nfserr_inval;
 		rdev = MKDEV(create->cr_specdata1, create->cr_specdata2);
 		if (MAJOR(rdev) != create->cr_specdata1 ||
 		    MINOR(rdev) != create->cr_specdata2)
-			return nfserr_inval;
+			goto out_umask;
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
 				     &create->cr_iattr, S_IFBLK, rdev, &resfh);
 		break;
 
 	case NF4CHR:
+		status = nfserr_inval;
 		rdev = MKDEV(create->cr_specdata1, create->cr_specdata2);
 		if (MAJOR(rdev) != create->cr_specdata1 ||
 		    MINOR(rdev) != create->cr_specdata2)
-			return nfserr_inval;
+			goto out_umask;
 		status = nfsd_create(rqstp, &cstate->current_fh,
 				     create->cr_name, create->cr_namelen,
 				     &create->cr_iattr,S_IFCHR, rdev, &resfh);
@@ -668,6 +674,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	fh_dup2(&cstate->current_fh, &resfh);
 out:
 	fh_put(&resfh);
+out_umask:
+	current->fs->umask = 0;
 	return status;
 }
 
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index dd72cdf3662e..1d048dd95464 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -33,7 +33,6 @@
  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <linux/fs_struct.h>
 #include <linux/file.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
@@ -682,7 +681,7 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
 
 	status = nfsd4_decode_fattr(argp, create->cr_bmval, &create->cr_iattr,
 				    &create->cr_acl, &create->cr_label,
-				    &current->fs->umask);
+				    &create->cr_umask);
 	if (status)
 		goto out;
 
@@ -927,7 +926,6 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 	case NFS4_OPEN_NOCREATE:
 		break;
 	case NFS4_OPEN_CREATE:
-		current->fs->umask = 0;
 		READ_BUF(4);
 		open->op_createmode = be32_to_cpup(p++);
 		switch (open->op_createmode) {
@@ -935,7 +933,7 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 		case NFS4_CREATE_GUARDED:
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
 				&open->op_iattr, &open->op_acl, &open->op_label,
-				&current->fs->umask);
+				&open->op_umask);
 			if (status)
 				goto out;
 			break;
@@ -950,7 +948,7 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open)
 			COPYMEM(open->op_verf.data, NFS4_VERIFIER_SIZE);
 			status = nfsd4_decode_fattr(argp, open->op_bmval,
 				&open->op_iattr, &open->op_acl, &open->op_label,
-				&current->fs->umask);
+				&open->op_umask);
 			if (status)
 				goto out;
 			break;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 468020fe2a07..17c453a7999c 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -119,6 +119,7 @@ struct nfsd4_create {
 	} u;
 	u32		cr_bmval[3];        /* request */
 	struct iattr	cr_iattr;           /* request */
+	int		cr_umask;           /* request */
 	struct nfsd4_change_info  cr_cinfo; /* response */
 	struct nfs4_acl *cr_acl;
 	struct xdr_netobj cr_label;
@@ -230,6 +231,7 @@ struct nfsd4_open {
 	u32		op_why_no_deleg;    /* response - DELEG_NONE_EXT only */
 	u32		op_create;     	    /* request */
 	u32		op_createmode;      /* request */
+	int		op_umask;           /* request */
 	u32		op_bmval[3];        /* request */
 	struct iattr	op_iattr;           /* UNCHECKED4, GUARDED4, EXCLUSIVE4_1 */
 	nfs4_verifier	op_verf __attribute__((aligned(32)));