On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@xxxxxxxxx> wrote:
>
> 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
@Winston Wen I think the following change should be sufficient to fix
this issue:
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 9d16626e7a66..78874eb2537d 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1963,10 +1963,11 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
spin_unlock(&cifs_tcp_ses_lock);
return;
}
- spin_unlock(&cifs_tcp_ses_lock);
/* ses_count can never go negative */
WARN_ON(ses->ses_count < 0);
+ list_del_init(&ses->smb_ses_list);
+ spin_unlock(&cifs_tcp_ses_lock);
spin_lock(&ses->ses_lock);
if (ses->ses_status == SES_GOOD)
@@ -1986,9 +1987,6 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
cifs_free_ipc(ses);
}
- spin_lock(&cifs_tcp_ses_lock);
- list_del_init(&ses->smb_ses_list);
- spin_unlock(&cifs_tcp_ses_lock);
chan_count = ses->chan_count;
The bug was that the ses was kept in the smb ses list, even after the
ref count had reached 0.
With the above change, that should be fixed, and no one should be able
to get to the ses from that point.
Please let me know if you see a problem with this.
--
Regards,
Shyam
