Re: [PATCH v4] smb: client: fix potential UAF in smb2_close_cached_fid()

[Henrique Carvalho <henrique.carvalho@suse.com>]

Hi Steve,

On 11/4/25 7:42 PM, Steve French wrote:
> One minor comment that AI had on this was on the change to
> fs/smb/client/cached_dir.c (line 447). See below.
> 
>     -spin_lock(&cfid->cfids->cfid_list_lock);
>     +lockdep_assert_held(&cfid->cfids->cfid_list_lock);
> 
> 
> The lockdep_assert_held validates that the lock is held on entry, but
> the function signature __releases(&cfid->cfids->cfid_list_lock)
> indicates the lock is released inside. Consider adding a corresponding
> __acquires annotation to the call sites that call this function via
> kref_put_lock, or document why callers must hold the lock before
> calling kref_put_lock.

But we don't actually acquire the lock every time in close_cached_dir.
Only when (a) refcount is 1; and (b) refcount did not increase after we
acquired the lock. So where should __acquires annotation go?

One option would be adding documentation to smb2_close_cached_fid,
though since it's a release function that shouldn't be called elsewhere,
I'm not sure that adds much value. Do you have thoughts?

> 
> On Mon, Nov 3, 2025 at 5:00 PM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
>>
>> 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() is called with
>> cfid_list_lock held, closing that gap.
>>
>> Fixes: ebe98f1447bb ("cifs: enable caching of directories for which a lease is held")
>> Cc: stable@vger.kernel.org
>> Reported-by: Jay Shin <jaeshin@redhat.com>
>> Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
>> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>> ---
>> V3 -> V4: rebase, added Reviewed-by and Reported-by, add
>> lockdep_assert_held in smb2_close_cached_fid, change commit subject (was
>> "smb: client: fix race in smb2_close_cached_fid()") to clearly state the
>> bug class (UAF)
>> 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, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>> index b8ac7b7faf61..018055fd2cdb 100644
>> --- a/fs/smb/client/cached_dir.c
>> +++ b/fs/smb/client/cached_dir.c
>> @@ -388,11 +388,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);
>> @@ -438,12 +438,14 @@ 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);
>> +       lockdep_assert_held(&cfid->cfids->cfid_list_lock);
>> +
>>         if (cfid->on_list) {
>>                 list_del(&cfid->entry);
>>                 cfid->on_list = false;
>> @@ -478,7 +480,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);
>> @@ -487,7 +489,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);
>>  }
>>
>>  /*
>> @@ -596,7 +598,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);
>>  }
>>
>> @@ -762,7 +764,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.50.1
>>
>>
> 
> 

-- 
Henrique
SUSE Labs