On Tue, Jun 27, 2023 at 1:04 PM Winston Wen <wentao@xxxxxxxxxxxxx> wrote:
>
> On Tue, 27 Jun 2023 12:24:04 +0530
> Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> > 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.
>
> Agreed.
>
> >
> > >
> > > 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.
>
> I think this is a better way to fix the problem, the session really
> should not stay in the list and be found after it has been marked
> EXITING.
>
> >
> > The other two patches are not strictly necessary with this change, but
> > don't hurt.
> >
>
> Yes. Feel free to drop them if they are not necessary. And if that's
> the case, perhaps we should do some cleaning work on other paths to
> ensure consistency.
I don't really have a strong opinion about this. Even if they stay, I'm okay.
But curious to know what you mean by the cleaning work on other paths here.
Do you still think there's more cleanup needed around this?
>
> Thanks for your review and comments!
>
> >
> > --
> > Regards,
> > Shyam
>
>
> --
> Thanks,
> Winston
--
Regards,
Shyam
