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