diff mbox series

[2/3] cifs: fix session state check in reconnect to avoid use-after-free issue

Message ID 20230626034257.2078391-3-wentao@uniontech.com (mailing list archive)
State New, archived
Headers show
Series cifs: fix session state checks to avoid use-after-free issues | expand

Commit Message

Winston Wen June 26, 2023, 3:42 a.m. UTC
Don't collect exiting session in smb2_reconnect_server(), because it
will be released soon.

Note that the exiting session will stay in server->smb_ses_list until
it complete the cifs_free_ipc() and logoff() and then delete itself
from the list.

Signed-off-by: Winston Wen <wentao@uniontech.com>
---
 fs/smb/client/smb2pdu.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Shyam Prasad N June 26, 2023, 5:27 a.m. UTC | #1
On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@uniontech.com> wrote:
>
> Don't collect exiting session in smb2_reconnect_server(), because it
> will be released soon.
>
> Note that the exiting session will stay in server->smb_ses_list until
> it complete the cifs_free_ipc() and logoff() and then delete itself
> from the list.
>
> Signed-off-by: Winston Wen <wentao@uniontech.com>
> ---
>  fs/smb/client/smb2pdu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index 17fe212ab895..e04766fe6f80 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct work_struct *work)
>
>         spin_lock(&cifs_tcp_ses_lock);
>         list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
> +               spin_lock(&ses->ses_lock);
> +               if (ses->ses_status == SES_EXITING) {
> +                       spin_unlock(&ses->ses_lock);
> +                       continue;
> +               }
> +               spin_unlock(&ses->ses_lock);
>
>                 tcon_selected = false;
>
> --
> 2.40.1
>

Hi Winston,

We already have this check in smb2_reconnect, which gets called from
smb2_reconnect_server.
But one additional check here will not hurt.

Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
Winston Wen June 26, 2023, 5:57 a.m. UTC | #2
On Mon, 26 Jun 2023 10:57:32 +0530
Shyam Prasad N <nspmangalore@gmail.com> wrote:

> On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@uniontech.com>
> wrote:
> >
> > Don't collect exiting session in smb2_reconnect_server(), because it
> > will be released soon.
> >
> > Note that the exiting session will stay in server->smb_ses_list
> > until it complete the cifs_free_ipc() and logoff() and then delete
> > itself from the list.
> >
> > Signed-off-by: Winston Wen <wentao@uniontech.com>
> > ---
> >  fs/smb/client/smb2pdu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > index 17fe212ab895..e04766fe6f80 100644
> > --- a/fs/smb/client/smb2pdu.c
> > +++ b/fs/smb/client/smb2pdu.c
> > @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct
> > work_struct *work)
> >
> >         spin_lock(&cifs_tcp_ses_lock);
> >         list_for_each_entry(ses, &pserver->smb_ses_list,
> > smb_ses_list) {
> > +               spin_lock(&ses->ses_lock);
> > +               if (ses->ses_status == SES_EXITING) {
> > +                       spin_unlock(&ses->ses_lock);
> > +                       continue;
> > +               }
> > +               spin_unlock(&ses->ses_lock);
> >
> >                 tcon_selected = false;
> >
> > --
> > 2.40.1
> >
> 
> Hi Winston,
> 
> We already have this check in smb2_reconnect, which gets called from
> smb2_reconnect_server.
> But one additional check here will not hurt.
> 
> Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> 

Hi Shyam,

Thanks for the review! And sorry for my mistake that when I replied a
minute ago I forgot to cc others...

I think the check in smb2_reconnect is not enough for this situation,
but maybe I missed something...

Consider the following process:

smb2_reconnect_server():
    spin_lock(&cifs_tcp_ses_lock)
    list_for_each_entry(ses, ...)
        ...
        if (ses->tcon_ipc && ses->tcon_ipc->need_reconnect)
            cifs_smb_ses_inc_refcount(ses)
    spin_unlock(&cifs_tcp_ses_lock)

    /* -> session may have been released before smb2_reconnect */
    list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist)
        smb2_reconnect()
        list_del_init(&tcon->rlist)
        if (tcon->ipc)
            cifs_put_smb_ses(tcon->ses)
        else
            cifs_put_tcon(tcon)

