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

[Winston Wen <wentao@xxxxxxxxxxxxx>]

On Mon, 26 Jun 2023 10:57:32 +0530
Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:

> On Mon, Jun 26, 2023 at 9:24 AM Winston Wen <wentao@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx>
> 

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...