On Mon, Jun 26, 2023 at 9:25 AM Winston Wen <wentao@xxxxxxxxxxxxx> wrote:
>
> We switch session state to SES_EXITING without cifs_tcp_ses_lock now,
> it may lead to potential use-after-free issue.
>
> Consider the following execution processes:
>
> Thread 1:
> __cifs_put_smb_ses()
> spin_lock(&cifs_tcp_ses_lock)
> if (--ses->ses_count > 0)
> spin_unlock(&cifs_tcp_ses_lock)
> return
> spin_unlock(&cifs_tcp_ses_lock)
> ---> **GAP**
> spin_lock(&ses->ses_lock)
> if (ses->ses_status == SES_GOOD)
> ses->ses_status = SES_EXITING
> spin_unlock(&ses->ses_lock)
>
> Thread 2:
> cifs_find_smb_ses()
> spin_lock(&cifs_tcp_ses_lock)
> list_for_each_entry(ses, ...)
> spin_lock(&ses->ses_lock)
> if (ses->ses_status == SES_EXITING)
> spin_unlock(&ses->ses_lock)
> continue
> ...
> spin_unlock(&ses->ses_lock)
> if (ret)
> cifs_smb_ses_inc_refcount(ret)
> spin_unlock(&cifs_tcp_ses_lock)
>
> If thread 1 is preempted in the gap and thread 2 start executing, thread 2
> will get the session, and soon thread 1 will switch the session state to
> SES_EXITING and start releasing it, even though thread 1 had increased the
> session's refcount and still uses it.
>
> So switch session state under cifs_tcp_ses_lock to eliminate this gap.
>
> Signed-off-by: Winston Wen <wentao@xxxxxxxxxxxxx>
> ---
> fs/smb/client/connect.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 9d16626e7a66..165ecb222c19 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1963,15 +1963,16 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> spin_unlock(&cifs_tcp_ses_lock);
> return;
> }
> + spin_lock(&ses->ses_lock);
> + if (ses->ses_status == SES_GOOD)
> + ses->ses_status = SES_EXITING;
> + spin_unlock(&ses->ses_lock);
> spin_unlock(&cifs_tcp_ses_lock);
>
> /* ses_count can never go negative */
> WARN_ON(ses->ses_count < 0);
>
> spin_lock(&ses->ses_lock);
> - if (ses->ses_status == SES_GOOD)
> - ses->ses_status = SES_EXITING;
> -
> if (ses->ses_status == SES_EXITING && server->ops->logoff) {
> spin_unlock(&ses->ses_lock);
> cifs_free_ipc(ses);
> --
> 2.40.1
>
Good catch.
Looks good to me.
@Steve French Please CC stable for this one.
--
Regards,
Shyam
