linux-kernel - Re: [PATCH] nfsd: free nfsd_file by gc after adding it to lru list

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a453c201-7dd4-49e7-a90a-5dc4c9359f2b@oracle.com>
Date: Wed, 15 Jan 2025 10:03:50 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>, Li Lingfeng <lilingfeng3@...wei.com>,
        neilb@...e.de, okorniev@...hat.com, Dai.Ngo@...cle.com, tom@...pey.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, lilingfeng@...weicloud.com
Subject: Re: [PATCH] nfsd: free nfsd_file by gc after adding it to lru list

On 1/14/25 2:39 PM, Jeff Layton wrote:
> On Tue, 2025-01-14 at 14:27 -0500, Jeff Layton wrote:
>> On Mon, 2025-01-13 at 10:59 +0800, Li Lingfeng wrote:
>>> In nfsd_file_put, after inserting the nfsd_file into the nfsd_file_lru
>>> list, gc may be triggered in another thread and immediately release this
>>> nfsd_file, which will lead to a UAF when accessing this nfsd_file again.
>>>
>>> All the places where unhash is done will also perform lru_remove, so there
>>> is no need to do lru_remove separately here. After inserting the nfsd_file
>>> into the nfsd_file_lru list, it can be released by relying on gc.
>>>
>>> Fixes: 4a0e73e635e3 ("NFSD: Leave open files out of the filecache LRU")
>>> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
>>> ---
>>>   fs/nfsd/filecache.c | 12 ++----------
>>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index a1cdba42c4fa..37b65cb1579a 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -372,18 +372,10 @@ nfsd_file_put(struct nfsd_file *nf)
>>>   		/* Try to add it to the LRU.  If that fails, decrement. */
>>>   		if (nfsd_file_lru_add(nf)) {
>>>   			/* If it's still hashed, we're done */
>>> -			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
>>> +			if (list_lru_count(&nfsd_file_lru))
>>>   				nfsd_file_schedule_laundrette();
>>> -				return;
>>> -			}
>>>   
>>> -			/*
>>> -			 * We're racing with unhashing, so try to remove it from
>>> -			 * the LRU. If removal fails, then someone else already
>>> -			 * has our reference.
>>> -			 */
>>> -			if (!nfsd_file_lru_remove(nf))
>>> -				return;
>>> +			return;
>>>   		}
>>>   	}
>>>   	if (refcount_dec_and_test(&nf->nf_ref))
>>
>> I think this looks OK. Filecache bugs are particularly nasty though, so
>> let's run this through a nice long testing cycle.
>>
>> Reviewed-by: Jeff Layton <jlayton@...nel.org>
> 
> Actually, I take it back. This is problematic in another way.
> 
> In this case, we're racing with another task that is unhashing the
> object, but we've put it on the LRU ourselves. What guarantee do we
> have that the unhashing and removal from the LRU didn't occur before
> this task called nfsd_file_lru_add()? That's why we attempt to remove
> it here -- we can't rely on the task that unhashed it to do so at that
> point.
> 
> What might be best is to take and hold the rcu_read_lock() before doing
> the nfsd_file_lru_add, and just release it after we do these racy
> checks. That should make it safe to access the object.
> 
> Thoughts?

Holding the RCU read lock will keep the dereferences safe since
nfsd_file objects are freed only after an RCU grace period. But will the
logic in nfsd_file_put() work properly on totally dead nfsd_file
objects? I don't see a specific failure mode there, but I'm not very
imaginative.

Overall, I think RCU would help.


-- 
Chuck Lever