When we do smb2_reconnect(), the session may have been released, and all
the access to its field in smb2_reconnect(), such as ses->status or
ses->ses_lock, is illegal. And when we call the cifs_put_smb_ses() on it
again, it will crash...
Shyam Prasad N June 26, 2023, 6:27 a.m. UTC | #3
On Mon, Jun 26, 2023 at 11:28 AM Winston Wen <wentao@uniontech.com> wrote:
>
> On Mon, 26 Jun 2023 10:57:32 +0530
> Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> > On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@uniontech.com>
> > wrote:
> > >
> > > Don't collect exiting session in smb2_reconnect_server(), because it
> > > will be released soon.
> > >
> > > Note that the exiting session will stay in server->smb_ses_list
> > > until it complete the cifs_free_ipc() and logoff() and then delete
> > > itself from the list.
> > >
> > > Signed-off-by: Winston Wen <wentao@uniontech.com>
> > > ---
> > >  fs/smb/client/smb2pdu.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > > index 17fe212ab895..e04766fe6f80 100644
> > > --- a/fs/smb/client/smb2pdu.c
> > > +++ b/fs/smb/client/smb2pdu.c
> > > @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct
> > > work_struct *work)
> > >
> > >         spin_lock(&cifs_tcp_ses_lock);
> > >         list_for_each_entry(ses, &pserver->smb_ses_list,
> > > smb_ses_list) {
> > > +               spin_lock(&ses->ses_lock);
> > > +               if (ses->ses_status == SES_EXITING) {
> > > +                       spin_unlock(&ses->ses_lock);
> > > +                       continue;
> > > +               }
> > > +               spin_unlock(&ses->ses_lock);
> > >
> > >                 tcon_selected = false;
> > >
> > > --
> > > 2.40.1
> > >
> >
> > Hi Winston,
> >
> > We already have this check in smb2_reconnect, which gets called from
> > smb2_reconnect_server.
> > But one additional check here will not hurt.
> >
> > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> >
>
> Hi Shyam,
>
> Thanks for the review! And sorry for my mistake that when I replied a
> minute ago I forgot to cc others...
>
> I think the check in smb2_reconnect is not enough for this situation,
> but maybe I missed something...
>
> Consider the following process:
>
> smb2_reconnect_server():
>     spin_lock(&cifs_tcp_ses_lock)
>     list_for_each_entry(ses, ...)
>         ...
>         if (ses->tcon_ipc && ses->tcon_ipc->need_reconnect)
>             cifs_smb_ses_inc_refcount(ses)
>     spin_unlock(&cifs_tcp_ses_lock)
>
>     /* -> session may have been released before smb2_reconnect */
>     list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist)
>         smb2_reconnect()
>         list_del_init(&tcon->rlist)
>         if (tcon->ipc)
>             cifs_put_smb_ses(tcon->ses)
>         else
>             cifs_put_tcon(tcon)
>
> When we do smb2_reconnect(), the session may have been released, and all
> the access to its field in smb2_reconnect(), such as ses->status or
> ses->ses_lock, is illegal. And when we call the cifs_put_smb_ses() on it
> again, it will crash...

I see what you mean.

I think __cifs_put_smb_ses is at fault here.
Once the ses_count reaches 0, it should do all the following before it
drops cifs_tcp_ses_lock:
1. Mark as SES_EXITING
2. Remove the session from it's list.

That way, smb2_reconnect_server should not even be able to find the
session in the list.
Winston Wen June 26, 2023, 8:41 a.m. UTC | #4
On Mon, 26 Jun 2023 11:57:16 +0530
Shyam Prasad N <nspmangalore@gmail.com> wrote:

