On Mon, Aug 12, 2013 at 12:39 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Thu, 8 Aug 2013 09:54:00 -0500
> shirishpargaonkar@xxxxxxxxx wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>> Use per smb session session key for signing for smb2 onwards.
>> For each signature genereation/validation, look for corrosponding
>> session (id) to fetch session key.
>> Skip checking signature of the session setup response.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> ---
>> fs/cifs/cifsglob.h | 5 +++--
>> fs/cifs/cifsproto.h | 5 ++++-
>> fs/cifs/connect.c | 14 +++++++++---
>> fs/cifs/smb2transport.c | 59 +++++++++++++++++++++++++++++++++++++++++--------
>> 4 files changed, 68 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 2909380..4e24355 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -368,7 +368,8 @@ struct smb_version_operations {
>> int (*perconnkey)(struct TCP_Server_Info *server,
>> struct cifs_ses *ses);
>> /* The next two functions will need to be changed to per smb session */
>> - void (*generate_signingkey)(struct TCP_Server_Info *server);
>> + int (*generate_signingkey)(struct TCP_Server_Info *server,
>> + struct cifs_ses *ses);
>> int (*calc_signature)(struct smb_rqst *rqst,
>> struct TCP_Server_Info *server);
>> };
>> @@ -546,7 +547,6 @@ struct TCP_Server_Info {
>> int timeAdj; /* Adjust for difference in server time zone in sec */
>> __u64 CurrentMid; /* multiplex id - rotating counter */
>> char cryptkey[CIFS_CRYPTO_KEY_SIZE]; /* used by ntlm, ntlmv2 etc */
>> - char smb3signingkey[SMB3_SIGN_KEY_SIZE]; /* for signing smb3 packets */
>> /* 16th byte of RFC1001 workstation name is always null */
>> char workstation_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
>> __u32 sequence_number; /* for signing, protected by srv_mutex */
>> @@ -729,6 +729,7 @@ struct cifs_ses {
>> bool need_reconnect:1; /* connection reset, uid now invalid */
>> #ifdef CONFIG_CIFS_SMB2
>> __u16 session_flags;
>> + char smb3signingkey[SMB3_SIGN_KEY_SIZE]; /* for signing smb3 packets */
>> #endif /* CONFIG_CIFS_SMB2 */
>> };
>>
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index b6c1d03..261c114 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -206,6 +206,8 @@ extern void cifs_dfs_release_automount_timer(void);
>> void cifs_proc_init(void);
>> void cifs_proc_clean(void);
>>
>> +extern void cifs_put_smb_ses(struct cifs_ses *);
>> +
>> extern void cifs_move_llist(struct list_head *source, struct list_head *dest);
>> extern void cifs_free_llist(struct list_head *llist);
>> extern void cifs_del_lock_waiters(struct cifsLockInfo *lock);
>> @@ -435,7 +437,8 @@ extern int setup_ntlm_response(struct cifs_ses *, const struct nls_table *);
>> extern int setup_ntlmv2_rsp(struct cifs_ses *, const struct nls_table *);
>> extern void cifs_crypto_shash_release(struct TCP_Server_Info *);
>> extern int calc_seckey(struct cifs_ses *);
>> -extern void generate_smb3signingkey(struct TCP_Server_Info *);
>> +extern int generate_smb3signingkey(struct TCP_Server_Info *,
>> + struct cifs_ses *);
>>
>> extern int perconnkey(struct TCP_Server_Info *, struct cifs_ses *);
>> extern void free_authkey(struct cifs_ses *);
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 98deb98..0109046 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2251,7 +2251,7 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
>> return NULL;
>> }
>>
>> -static void
>> +void
>> cifs_put_smb_ses(struct cifs_ses *ses)
>> {
>> unsigned int xid;
>> @@ -3847,8 +3847,16 @@ cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
>> server->sequence_number = 0x2;
>> server->session_estab = true;
>> }
>> - if (server->ops->generate_signingkey)
>> - server->ops->generate_signingkey(server);
>> + if (server->ops->generate_signingkey) {
>> + rc = server->ops->generate_signingkey(server, ses);
>> + if (rc) {
>> + cifs_dbg(VFS,
>> + "%s: Failure to generate signing key\n",
>> + __func__);
>> + mutex_unlock(&server->srv_mutex);
>> + goto setup_sess_ret;
>> + }
>> + }
>> mutex_unlock(&server->srv_mutex);
>>
>
> Again, I think it might make sense to move all of the signature key
> handling into the session setup routines themselves. There doesn't seem
> to be a strong need to abstract this out at the smb_ops layer.
>
>> cifs_dbg(FYI, "CIFS Session Established successfully\n");
>> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
>> index 301b191..570e5b5 100644
>> --- a/fs/cifs/smb2transport.c
>> +++ b/fs/cifs/smb2transport.c
>> @@ -109,6 +109,24 @@ smb3_crypto_shash_allocate(struct TCP_Server_Info *server)
>> return 0;
>> }
>>
>> +static struct cifs_ses *
>> +smb2_find_smb_ses(struct smb2_hdr *smb2hdr, struct TCP_Server_Info *server)
>> +{
>> + struct cifs_ses *ses;
>> +
>> + spin_lock(&cifs_tcp_ses_lock);
>> + list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
>> + if (ses->Suid != smb2hdr->SessionId)
>> + continue;
>> + ++ses->ses_count;
>> + spin_unlock(&cifs_tcp_ses_lock);
>> + return ses;
>> + }
>> + spin_unlock(&cifs_tcp_ses_lock);
>> +
>> + return NULL;
>> +}
>> +
>>
>
> I guess this will work but it's not going to be great for scalability
> since you now have to serialize on this spinlock.
>
> The ideal scheme would be to keep a pointer to the smb session in the
> smb_rqst that gets set when the SessionId field is set. Then you
> wouldn't need to do this search on every outgoing frame. That's not
> trivial to do though, due to the fact that much of this code is based
> around legacy wrappers that don't know anything about struct smb_rqst.
> So, in light of that, I guess this is ok for now, even if it is ugly.
>
> One thing I'm not clear on though -- why are you bumping the refcount
> on the session here? It looks to me like you'll always have a pinned
> session reference any time that you call this, right? Or is that not
> the case in the receive path?
I think this is not needed. By virtue of taking a reference/count,
every session will be around till it is being taken down.
Once I fix the case where we remove a session from the list
after session logoff with smb2/3, this will be the case always.
Will not add the ++ and put for sesssion during the next spin
of this patchset.
Regards,
Shirish
>
>> int
>> smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>> @@ -119,23 +137,34 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>> struct kvec *iov = rqst->rq_iov;
>> int n_vec = rqst->rq_nvec;
>> struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base;
>> + struct cifs_ses *ses;
>> +
>> + ses = smb2_find_smb_ses(smb2_pdu, server);
>> + if (!ses) {
>> + cifs_dbg(VFS, "%s: Could not find session\n", __func__);
>> + return 0;
>> + }
>>
>> memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
>> memset(smb2_pdu->Signature, 0x0, SMB2_SIGNATURE_SIZE);
>>
>> rc = smb2_crypto_shash_allocate(server);
>> if (rc) {
>> + cifs_put_smb_ses(ses);
>> cifs_dbg(VFS, "%s: shah256 alloc failed\n", __func__);
>> return rc;
>> }
>>
>> rc = crypto_shash_setkey(server->secmech.hmacsha256,
>> - server->session_key.response, SMB2_NTLMV2_SESSKEY_SIZE);
>> + ses->auth_key.response, SMB2_NTLMV2_SESSKEY_SIZE);
>> if (rc) {
>> + cifs_put_smb_ses(ses);
>> cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
>> return rc;
>> }
>>
>> + cifs_put_smb_ses(ses);
>> +
>> rc = crypto_shash_init(&server->secmech.sdeschmacsha256->shash);
>> if (rc) {
>> cifs_dbg(VFS, "%s: Could not init sha256", __func__);
>> @@ -193,8 +222,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>> return rc;
>> }
>>
>> -void
>> -generate_smb3signingkey(struct TCP_Server_Info *server)
>> +int
>> +generate_smb3signingkey(struct TCP_Server_Info *server, struct cifs_ses *ses)
>> {
>> unsigned char zero = 0x0;
>> __u8 i[4] = {0, 0, 0, 1};
>> @@ -204,7 +233,7 @@ generate_smb3signingkey(struct TCP_Server_Info *server)
>> unsigned char *hashptr = prfhash;
>>
>> memset(prfhash, 0x0, SMB2_HMACSHA256_SIZE);
>> - memset(server->smb3signingkey, 0x0, SMB3_SIGNKEY_SIZE);
>> + memset(ses->smb3signingkey, 0x0, SMB3_SIGNKEY_SIZE);
>>
>> rc = smb3_crypto_shash_allocate(server);
>> if (rc) {
>> @@ -213,7 +242,7 @@ generate_smb3signingkey(struct TCP_Server_Info *server)
>> }
>>
>> rc = crypto_shash_setkey(server->secmech.hmacsha256,
>> - server->session_key.response, SMB2_NTLMV2_SESSKEY_SIZE);
>> + ses->auth_key.response, SMB2_NTLMV2_SESSKEY_SIZE);
>> if (rc) {
>> cifs_dbg(VFS, "%s: Could not set with session key\n", __func__);
>> goto smb3signkey_ret;
>> @@ -267,27 +296,38 @@ generate_smb3signingkey(struct TCP_Server_Info *server)
>> goto smb3signkey_ret;
>> }
>>
>> - memcpy(server->smb3signingkey, hashptr, SMB3_SIGNKEY_SIZE);
>> + memcpy(ses->smb3signingkey, hashptr, SMB3_SIGNKEY_SIZE);
>>
>> smb3signkey_ret:
>> - return;
>> + return rc;
>> }
>>
>> int
>> smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>> {
>> - int i, rc;
>> + int i;
>> + int rc = 0;
>> unsigned char smb3_signature[SMB2_CMACAES_SIZE];
>> unsigned char *sigptr = smb3_signature;
>> struct kvec *iov = rqst->rq_iov;
>> int n_vec = rqst->rq_nvec;
>> struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base;
>> + struct cifs_ses *ses;
>> +
>> + ses = smb2_find_smb_ses(smb2_pdu, server);
>> + if (!ses) {
>> + cifs_dbg(VFS, "%s: Could not find session\n", __func__);
>> + return 0;
>> + }
>>
>> memset(smb3_signature, 0x0, SMB2_CMACAES_SIZE);
>> memset(smb2_pdu->Signature, 0x0, SMB2_SIGNATURE_SIZE);
>>
>> rc = crypto_shash_setkey(server->secmech.cmacaes,
>> - server->smb3signingkey, SMB2_CMACAES_SIZE);
>> + ses->smb3signingkey, SMB2_CMACAES_SIZE);
>> +
>> + cifs_put_smb_ses(ses);
>> +
>> if (rc) {
>> cifs_dbg(VFS, "%s: Could not set key for cmac aes\n", __func__);
>> return rc;
>> @@ -384,6 +424,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>> struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)rqst->rq_iov[0].iov_base;
>>
>> if ((smb2_pdu->Command == SMB2_NEGOTIATE) ||
>> + (smb2_pdu->Command == SMB2_SESSION_SETUP) ||
>> (smb2_pdu->Command == SMB2_OPLOCK_BREAK) ||
>> (!server->session_estab))
>> return 0;
>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
