linux-kernel - Re: [PATCH v2] nfs: fix the race of lock/unlock and open

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] [day] [month] [year] [list]
Message-ID: <03488bcb-f2c2-4da7-913e-d262ff73ada3@huawei.com>
Date: Mon, 1 Sep 2025 22:25:14 +0800
From: Li Lingfeng <lilingfeng3@...wei.com>
To: <trondmy@...nel.org>, <anna@...nel.org>, <jlayton@...nel.org>,
	<bcodding@...hat.com>
CC: <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<yukuai1@...weicloud.com>, <houtao1@...wei.com>, <yi.zhang@...wei.com>,
	<yangerkun@...wei.com>, <zhangjian496@...artners.com>,
	<lilingfeng@...weicloud.com>
Subject: Re: [PATCH v2] nfs: fix the race of lock/unlock and open

Friendly ping..

Thanks

在 2025/7/15 11:05, Li Lingfeng 写道:
> LOCK may extend an existing lock and release another one and UNLOCK may
> also release an existing lock.
> When opening a file, there may be access to file locks that have been
> concurrently released by lock/unlock operations, potentially triggering
> UAF.
> While certain concurrent scenarios involving lock/unlock and open
> operations have been safeguarded with locks – for example,
> nfs4_proc_unlckz() acquires the so_delegreturn_mutex prior to invoking
> locks_lock_inode_wait() – there remain cases where such protection is not
> yet implemented.
>
> The issue can be reproduced through the following steps:
> T1: open in read-only mode with three consecutive lock operations applied
>      lock1(0~100) --> add lock1 to file
>      lock2(120~200) --> add lock2 to file
>      lock3(50~150) --> extend lock1 to cover range 0~200 and release lock2
> T2: restart nfs-server and run state manager
> T3: open in write-only mode
>      T1                            T2                                T3
>                              start recover
> lock1
> lock2
>                              nfs4_open_reclaim
>                              clear_bit // NFS_DELEGATED_STATE
> lock3
>   _nfs4_proc_setlk
>    lock so_delegreturn_mutex
>    unlock so_delegreturn_mutex
>    _nfs4_do_setlk
>                              recover done
>                                                  lock so_delegreturn_mutex
>                                                  nfs_delegation_claim_locks
>                                                  get lock2
>     rpc_run_task
>     ...
>     nfs4_lock_done
>      locks_lock_inode_wait
>      ...
>       locks_dispose_list
>       free lock2
>                                                  use lock2
>                                                  // UAF
>                                                  unlock so_delegreturn_mutex
>
> Protect file lock by nfsi->rwsem to fix this issue.
>
> Fixes: c69899a17ca4 ("NFSv4: Update of VFS byte range lock must be atomic with the stateid update")
> Reported-by: zhangjian (CG) <zhangjian496@...wei.com>
> Suggested-by: yangerkun <yangerkun@...wei.com>
> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
> ---
> Changes in v2:
>    Use nfsi->rwsem instead of sp->so_delegreturn_mutex to prevent concurrency.
>
>   fs/nfs/delegation.c | 5 ++++-
>   fs/nfs/nfs4proc.c   | 8 +++++++-
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 10ef46e29b25..4467b4f61905 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -149,15 +149,17 @@ int nfs4_check_delegation(struct inode *inode, fmode_t type)
>   static int nfs_delegation_claim_locks(struct nfs4_state *state, const nfs4_stateid *stateid)
>   {
>   	struct inode *inode = state->inode;
> +	struct nfs_inode *nfsi = NFS_I(inode);
>   	struct file_lock *fl;
>   	struct file_lock_context *flctx = locks_inode_context(inode);
>   	struct list_head *list;
>   	int status = 0;
>   
>   	if (flctx == NULL)
> -		goto out;
> +		return status;
>   
>   	list = &flctx->flc_posix;
> +	down_write(&nfsi->rwsem);
>   	spin_lock(&flctx->flc_lock);
>   restart:
>   	for_each_file_lock(fl, list) {
> @@ -175,6 +177,7 @@ static int nfs_delegation_claim_locks(struct nfs4_state *state, const nfs4_state
>   	}
>   	spin_unlock(&flctx->flc_lock);
>   out:
> +	up_write(&nfsi->rwsem);
>   	return status;
>   }
>   
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 341740fa293d..06f109c7eb2e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7294,14 +7294,18 @@ static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *
>   	status = -ENOMEM;
>   	if (IS_ERR(seqid))
>   		goto out;
> +	down_read(&nfsi->rwsem);
>   	task = nfs4_do_unlck(request,
>   			     nfs_file_open_context(request->c.flc_file),
>   			     lsp, seqid);
>   	status = PTR_ERR(task);
> -	if (IS_ERR(task))
> +	if (IS_ERR(task)) {
> +		up_read(&nfsi->rwsem);
>   		goto out;
> +	}
>   	status = rpc_wait_for_completion_task(task);
>   	rpc_put_task(task);
> +	up_read(&nfsi->rwsem);
>   out:
>   	request->c.flc_flags = saved_flags;
>   	trace_nfs4_unlock(request, state, F_SETLK, status);
> @@ -7642,7 +7646,9 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
>   	}
>   	up_read(&nfsi->rwsem);
>   	mutex_unlock(&sp->so_delegreturn_mutex);
> +	down_read(&nfsi->rwsem);
>   	status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW);
> +	up_read(&nfsi->rwsem);
>   out:
>   	request->c.flc_flags = flags;
>   	return status;