> On Mon, Jun 26, 2023 at 11:28 AM Winston Wen <wentao@uniontech.com>
> wrote:
> >
> > On Mon, 26 Jun 2023 10:57:32 +0530
> > Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > > On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@uniontech.com>
> > > wrote:
> > > >
> > > > Don't collect exiting session in smb2_reconnect_server(),
> > > > because it will be released soon.
> > > >
> > > > Note that the exiting session will stay in server->smb_ses_list
> > > > until it complete the cifs_free_ipc() and logoff() and then
> > > > delete itself from the list.
> > > >
> > > > Signed-off-by: Winston Wen <wentao@uniontech.com>
> > > > ---
> > > >  fs/smb/client/smb2pdu.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > > > index 17fe212ab895..e04766fe6f80 100644
> > > > --- a/fs/smb/client/smb2pdu.c
> > > > +++ b/fs/smb/client/smb2pdu.c
> > > > @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct
> > > > work_struct *work)
> > > >
> > > >         spin_lock(&cifs_tcp_ses_lock);
> > > >         list_for_each_entry(ses, &pserver->smb_ses_list,
> > > > smb_ses_list) {
> > > > +               spin_lock(&ses->ses_lock);
> > > > +               if (ses->ses_status == SES_EXITING) {
> > > > +                       spin_unlock(&ses->ses_lock);
> > > > +                       continue;
> > > > +               }
> > > > +               spin_unlock(&ses->ses_lock);
> > > >
> > > >                 tcon_selected = false;
> > > >
> > > > --
> > > > 2.40.1
> > > >
> > >
> > > Hi Winston,
> > >
> > > We already have this check in smb2_reconnect, which gets called
> > > from smb2_reconnect_server.
> > > But one additional check here will not hurt.
> > >
> > > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> > >
> >
> > Hi Shyam,
> >
> > Thanks for the review! And sorry for my mistake that when I replied
> > a minute ago I forgot to cc others...
> >
> > I think the check in smb2_reconnect is not enough for this
> > situation, but maybe I missed something...
> >
> > Consider the following process:
> >
> > smb2_reconnect_server():
> >     spin_lock(&cifs_tcp_ses_lock)
> >     list_for_each_entry(ses, ...)
> >         ...
> >         if (ses->tcon_ipc && ses->tcon_ipc->need_reconnect)
> >             cifs_smb_ses_inc_refcount(ses)
> >     spin_unlock(&cifs_tcp_ses_lock)
> >
> >     /* -> session may have been released before smb2_reconnect */
> >     list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist)
> >         smb2_reconnect()
> >         list_del_init(&tcon->rlist)
> >         if (tcon->ipc)
> >             cifs_put_smb_ses(tcon->ses)
> >         else
> >             cifs_put_tcon(tcon)
> >
> > When we do smb2_reconnect(), the session may have been released,
> > and all the access to its field in smb2_reconnect(), such as
> > ses->status or ses->ses_lock, is illegal. And when we call the
> > cifs_put_smb_ses() on it again, it will crash...
> 
> I see what you mean.
> 
> I think __cifs_put_smb_ses is at fault here.
> Once the ses_count reaches 0, it should do all the following before it
> drops cifs_tcp_ses_lock:
> 1. Mark as SES_EXITING
> 2. Remove the session from it's list.
> 
> That way, smb2_reconnect_server should not even be able to find the
> session in the list.
> 

Hi Shyam,

Please see the reply in patch 1.
Steve French June 27, 2023, 4:20 a.m. UTC | #5
added RB and merged into cifs-2.6.git for-next

On Mon, Jun 26, 2023 at 12:33 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@uniontech.com> wrote:
> >
> > Don't collect exiting session in smb2_reconnect_server(), because it
> > will be released soon.
> >
> > Note that the exiting session will stay in server->smb_ses_list until
> > it complete the cifs_free_ipc() and logoff() and then delete itself
> > from the list.
> >
> > Signed-off-by: Winston Wen <wentao@uniontech.com>
> > ---
> >  fs/smb/client/smb2pdu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > index 17fe212ab895..e04766fe6f80 100644
> > --- a/fs/smb/client/smb2pdu.c
> > +++ b/fs/smb/client/smb2pdu.c
> > @@ -3797,6 +3797,12 @@ void smb2_reconnect_server(struct work_struct *work)
> >
> >         spin_lock(&cifs_tcp_ses_lock);
> >         list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
> > +               spin_lock(&ses->ses_lock);
> > +               if (ses->ses_status == SES_EXITING) {
> > +                       spin_unlock(&ses->ses_lock);
> > +                       continue;
> > +               }
> > +               spin_unlock(&ses->ses_lock);
> >
> >                 tcon_selected = false;
> >
> > --
> > 2.40.1
> >
>
> Hi Winston,
>
> We already have this check in smb2_reconnect, which gets called from
> smb2_reconnect_server.
> But one additional check here will not hurt.
>
> Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
>
> --
> Regards,
> Shyam
diff mbox series

Patch

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 17fe212ab895..e04766fe6f80 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -3797,6 +3797,12 @@  void smb2_reconnect_server(struct work_struct *work)
 
 	spin_lock(&cifs_tcp_ses_lock);
 	list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
+		spin_lock(&ses->ses_lock);
+		if (ses->ses_status == SES_EXITING) {
+			spin_unlock(&ses->ses_lock);
+			continue;
+		}
+		spin_unlock(&ses->ses_lock);
 
 		tcon_selected = false;