On Thu, Jun 5, 2025 at 12:01 AM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxx> > Closes: https://lore.kernel.org/linux-nfs/CAEBF3_b=UvqzNKdnfD_52L05Mqrqui9vZ2eFamgAbV0WG+FNWQ@xxxxxxxxxxxxxx/ > Fixes: d20c11d86d8f ("nfsd: Protect session creation and client confirm using client_lock") > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> >
