[PATCH v3] smb: client: fix race in smb2_close_cached_fid()

[PATCH v3] smb: client: fix race in smb2_close_cached_fid()

[Henrique Carvalho]

find_or_create_cached_dir() could grab a new reference after kref_put()
had seen the refcount drop to zero but before cfid_list_lock is acquired
in smb2_close_cached_fid(), leading to use-after-free.

Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
held, closing that gap.

Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
V2 -> V3: rebase, remove unneeded comments, modify commit subject
V1 -> V2: kept the original function names and added __releases annotation
to silence sparse warning

 fs/smb/client/cached_dir.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 89d2dbbb742c..09daf9500baf 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -365,11 +365,11 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 			 * lease. Release one here, and the second below.
 			 */
 			cfid->has_lease = false;
-			kref_put(&cfid->refcount, smb2_close_cached_fid);
+			close_cached_dir(cfid);
 		}
 		spin_unlock(&cfids->cfid_list_lock);
 
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
+		close_cached_dir(cfid);
 	} else {
 		*ret_cfid = cfid;
 		atomic_inc(&tcon->num_remote_opens);
@@ -409,12 +409,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 
 static void
 smb2_close_cached_fid(struct kref *ref)
+__releases(&cfid->cfids->cfid_list_lock)
 {
 	struct cached_fid *cfid = container_of(ref, struct cached_fid,
 					       refcount);
 	int rc;
 
-	spin_lock(&cfid->cfids->cfid_list_lock);
 	if (cfid->on_list) {
 		list_del(&cfid->entry);
 		cfid->on_list = false;
@@ -449,7 +449,7 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
 	spin_lock(&cfid->cfids->cfid_list_lock);
 	if (cfid->has_lease) {
 		cfid->has_lease = false;
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
+		close_cached_dir(cfid);
 	}
 	spin_unlock(&cfid->cfids->cfid_list_lock);
 	close_cached_dir(cfid);
@@ -458,7 +458,7 @@ void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon,
 
 void close_cached_dir(struct cached_fid *cfid)
 {
-	kref_put(&cfid->refcount, smb2_close_cached_fid);
+	kref_put_lock(&cfid->refcount, smb2_close_cached_fid, &cfid->cfids->cfid_list_lock);
 }
 
 /*
@@ -559,7 +559,7 @@ cached_dir_offload_close(struct work_struct *work)
 
 	WARN_ON(cfid->on_list);
 
-	kref_put(&cfid->refcount, smb2_close_cached_fid);
+	close_cached_dir(cfid);
 	cifs_put_tcon(tcon, netfs_trace_tcon_ref_put_cached_close);
 }
 
@@ -683,7 +683,7 @@ static void cfids_invalidation_worker(struct work_struct *work)
 	list_for_each_entry_safe(cfid, q, &entry, entry) {
 		list_del(&cfid->entry);
 		/* Drop the ref-count acquired in invalidate_all_cached_dirs */
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
+		close_cached_dir(cfid);
 	}
 }
 
@@ -736,7 +736,7 @@ static void cfids_laundromat_worker(struct work_struct *work)
 			 * Drop the ref-count from above, either the lease-ref (if there
 			 * was one) or the extra one acquired.
 			 */
-			kref_put(&cfid->refcount, smb2_close_cached_fid);
+			close_cached_dir(cfid);
 	}
 	queue_delayed_work(cfid_put_wq, &cfids->laundromat_work,
 			   dir_cache_timeout * HZ);
-- 
2.47.0

Re: [PATCH v3] smb: client: fix race in smb2_close_cached_fid()

[Paulo Alcantara]

Henrique Carvalho <henrique.carvalho@suse.com> writes:

> find_or_create_cached_dir() could grab a new reference after kref_put()
> had seen the refcount drop to zero but before cfid_list_lock is acquired
> in smb2_close_cached_fid(), leading to use-after-free.
>
> Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
> held, closing that gap.

Does this mean that SMB2_close() will be called with @cfid_list_lock
held?  If so, that's wrong.

Re: [PATCH v3] smb: client: fix race in smb2_close_cached_fid()

[Henrique Carvalho]

On Tue, May 27, 2025 at 04:06:01PM -0300, Paulo Alcantara wrote:
> Henrique Carvalho <henrique.carvalho@suse.com> writes:
> 
> > find_or_create_cached_dir() could grab a new reference after kref_put()
> > had seen the refcount drop to zero but before cfid_list_lock is acquired
> > in smb2_close_cached_fid(), leading to use-after-free.
> >
> > Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
> > held, closing that gap.
> 
> Does this mean that SMB2_close() will be called with @cfid_list_lock
> held?  If so, that's wrong.

What I mean here is "Switch to kref_put_lock() so
smb2_close_cached_fid() *IS CALLED* with cfid_list_lock held". Poor
wording on my part.

The lock will be released after cfid is removed from list.

My argument is that cfid can still be found before locking
cfid_list_lock in smb2_close_cached_fid. Here is the current code:

    static void
    smb2_close_cached_fid(struct kref *ref)
    {
            struct cached_fid *cfid = container_of(ref, struct cached_fid,
                                                   refcount);
            int rc;
    
+           /* kref is 0, but cfid can still be found here */
    
            spin_lock(&cfid->cfids->cfid_list_lock);
            if (cfid->on_list) {
                    list_del(&cfid->entry);
                    cfid->on_list = false;
                    cfid->cfids->num_entries--;
            }
            spin_unlock(&cfid->cfids->cfid_list_lock);
    
            dput(cfid->dentry);
            cfid->dentry = NULL;
    
            if (cfid->is_open) {
                    rc = SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
                               cfid->fid.volatile_fid);
                    if (rc) /* should we retry on -EBUSY or -EAGAIN? */
                            cifs_dbg(VFS, "close cached dir rc %d\n", rc);
            }
     [...]

Re: [PATCH v3] smb: client: fix race in smb2_close_cached_fid()

[Paulo Alcantara]

Henrique Carvalho <henrique.carvalho@suse.com> writes:

> On Tue, May 27, 2025 at 04:06:01PM -0300, Paulo Alcantara wrote:
>> Henrique Carvalho <henrique.carvalho@suse.com> writes:
>> 
>> > find_or_create_cached_dir() could grab a new reference after kref_put()
>> > had seen the refcount drop to zero but before cfid_list_lock is acquired
>> > in smb2_close_cached_fid(), leading to use-after-free.
>> >
>> > Switch to kref_put_lock() so cfid_release() runs with cfid_list_lock
>> > held, closing that gap.
>> 
>> Does this mean that SMB2_close() will be called with @cfid_list_lock
>> held?  If so, that's wrong.
>
> What I mean here is "Switch to kref_put_lock() so
> smb2_close_cached_fid() *IS CALLED* with cfid_list_lock held". Poor
> wording on my part.
>
> The lock will be released after cfid is removed from list.

Awesome, thanks for making it clearer.