Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

On Wed, 13 Oct 2010 13:42:24 -0500
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Wed, 13 Oct 2010 10:28:25 -0500
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> > On Tue, 12 Oct 2010 14:45:03 -0500
> >> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> >
> >> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >>
> >> >> Start calculation auth response within a session.  Move/Add pertinet
> >> >> data structures like session key, server challenge and ntlmv2_hash in
> >> >> a session structure.  We should do the calculations within a session
> >> >> before copying session key and response over to server data
> >> >> structures because a session setup can fail.
> >> >>
> >> >> Only after a very first smb session succeeds, it copies/makes its
> >> >> session key, session key of smb connection.  This key stays with
> >> >> the smb connection throughout its life.
> >> >>
> >> >>
> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >
> >> >
> >> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
> >> >>               if (ses->capabilities & CAP_UNICODE) {
> >> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
> >> >>       if (rc) {
> >> >>               cERROR(1, "Send error in SessSetup = %d", rc);
> >> >>       } else {
> >> >> +             mutex_lock(&ses->server->srv_mutex);
> >> >> +             if (!server->session_estab) {
> >> >> +                     memcpy(&server->session_key.data,
> >> >> +                             &ses->auth_key.data, ses->auth_key.len);
> >> >> +                     server->session_key.len = ses->auth_key.len;
> >> >> +                     ses->server->session_estab = true;
> >> >                        ^^^^
> >> >                        nit: Aren't "server" and "ses->server" the
> >> >                        same? Why access them differently here?
> >> >
> >> >> +             }
> >> >> +             mutex_unlock(&server->srv_mutex);
> >> >> +
> >> >                ^^^^^^
> >> >                What are the rules for locking around the session_estab
> >> >                flag? Here you're holding the srv_mutex when checking
> >> >                and altering it, but in cifs_reconnect you're altering
> >> >                it without holding the mutex. Is that a potential race
> >> >                condition?
> >>
> >> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
> >> time session_estab is set as false, all the sessions have been
> >> invalidated and socket connection has been taken down
> >> and restablished. Isn't that as good as new connection?
> >> Setting session_estab under srv_mutex is mainly to prevent
> >> newly established sessions from racing.
> >>
> >
> > You may be right, but I don't immediately see what prevents the
> > following scenario:
> >
> > The sessions have been invalidated in cifs_reconnect and we're making
> > our way down to the point where that flag is set to false.
> >
> > Another task then calls smb_init to issue a new call, notices that the
> > session is invalid and reconnects it.
> >
> > This succeeds, but cifs_reconnect isn't done yet. session_estab is
> > still true. The session setup code takes the mutex and then checks and
> > finds that the flag is still true so it doesn't copy the key.
> >
> > Eventually cifs_reconnect flips the flag to false (maybe even while
> > we're holding the mutex) but now we're borked. We have a valid session
> > on the socket but the key is likely to be wrong.
> >
> > This is admittedly unlikely, and maybe impossible, but we should
> > understand what makes it impossible before we wave our hands and say
> > there's no race.
> >
> > --
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >
> 
> Or we could move them earlier
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 848e458..d7e0651 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -173,6 +173,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                 sock_release(server->ssocket);
>                 server->ssocket = NULL;
>         }
> +       server->sequence_number = 0;
> +       server->session_estab = false;
> 
>         spin_lock(&GlobalMid_Lock);
>         list_for_each(tmp, &server->pending_mid_q) {
> @@ -205,8 +207,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                         spin_lock(&GlobalMid_Lock);
>                         if (server->tcpStatus != CifsExiting)
>                                 server->tcpStatus = CifsGood;
> -                       server->sequence_number = 0;
> -                       server->session_estab = false;
>                         spin_unlock(&GlobalMid_Lock);
>         /*              atomic_set(&server->inFlight,0);*/
>                         wake_up(&server->response_q);
> 
> 
> There is no smb connection at this point and (certainly) sessions are
> not established at this point.
> 

I think that's the right idea.

You want to make sure that everything is properly torn down, flags
reset, etc. before the socket is reconnected.

There may be other pieces of cifs_reconnect that violate that rule as
well. If there are, I'd suggest fixing those in a separate patch that
goes before this one.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>