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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <227e2d809951caa485e8ea03978afea515e89464.camel@kernel.org>
Date: Mon, 26 Aug 2024 10:37:51 -0400
From: Jeff Layton <jlayton@...nel.org>
To: NeilBrown <neilb@...e.de>, Chuck Lever <chuck.lever@...cle.com>
Cc: Olga Kornievskaia <kolga@...app.com>, Dai Ngo <dai.ngo@...cle.com>, Tom
 Talpey <tom@...pey.com>, "linux-nfs@...r.kernel.org"
 <linux-nfs@...r.kernel.org>,  "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> On Sun, 25 Aug 2024, Chuck Lever wrote:
> > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > delstid set. Mostly this just ensures that we hold references to the
> > > delegation while working with it.
> > > 
> > > 
> > 
> > Applied to nfsd-fixes for v6.11-rc, thanks!
> > 
> > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> >       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> >       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> > 
> > -- 
> > Chuck Lever
> > 
> 
> Maybe the following can tidy up that code.  I can split this into
> a few separate patches if you like.
> Thoughts?
> 
> Note that the patch is easier to review if you apply it then use "git
> diff -b".
> 
> NeilBrown
> 
> 
> From: NeilBrown <neilb@...e.de>
> Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
> 
> The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
> 
> With this patch we:
>  - properly handle non-nfsd leases.  We must not assume flc_owner is a
>     delegation unless fl_lmops == &nfsd_lease_mng_ops

AFAICT, non-nfsd leases are already properly handled (though I do agree
that the "flow" of this code is awkward). What case do you see that's
wrong?

>  - move the main code out of the for loop
>  - have a single exit which calls nfs4_put_stid()
>    (and other exits which don't need to call that)
> 
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: NeilBrown <neilb@...e.de>
> ---
>  fs/nfsd/nfs4state.c | 130 ++++++++++++++++++++++----------------------
>  1 file changed, 65 insertions(+), 65 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2c4b9a22b2bb..7672fa7a70f3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8837,6 +8837,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	struct inode *inode = d_inode(dentry);
>  	struct file_lock_context *ctx;
> +	struct nfs4_delegation *dp = NULL;
>  	struct nfs4_cb_fattr *ncf;
>  	struct file_lease *fl;
>  	struct iattr attrs;
> @@ -8845,77 +8846,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
>  	ctx = locks_inode_context(inode);
>  	if (!ctx)
>  		return 0;
> +
> +#define NON_NFSD_LEASE ((void*)1)
> +
>  	spin_lock(&ctx->flc_lock);
>  	for_each_file_lock(fl, &ctx->flc_lease) {
> -		unsigned char type = fl->c.flc_type;
> -
>  		if (fl->c.flc_flags == FL_LAYOUT)
>  			continue;
> -		if (fl->fl_lmops != &nfsd_lease_mng_ops) {
> -			/*
> -			 * non-nfs lease, if it's a lease with F_RDLCK then
> -			 * we are done; there isn't any write delegation
> -			 * on this inode
> -			 */
> -			if (type == F_RDLCK)
> -				break;
> -			goto break_lease;
> -		}
> -		if (type == F_WRLCK) {
> -			struct nfs4_delegation *dp = fl->c.flc_owner;
> -
> -			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> -				spin_unlock(&ctx->flc_lock);
> -				return 0;
> -			}
> -break_lease:
> -			nfsd_stats_wdeleg_getattr_inc(nn);
> -			dp = fl->c.flc_owner;
> -			refcount_inc(&dp->dl_stid.sc_count);
> -			ncf = &dp->dl_cb_fattr;
> -			nfs4_cb_getattr(&dp->dl_cb_fattr);
> -			spin_unlock(&ctx->flc_lock);
> -			wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> -					TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> -			if (ncf->ncf_cb_status) {
> -				/* Recall delegation only if client didn't respond */
> -				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> -				if (status != nfserr_jukebox ||
> -						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> -					nfs4_put_stid(&dp->dl_stid);
> -					return status;
> -				}
> -			}
> -			if (!ncf->ncf_file_modified &&
> -					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> -					ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> -				ncf->ncf_file_modified = true;
> -			if (ncf->ncf_file_modified) {
> -				int err;
> -
> -				/*
> -				 * Per section 10.4.3 of RFC 8881, the server would
> -				 * not update the file's metadata with the client's
> -				 * modified size
> -				 */
> -				attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> -				attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> -				inode_lock(inode);
> -				err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> -				inode_unlock(inode);
> -				if (err) {
> -					nfs4_put_stid(&dp->dl_stid);
> -					return nfserrno(err);
> -				}
> -				ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> -				*size = ncf->ncf_cur_fsize;
> -				*modified = true;
> -			}
> -			nfs4_put_stid(&dp->dl_stid);
> -			return 0;
> +		if (fl->c.flc_type == F_WRLCK) {
> +			if (fl->fl_lmops == &nfsd_lease_mng_ops)
> +				dp = fl->c.flc_owner;
> +			else
> +				dp = NON_NFSD_LEASE;
>  		}
>  		break;
>  	}
> +	if (dp == NULL || dp == NON_NFSD_LEASE ||
> +	    dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> +		spin_unlock(&ctx->flc_lock);
> +		if (dp == NON_NFSD_LEASE) {
> +			status = nfserrno(nfsd_open_break_lease(inode,
> +								NFSD_MAY_READ));
> +			if (status != nfserr_jukebox ||
> +			    !nfsd_wait_for_delegreturn(rqstp, inode))
> +				return status;
> +		}
> +		return 0;
> +	}
> +
> +	nfsd_stats_wdeleg_getattr_inc(nn);
> +	refcount_inc(&dp->dl_stid.sc_count);
> +	ncf = &dp->dl_cb_fattr;
> +	nfs4_cb_getattr(&dp->dl_cb_fattr);
>  	spin_unlock(&ctx->flc_lock);
> -	return 0;
> +
> +	wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> +			    TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> +	if (ncf->ncf_cb_status) {
> +		/* Recall delegation only if client didn't respond */
> +		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> +		if (status != nfserr_jukebox ||
> +		    !nfsd_wait_for_delegreturn(rqstp, inode))
> +			goto out_status;
> +	}
> +	if (!ncf->ncf_file_modified &&
> +	    (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> +	     ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> +		ncf->ncf_file_modified = true;
> +	if (ncf->ncf_file_modified) {
> +		int err;
> +
> +		/*
> +		 * Per section 10.4.3 of RFC 8881, the server would
> +		 * not update the file's metadata with the client's
> +		 * modified size
> +		 */
> +		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> +		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> +		inode_lock(inode);
> +		err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> +		inode_unlock(inode);
> +		if (err) {
> +			status = nfserrno(err);
> +			goto out_status;
> +		}
> +		ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> +		*size = ncf->ncf_cur_fsize;
> +		*modified = true;
> +	}
> +	status = 0;
> +out_status:
> +	nfs4_put_stid(&dp->dl_stid);
> +	return status;
>  }

Patch looks like a nice cleanup, but I don't think the Fixes tag is
appropriate here.
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