Re: [PATCH v2] NFSD: fix use-after-free on source server when doing inter-server copy

[dai.ngo@oracle.com]

On 9/23/22 9:28 AM, Jeff Layton wrote:
> On Mon, 2022-08-01 at 18:52 -0700, Dai Ngo wrote:
>> Use-after-free occurred when the laundromat tried to free expired
>> cpntf_state entry on the s2s_cp_stateids list after inter-server
>> copy completed. The sc_cp_list that the expired copy state was
>> inserted on was already freed.
>>
>> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
>> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
>> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
>> from the s2s_cp_stateids list before freeing the lock state's stid.
>>
>> However, sometimes the CLOSE was sent before the FREE_STATEID request.
>> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
>> frees all lock states on its st_locks list without cleaning up the copy
>> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
>> server returns BAD_STATEID since the lock state was freed. This causes
>> the use-after-free error to occur when the laundromat tries to free
>> the expired cpntf_state.
>>
>> This patch adds a call to nfs4_free_cpntf_statelist in
>> nfsd4_close_open_stateid to clean up the copy state before calling
>> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9409a0dc1b76..b99c545f93e4 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1049,6 +1049,9 @@ static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
>>   
>>   static void nfs4_free_deleg(struct nfs4_stid *stid)
>>   {
>> +	struct nfs4_ol_stateid *stp = openlockstateid(stid);
>> +
>> +	WARN_ON(!list_empty(&stp->st_stid.sc_cp_list));
> You're casting a delegation stateid to an openlockstateid, and then
> getting stid back from that. How about this instead, and drop the
> openlockstateid weirdness?
>
>        WARN_ON(!list_empty(&stid->sc_cp_list));

Not sure what I was thinking, I'll fix it.

Thanks,
-Dai

>
>>   	kmem_cache_free(deleg_slab, stid);
>>   	atomic_long_dec(&num_delegations);
>>   }
>> @@ -1463,6 +1466,7 @@ static void nfs4_free_ol_stateid(struct nfs4_stid *stid)
>>   	release_all_access(stp);
>>   	if (stp->st_stateowner)
>>   		nfs4_put_stateowner(stp->st_stateowner);
>> +	WARN_ON(!list_empty(&stp->st_stid.sc_cp_list));
>>   	kmem_cache_free(stateid_slab, stid);
>>   }
>>   
>> @@ -6608,6 +6612,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>   	struct nfs4_client *clp = s->st_stid.sc_client;
>>   	bool unhashed;
>>   	LIST_HEAD(reaplist);
>> +	struct nfs4_ol_stateid *stp;
>>   
>>   	spin_lock(&clp->cl_lock);
>>   	unhashed = unhash_open_stateid(s, &reaplist);
>> @@ -6616,6 +6621,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>   		if (unhashed)
>>   			put_ol_stateid_locked(s, &reaplist);
>>   		spin_unlock(&clp->cl_lock);
>> +		list_for_each_entry(stp, &reaplist, st_locks)
>> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>>   		free_ol_stateid_reaplist(&reaplist);
>>   	} else {
>>   		spin_unlock(&clp->cl_lock);