| 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 |
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>
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...
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.
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.
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 --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;
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(+)