| Message ID | 20211101200623.2635785-2-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix nfs4_slot use-after-free by FREE_STATEID | expand |
Hi Scott, Thanks. This mostly looks good, but I do have 1 comment below. On Mon, 2021-11-01 at 16:06 -0400, Scott Mayhew wrote: > During umount, the session slot tables are freed. If there are > outstanding FREE_STATEID tasks, a use-after-free and slab corruption > can > occur when rpc_exit_task calls rpc_call_done -> nfs41_sequence_done - > > > nfs4_sequence_process/nfs41_sequence_free_slot. > > Prevent that from happening by taking a reference on the nfs_client > in > nfs41_free_stateid and putting it in nfs41_free_stateid_release. > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > fs/nfs/nfs4proc.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index e1214bb6b7ee..76e6786b797e 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -10145,18 +10145,24 @@ static void > nfs41_free_stateid_prepare(struct rpc_task *task, void *calldata) > static void nfs41_free_stateid_done(struct rpc_task *task, void > *calldata) > { > struct nfs_free_stateid_data *data = calldata; > + struct nfs_client *clp = data->server->nfs_client; > > nfs41_sequence_done(task, &data->res.seq_res); > > switch (task->tk_status) { > case -NFS4ERR_DELAY: > if (nfs4_async_handle_error(task, data->server, NULL, > NULL) == -EAGAIN) > - rpc_restart_call_prepare(task); > + if (refcount_read(&clp->cl_count) > 1) > + rpc_restart_call_prepare(task); Do we really need to make the rpc restart call conditional here? Most servers prefer that you finish freeing state before calling DESTROY_CLIENTID. > } > } > > static void nfs41_free_stateid_release(void *calldata) > { > + struct nfs_free_stateid_data *data = calldata; > + struct nfs_client *clp = data->server->nfs_client; > + > + nfs_put_client(clp); > kfree(calldata); > } > > @@ -10193,6 +10199,10 @@ static int nfs41_free_stateid(struct > nfs_server *server, > }; > struct nfs_free_stateid_data *data; > struct rpc_task *task; > + struct nfs_client *clp = server->nfs_client; > + > + if (!refcount_inc_not_zero(&clp->cl_count)) > + return -EIO; > > nfs4_state_protect(server->nfs_client, > NFS_SP4_MACH_CRED_STATEID, > &task_setup.rpc_client, &msg);
On Tue, 02 Nov 2021, Trond Myklebust wrote: > Hi Scott, > > Thanks. This mostly looks good, but I do have 1 comment below. > > On Mon, 2021-11-01 at 16:06 -0400, Scott Mayhew wrote: > > During umount, the session slot tables are freed. If there are > > outstanding FREE_STATEID tasks, a use-after-free and slab corruption > > can > > occur when rpc_exit_task calls rpc_call_done -> nfs41_sequence_done - > > > > > nfs4_sequence_process/nfs41_sequence_free_slot. > > > > Prevent that from happening by taking a reference on the nfs_client > > in > > nfs41_free_stateid and putting it in nfs41_free_stateid_release. > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > fs/nfs/nfs4proc.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index e1214bb6b7ee..76e6786b797e 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -10145,18 +10145,24 @@ static void > > nfs41_free_stateid_prepare(struct rpc_task *task, void *calldata) > > static void nfs41_free_stateid_done(struct rpc_task *task, void > > *calldata) > > { > > struct nfs_free_stateid_data *data = calldata; > > + struct nfs_client *clp = data->server->nfs_client; > > > > nfs41_sequence_done(task, &data->res.seq_res); > > > > switch (task->tk_status) { > > case -NFS4ERR_DELAY: > > if (nfs4_async_handle_error(task, data->server, NULL, > > NULL) == -EAGAIN) > > - rpc_restart_call_prepare(task); > > + if (refcount_read(&clp->cl_count) > 1) > > + rpc_restart_call_prepare(task); > > Do we really need to make the rpc restart call conditional here? Most > servers prefer that you finish freeing state before calling > DESTROY_CLIENTID. Good point. No, it's not necessary. Do you want me to send a v2, or can you apply the patch without this hunk? -Scott > > > } > > } > > > > static void nfs41_free_stateid_release(void *calldata) > > { > > + struct nfs_free_stateid_data *data = calldata; > > + struct nfs_client *clp = data->server->nfs_client; > > + > > + nfs_put_client(clp); > > kfree(calldata); > > } > > > > @@ -10193,6 +10199,10 @@ static int nfs41_free_stateid(struct > > nfs_server *server, > > }; > > struct nfs_free_stateid_data *data; > > struct rpc_task *task; > > + struct nfs_client *clp = server->nfs_client; > > + > > + if (!refcount_inc_not_zero(&clp->cl_count)) > > + return -EIO; > > > > nfs4_state_protect(server->nfs_client, > > NFS_SP4_MACH_CRED_STATEID, > > &task_setup.rpc_client, &msg); > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
On Tue, 2021-11-02 at 16:11 -0400, Scott Mayhew wrote: > On Tue, 02 Nov 2021, Trond Myklebust wrote: > > > Hi Scott, > > > > Thanks. This mostly looks good, but I do have 1 comment below. > > > > On Mon, 2021-11-01 at 16:06 -0400, Scott Mayhew wrote: > > > During umount, the session slot tables are freed. If there are > > > outstanding FREE_STATEID tasks, a use-after-free and slab > > > corruption > > > can > > > occur when rpc_exit_task calls rpc_call_done -> > > > nfs41_sequence_done - > > > > > > > nfs4_sequence_process/nfs41_sequence_free_slot. > > > > > > Prevent that from happening by taking a reference on the > > > nfs_client > > > in > > > nfs41_free_stateid and putting it in nfs41_free_stateid_release. > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > --- > > > fs/nfs/nfs4proc.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index e1214bb6b7ee..76e6786b797e 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -10145,18 +10145,24 @@ static void > > > nfs41_free_stateid_prepare(struct rpc_task *task, void *calldata) > > > static void nfs41_free_stateid_done(struct rpc_task *task, void > > > *calldata) > > > { > > > struct nfs_free_stateid_data *data = calldata; > > > + struct nfs_client *clp = data->server->nfs_client; > > > > > > nfs41_sequence_done(task, &data->res.seq_res); > > > > > > switch (task->tk_status) { > > > case -NFS4ERR_DELAY: > > > if (nfs4_async_handle_error(task, data->server, > > > NULL, > > > NULL) == -EAGAIN) > > > - rpc_restart_call_prepare(task); > > > + if (refcount_read(&clp->cl_count) > 1) > > > + rpc_restart_call_prepare(task); > > > > Do we really need to make the rpc restart call conditional here? > > Most > > servers prefer that you finish freeing state before calling > > DESTROY_CLIENTID. > > Good point. No, it's not necessary. Do you want me to send a v2, or > can you apply the patch without this hunk? > Can you please send a v2? Thanks! > -Scott > > > > > } > > > } > > > > > > static void nfs41_free_stateid_release(void *calldata) > > > { > > > + struct nfs_free_stateid_data *data = calldata; > > > + struct nfs_client *clp = data->server->nfs_client; > > > + > > > + nfs_put_client(clp); > > > kfree(calldata); > > > } > > > > > > @@ -10193,6 +10199,10 @@ static int nfs41_free_stateid(struct > > > nfs_server *server, > > > }; > > > struct nfs_free_stateid_data *data; > > > struct rpc_task *task; > > > + struct nfs_client *clp = server->nfs_client; > > > + > > > + if (!refcount_inc_not_zero(&clp->cl_count)) > > > + return -EIO; > > > > > > nfs4_state_protect(server->nfs_client, > > > NFS_SP4_MACH_CRED_STATEID, > > > &task_setup.rpc_client, &msg); > > > > -- > > Trond Myklebust > > Linux NFS client maintainer, Hammerspace > > trond.myklebust@hammerspace.com > > > > >
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index e1214bb6b7ee..76e6786b797e 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -10145,18 +10145,24 @@ static void nfs41_free_stateid_prepare(struct rpc_task *task, void *calldata) static void nfs41_free_stateid_done(struct rpc_task *task, void *calldata) { struct nfs_free_stateid_data *data = calldata; + struct nfs_client *clp = data->server->nfs_client; nfs41_sequence_done(task, &data->res.seq_res); switch (task->tk_status) { case -NFS4ERR_DELAY: if (nfs4_async_handle_error(task, data->server, NULL, NULL) == -EAGAIN) - rpc_restart_call_prepare(task); + if (refcount_read(&clp->cl_count) > 1) + rpc_restart_call_prepare(task); } } static void nfs41_free_stateid_release(void *calldata) { + struct nfs_free_stateid_data *data = calldata; + struct nfs_client *clp = data->server->nfs_client; + + nfs_put_client(clp); kfree(calldata); } @@ -10193,6 +10199,10 @@ static int nfs41_free_stateid(struct nfs_server *server, }; struct nfs_free_stateid_data *data; struct rpc_task *task; + struct nfs_client *clp = server->nfs_client; + + if (!refcount_inc_not_zero(&clp->cl_count)) + return -EIO; nfs4_state_protect(server->nfs_client, NFS_SP4_MACH_CRED_STATEID, &task_setup.rpc_client, &msg);
During umount, the session slot tables are freed. If there are outstanding FREE_STATEID tasks, a use-after-free and slab corruption can occur when rpc_exit_task calls rpc_call_done -> nfs41_sequence_done -> nfs4_sequence_process/nfs41_sequence_free_slot. Prevent that from happening by taking a reference on the nfs_client in nfs41_free_stateid and putting it in nfs41_free_stateid_release. Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- fs/nfs/nfs4proc.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)