Added Cc: stable and Shyam's RB and merged into cifs-2.6.git for-next
On Mon, Jun 26, 2023 at 12:15 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> 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
--
Thanks,
Steve
