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

[PATCH v4] smb: client: fix potential UAF 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() 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

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

[Steve French]

Added to for-next . thx

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
>
>


-- 
Thanks,

Steve

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

[Steve French]

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.

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
>
>


-- 
Thanks,

Steve

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

[Henrique Carvalho]

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

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

[Henrique Carvalho]

On 11/5/25 10:30 AM, Henrique Carvalho wrote:
> 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?
> 

I think it's worth adding that kref_put_lock uses refcount_dec_and_lock,
which is defined as

extern __must_check bool refcount_dec_and_lock(refcount_t *r, spinlock_t
*lock) __cond_acquires(lock);

__cond_acquires() was introduced in commit 4a557a5d1a614 ("sparse:
introduce conditional lock acquire function attribute").

So in a sense, if I understand it correctly, the "acquires" is already
there to make the sparse __releases annotation work. This should be
enough to satisfy the comment the AI raised.

>>
>> 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

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

[Henrique Carvalho]

Hi Steve,

On 11/5/25 1:20 PM, Steve French wrote:
> I can run it through again if you semd mean updated patch
> 
> Thanks,
> 
> Steve

I don't think there's anything to update here.

Adding a separate __acquires at call sites would be misleading. Not only
the call site is correctly annotated -- refcount_dec_and_lock() is
annotated with __cond_acquires(lock). Sparse reports no warnings, so
there's no missing annotation to fix.

Likewise, documenting that callers must hold the lock before calling
kref_put_lock() would be incorrect -- they must not.

No one should call kref_put_lock() directly. Likewise, no one should
call smb2_close_cached_fid() directly. lockdep_assert_held() is there
simply to ensure the invariant at runtime and as a hint to other
developers.

If you think it would help, I can add a short comment in
smb2_close_cached_fid() noting that the lock is held via
kref_put_lock().

Let me know if you prefer that comment and I'll send a v5.

> 
> On Wed, Nov 5, 2025, 10:18 AM Henrique Carvalho <henrique.carvalho@suse.com>
> wrote:
> 
>>
>>
>> On 11/5/25 10:30 AM, Henrique Carvalho wrote:
>>> 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?
>>>
>>
>> I think it's worth adding that kref_put_lock uses refcount_dec_and_lock,
>> which is defined as
>>
>> extern __must_check bool refcount_dec_and_lock(refcount_t *r, spinlock_t
>> *lock) __cond_acquires(lock);
>>
>> __cond_acquires() was introduced in commit 4a557a5d1a614 ("sparse:
>> introduce conditional lock acquire function attribute").
>>
>> So in a sense, if I understand it correctly, the "acquires" is already
>> there to make the sparse __releases annotation work. This should be
>> enough to satisfy the comment the AI raised.
>>
>>>>
>>>> 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
>>
> 

-- 
Henrique
SUSE Labs

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

[Henrique Carvalho]

Resending to include linux-cifs@vger.kernel.org.

Since there were concerns about a potential deadlock in this path (from Enzo
and copilot), I reworked the patch a bit.

In theory the existing code is fine as long as the refcount invariant holds,
but we've already been proven wrong in this area a few times. So I'd rather
make the deadlock impossible and WARN if the invariant is violated
instead of
relying on it being perfect.

@Paulo, what do you think about this?

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 018055fd2cdb..50074c9f125f 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -16,6 +16,7 @@ static struct cached_fid *init_cached_dir(const char
*path);
 static void free_cached_dir(struct cached_fid *cfid);
 static void smb2_close_cached_fid(struct kref *ref);
 static void cfids_laundromat_worker(struct work_struct *work);
+static void close_cached_dir_locked(struct cached_fid *cfid);

 struct cached_dir_dentry {
 	struct list_head entry;
@@ -388,7 +389,7 @@ int open_cached_dir(unsigned int xid, struct
cifs_tcon *tcon,
 			 * lease. Release one here, and the second below.
 			 */
 			cfid->has_lease = false;
-			close_cached_dir(cfid);
+			close_cached_dir_locked(cfid);
 		}
 		spin_unlock(&cfids->cfid_list_lock);

@@ -480,18 +481,52 @@ 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;
-		close_cached_dir(cfid);
+		close_cached_dir_locked(cfid);
 	}
 	spin_unlock(&cfid->cfids->cfid_list_lock);
 	close_cached_dir(cfid);
 }

-
+/**
+ * close_cached_dir - drop a reference of a cached dir
+ *
+ * The release function will be called with cfid_list_lock held to
remove the
+ * cached dirs from the list before any other thread can take another @cfid
+ * ref. Must not be called with cfid_list_lock held; use
+ * close_cached_dir_locked() called instead.
+ *
+ * @cfid: cached dir
+ */
 void close_cached_dir(struct cached_fid *cfid)
 {
+	lockdep_assert_not_held(&cfid->cfids->cfid_list_lock);
 	kref_put_lock(&cfid->refcount, smb2_close_cached_fid,
&cfid->cfids->cfid_list_lock);
 }

+/**
+ * close_cached_dir_locked - put a reference of a cached dir with
+ * cfid_list_lock held
+ *
+ * Calling close_cached_dir() with cfid_list_lock held has the
potential effect
+ * of causing a deadlock if the invariant of refcount >= 2 is false.
+ *
+ * This function is used in paths that hold cfid_list_lock and expect
at least
+ * two references. If that invariant is violated, WARNs and returns without
+ * dropping a reference; the final put must still go through
+ * close_cached_dir().
+ *
+ * @cfid: cached dir struct
+ */
+static void close_cached_dir_locked(struct cached_fid *cfid)
+{
+	lockdep_assert_held(&cfid->cfids->cfid_list_lock);
+
+	if (WARN_ON(kref_read(&cfid->refcount) < 2))
+		return;
+
+	kref_put(&cfid->refcount, smb2_close_cached_fid);
+}
+
 /*
  * Called from cifs_kill_sb when we unmount a share
  */
-- 
Henrique
SUSE Labs

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

[Paulo Alcantara]

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

> Resending to include linux-cifs@vger.kernel.org.
>
> Since there were concerns about a potential deadlock in this path (from Enzo
> and copilot), I reworked the patch a bit.
>
> In theory the existing code is fine as long as the refcount invariant holds,
> but we've already been proven wrong in this area a few times. So I'd rather
> make the deadlock impossible and WARN if the invariant is violated
> instead of
> relying on it being perfect.
>
> @Paulo, what do you think about this?

Sounds good.