[PATCH] nfsd: handle get_client_locked() failure in nfsd4_setclientid_confirm()

[PATCH] nfsd: handle get_client_locked() failure in nfsd4_setclientid_confirm()

[Jeff Layton]

Lei Lu recently reported that nfsd4_setclientid_confirm() did not check
the return value from get_client_locked(). a SETCLIENTID_CONFIRM could
race with a confirmed client expiring and fail to get a reference. That
could later lead to a UAF.

Fix this by getting a reference early in the case where there is an
extant confirmed client. If that fails then treat it as if there were no
confirmed client found at all.

In the case where the unconfirmed client is expiring, just fail and
return the result from get_client_locked().

Reported-by: lei lu <llfamsec@gmail.com>
Closes: https://lore.kernel.org/linux-nfs/CAEBF3_b=UvqzNKdnfD_52L05Mqrqui9vZ2eFamgAbV0WG+FNWQ@mail.gmail.com/
Fixes: d20c11d86d8f ("nfsd: Protect session creation and client confirm using client_lock")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
I ran this vs. pynfs and it seemed to do OK. lei lu, can you test this
patch vs. your reproducer and tell us whether it fixes it?
---
 fs/nfsd/nfs4state.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d5694987f86fadab985e55cce6261ad680e83b69..d61a7910dde3b8536b8715c2eebd1f1faec95f8f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4697,10 +4697,16 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 	}
 	status = nfs_ok;
 	if (conf) {
-		old = unconf;
-		unhash_client_locked(old);
-		nfsd4_change_callback(conf, &unconf->cl_cb_conn);
-	} else {
+		if (get_client_locked(conf) == nfs_ok) {
+			old = unconf;
+			unhash_client_locked(old);
+			nfsd4_change_callback(conf, &unconf->cl_cb_conn);
+		} else {
+			conf = NULL;
+		}
+	}
+
+	if (!conf) {
 		old = find_confirmed_client_by_name(&unconf->cl_name, nn);
 		if (old) {
 			status = nfserr_clid_inuse;
@@ -4717,10 +4723,14 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			}
 			trace_nfsd_clid_replaced(&old->cl_clientid);
 		}
+		status = get_client_locked(unconf);
+		if (status != nfs_ok) {
+			old = NULL;
+			goto out;
+		}
 		move_to_confirmed(unconf);
 		conf = unconf;
 	}
-	get_client_locked(conf);
 	spin_unlock(&nn->client_lock);
 	if (conf == unconf)
 		fsnotify_dentry(conf->cl_nfsd_info_dentry, FS_MODIFY);

---
base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
change-id: 20250604-testing-8d988ff48076

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: handle get_client_locked() failure in nfsd4_setclientid_confirm()

[lei lu]

On Thu, Jun 5, 2025 at 12:01 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> Lei Lu recently reported that nfsd4_setclientid_confirm() did not check
> the return value from get_client_locked(). a SETCLIENTID_CONFIRM could
> race with a confirmed client expiring and fail to get a reference. That
> could later lead to a UAF.
>
> Fix this by getting a reference early in the case where there is an
> extant confirmed client. If that fails then treat it as if there were no
> confirmed client found at all.
>
> In the case where the unconfirmed client is expiring, just fail and
> return the result from get_client_locked().
>
> Reported-by: lei lu <llfamsec@gmail.com>
> Closes: https://lore.kernel.org/linux-nfs/CAEBF3_b=UvqzNKdnfD_52L05Mqrqui9vZ2eFamgAbV0WG+FNWQ@mail.gmail.com/
> Fixes: d20c11d86d8f ("nfsd: Protect session creation and client confirm using client_lock")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> I ran this vs. pynfs and it seemed to do OK. lei lu, can you test this
> patch vs. your reproducer and tell us whether it fixes it?

Patch works for me, the issue is fixed.

Thanks,
LL

> ---
>  fs/nfsd/nfs4state.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d5694987f86fadab985e55cce6261ad680e83b69..d61a7910dde3b8536b8715c2eebd1f1faec95f8f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4697,10 +4697,16 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>         }
>         status = nfs_ok;
>         if (conf) {
> -               old = unconf;
> -               unhash_client_locked(old);
> -               nfsd4_change_callback(conf, &unconf->cl_cb_conn);
> -       } else {
> +               if (get_client_locked(conf) == nfs_ok) {
> +                       old = unconf;
> +                       unhash_client_locked(old);
> +                       nfsd4_change_callback(conf, &unconf->cl_cb_conn);
> +               } else {
> +                       conf = NULL;
> +               }
> +       }
> +
> +       if (!conf) {
>                 old = find_confirmed_client_by_name(&unconf->cl_name, nn);
>                 if (old) {
>                         status = nfserr_clid_inuse;
> @@ -4717,10 +4723,14 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>                         }
>                         trace_nfsd_clid_replaced(&old->cl_clientid);
>                 }
> +               status = get_client_locked(unconf);
> +               if (status != nfs_ok) {
> +                       old = NULL;
> +                       goto out;
> +               }
>                 move_to_confirmed(unconf);
>                 conf = unconf;
>         }
> -       get_client_locked(conf);
>         spin_unlock(&nn->client_lock);
>         if (conf == unconf)
>                 fsnotify_dentry(conf->cl_nfsd_info_dentry, FS_MODIFY);
>
> ---
> base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
> change-id: 20250604-testing-8d988ff48076
>
> Best regards,
> --
> Jeff Layton <jlayton@kernel.org>
>

Re: [PATCH] nfsd: handle get_client_locked() failure in nfsd4_setclientid_confirm()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Wed, 04 Jun 2025 12:01:10 -0400, Jeff Layton wrote:
> Lei Lu recently reported that nfsd4_setclientid_confirm() did not check
> the return value from get_client_locked(). a SETCLIENTID_CONFIRM could
> race with a confirmed client expiring and fail to get a reference. That
> could later lead to a UAF.
> 
> Fix this by getting a reference early in the case where there is an
> extant confirmed client. If that fails then treat it as if there were no
> confirmed client found at all.
> 
> [...]

Applied to nfsd-testing, thanks!

[1/1] nfsd: handle get_client_locked() failure in nfsd4_setclientid_confirm()
      commit: 2de843e11f5d16b31742259f2d3929b681a2de32

--
Chuck Lever