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.
So I gave up on this approach and did a small fix to make it work, but
maybe I missed something elsewhere...
--
Thanks,
Winston
