On Mon, Jun 26, 2023 at 2:09 PM Winston Wen <wentao@xxxxxxxxxxxxx> wrote:
>
> On Mon, 26 Jun 2023 12:24:35 +0530
> Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> > 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.
> >
>
> Hi Shyam,
>
> Thanks for the comments! And sorry for my late reply...
>
> It make sense to me that maybe we should remove the session from the
> list once its refcount is reduced to 0 to avoid any futher access. In
> fact, I did try to do this from the beginning. But I was not sure if we
> need to access the session from the list in the free process, such
> as the following:
>
> smb2_check_receive()
> smb2_verify_signature()
> server->ops->calc_signature()
> smb2_calc_signature()
> smb2_find_smb_ses()
> /* scan the list and find the session */
>
> Perhaps we need some refactoring here.
Yes. The above ses finding is expected to fail during a reconnect.
>
> So I gave up on this approach and did a small fix to make it work, but
> maybe I missed something elsewhere...
>
>
> --
> Thanks,
> Winston
Attaching the above change as a patch.
It replaces this particular patch in the series.
The other two patches are not strictly necessary with this change, but
don't hurt.
--
Regards,
Shyam
Attachment:
0001-cifs-remove-smb-session-from-list-rest-of-the-cleanu.patch
Description: Binary data
