>Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when >calculating SMB2/SMB3 signature on receiving packets > >Wonder if we should add a separate documentation patch to cifsglob.h to >extend the descriptions of what the various locks and semaphores are >supposed to protect to include what you noticed? See below Yes, I will send this patch. > > * Locking notes. All updates to global variables and lists should be > * protected by spinlocks or semaphores. > * > * Spinlocks > * --------- > * GlobalMid_Lock protects: > * list operations on pending_mid_q and oplockQ > * updates to XID counters, multiplex id and SMB sequence numbers > * list operations on global DnotifyReqList > * tcp_ses_lock protects: > * list operations on tcp and SMB session lists > * tcon->open_file_lock protects the list of open files hanging off the tcon > * inode->open_file_lock protects the openFileList hanging off the inode > * cfile->file_info_lock protects counters and fields in cifs file struct > * f_owner.lock protects certain per file struct operations > * mapping->page_lock protects certain per page operations > * > * Note that the cifs_tcon.open_file_lock should be taken before > * not after the cifsInodeInfo.open_file_lock > * > * Semaphores > * ---------- > * sesSem operations on smb session > * tconSem operations on tree connection > * fh_sem file handle reconnection operations > >On Wed, Mar 25, 2020 at 6:30 PM Long Li <longli@xxxxxxxxxxxxx> wrote: >> >> >Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() >> >when calculating SMB2/SMB3 signature on receiving packets >> > >> >ср, 25 мар. 2020 г. в 11:15, <longli@xxxxxxxxxxxxxxxxx>: >> >> >> >> From: Long Li <longli@xxxxxxxxxxxxx> >> >> >> >> On the sending and receiving paths, CIFS uses the same cypto data >> >> structures to calculate SMB2/SMB3 packet signatures. A lock on the >> >> receiving path is necessary to control shared access to crypto data >> >> structures. This lock degrades performance because it races with >> >> the >> >sending path. >> >> >> >> Define separate crypto data structures for sending and receiving >> >> paths and remove this lock. >> >> >> >> Signed-off-by: Long Li <longli@xxxxxxxxxxxxx> >> >> --- >> >> fs/cifs/cifsencrypt.c | 29 +++++---- >> >> fs/cifs/cifsglob.h | 21 +++++-- >> >> fs/cifs/smb2proto.h | 4 +- >> >> fs/cifs/smb2transport.c | 130 >> >> +++++++++++++++++++++++++--------------- >> >> 4 files changed, 119 insertions(+), 65 deletions(-) >> >> >> >> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c index >> >> 97b7497c13ef..222e8d13302c 100644 >> >> --- a/fs/cifs/cifsencrypt.c >> >> +++ b/fs/cifs/cifsencrypt.c >> >> @@ -804,16 +804,27 @@ calc_seckey(struct cifs_ses *ses) void >> >> cifs_crypto_secmech_release(struct TCP_Server_Info *server) { >> >> - if (server->secmech.cmacaes) { >> >> - crypto_free_shash(server->secmech.cmacaes); >> >> - server->secmech.cmacaes = NULL; >> >> - } >> >> + int i; >> >> + >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + if (server->secmech.cmacaes[i]) { >> >> + crypto_free_shash(server->secmech.cmacaes[i]); >> >> + server->secmech.cmacaes[i] = NULL; >> >> + } >> >> >> >> - if (server->secmech.hmacsha256) { >> >> - crypto_free_shash(server->secmech.hmacsha256); >> >> - server->secmech.hmacsha256 = NULL; >> >> + if (server->secmech.hmacsha256[i]) { >> >> + crypto_free_shash(server->secmech.hmacsha256[i]); >> >> + server->secmech.hmacsha256[i] = NULL; >> >> + } >> >> + >> >> + kfree(server->secmech.sdesccmacaes[i]); >> >> + server->secmech.sdesccmacaes[i] = NULL; >> >> + >> >> + kfree(server->secmech.sdeschmacsha256[i]); >> >> + server->secmech.sdeschmacsha256[i] = NULL; >> >> } >> >> >> >> + >> >> if (server->secmech.md5) { >> >> crypto_free_shash(server->secmech.md5); >> >> server->secmech.md5 = NULL; @@ -839,10 +850,6 @@ >> >> cifs_crypto_secmech_release(struct TCP_Server_Info *server) >> >> server->secmech.ccmaesdecrypt = NULL; >> >> } >> >> >> >> - kfree(server->secmech.sdesccmacaes); >> >> - server->secmech.sdesccmacaes = NULL; >> >> - kfree(server->secmech.sdeschmacsha256); >> >> - server->secmech.sdeschmacsha256 = NULL; >> >> kfree(server->secmech.sdeschmacmd5); >> >> server->secmech.sdeschmacmd5 = NULL; >> >> kfree(server->secmech.sdescmd5); diff --git >> >> a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index >> >> 0d956360e984..e31a902ebadc 100644 >> >> --- a/fs/cifs/cifsglob.h >> >> +++ b/fs/cifs/cifsglob.h >> >> @@ -137,17 +137,27 @@ struct sdesc { >> >> char ctx[]; >> >> }; >> >> >> >> +enum { >> >> + CIFS_SECMECH_DIR_IN = 0, >> >> + CIFS_SECMECH_DIR_OUT, >> >> + CIFS_SECMECH_DIR_MAX >> >> +}; >> >> + >> >> /* crypto hashing related structure/fields, not specific to a sec >> >> mech */ struct cifs_secmech { >> >> struct crypto_shash *hmacmd5; /* hmac-md5 hash function */ >> >> struct crypto_shash *md5; /* md5 hash function */ >> >> - struct crypto_shash *hmacsha256; /* hmac-sha256 hash function */ >> >> - struct crypto_shash *cmacaes; /* block-cipher based MAC function >*/ >> >> + /* hmac-sha256 hash functions */ >> >> + struct crypto_shash *hmacsha256[CIFS_SECMECH_DIR_MAX]; >> >> + /* block-cipher based MAC function */ >> >> + struct crypto_shash *cmacaes[CIFS_SECMECH_DIR_MAX]; >> >> struct crypto_shash *sha512; /* sha512 hash function */ >> >> struct sdesc *sdeschmacmd5; /* ctxt to generate ntlmv2 hash, CR1 >*/ >> >> struct sdesc *sdescmd5; /* ctxt to generate cifs/smb signature */ >> >> - struct sdesc *sdeschmacsha256; /* ctxt to generate smb2 signature >*/ >> >> - struct sdesc *sdesccmacaes; /* ctxt to generate smb3 signature */ >> >> + /* ctxt to generate smb2 signature */ >> >> + struct sdesc *sdeschmacsha256[CIFS_SECMECH_DIR_MAX]; >> >> + /* ctxt to generate smb3 signature */ >> >> + struct sdesc *sdesccmacaes[CIFS_SECMECH_DIR_MAX]; >> >> struct sdesc *sdescsha512; /* ctxt to generate smb3.11 signing key */ >> >> struct crypto_aead *ccmaesencrypt; /* smb3 encryption aead */ >> >> struct crypto_aead *ccmaesdecrypt; /* smb3 decryption aead >> >> */ @@ -426,7 +436,8 @@ struct smb_version_operations { >> >> /* generate new lease key */ >> >> void (*new_lease_key)(struct cifs_fid *); >> >> int (*generate_signingkey)(struct cifs_ses *); >> >> - int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *); >> >> + int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *, >> >> + int direction); >> >> int (*set_integrity)(const unsigned int, struct cifs_tcon *tcon, >> >> struct cifsFileInfo *src_file); >> >> int (*enum_snapshots)(const unsigned int xid, struct >> >> cifs_tcon *tcon, diff --git a/fs/cifs/smb2proto.h >> >> b/fs/cifs/smb2proto.h index >> >> 4d1ff7b66fdc..f5edd6ea3639 100644 >> >> --- a/fs/cifs/smb2proto.h >> >> +++ b/fs/cifs/smb2proto.h >> >> @@ -55,9 +55,9 @@ extern struct cifs_ses *smb2_find_smb_ses(struct >> >> TCP_Server_Info *server, extern struct cifs_tcon >> >*smb2_find_smb_tcon(struct TCP_Server_Info *server, >> >> __u64 ses_id, __u32 >> >> tid); extern int smb2_calc_signature(struct smb_rqst *rqst, >> >> - struct TCP_Server_Info *server); >> >> + struct TCP_Server_Info *server, int >> >> + direction); >> >> extern int smb3_calc_signature(struct smb_rqst *rqst, >> >> - struct TCP_Server_Info *server); >> >> + struct TCP_Server_Info *server, int >> >> + direction); >> >> extern void smb2_echo_request(struct work_struct *work); extern >> >> __le32 smb2_get_lease_state(struct cifsInodeInfo *cinode); extern >> >> bool smb2_is_valid_oplock_break(char *buffer, diff --git >> >> a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c index >> >> 08b703b7a15e..c8ba40d8fedf 100644 >> >> --- a/fs/cifs/smb2transport.c >> >> +++ b/fs/cifs/smb2transport.c >> >> @@ -43,30 +43,51 @@ >> >> static int >> >> smb2_crypto_shash_allocate(struct TCP_Server_Info *server) { >> >> - return cifs_alloc_hash("hmac(sha256)", >> >> - &server->secmech.hmacsha256, >> >> - &server->secmech.sdeschmacsha256); >> >> + int i, rc; >> >> + >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + rc = cifs_alloc_hash("hmac(sha256)", >> >> + &server->secmech.hmacsha256[i], >> >> + &server->secmech.sdeschmacsha256[i]); >> >> + if (rc) >> >> + goto fail; >> >> + } >> >> + return 0; >> >> + >> >> +fail: >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) >> >> + cifs_free_hash( >> >> + &server->secmech.hmacsha256[i], >> >> + &server->secmech.sdeschmacsha256[i]); >> >> + return rc; >> >> } >> >> >> >> static int >> >> smb3_crypto_shash_allocate(struct TCP_Server_Info *server) { >> >> struct cifs_secmech *p = &server->secmech; >> >> - int rc; >> >> + int i, rc; >> >> >> >> - rc = cifs_alloc_hash("hmac(sha256)", >> >> - &p->hmacsha256, >> >> - &p->sdeschmacsha256); >> >> - if (rc) >> >> - goto err; >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + rc = cifs_alloc_hash("hmac(sha256)", >> >> + &p->hmacsha256[i], >> >> + &p->sdeschmacsha256[i]); >> >> + if (rc) >> >> + goto fail; >> >> >> >> - rc = cifs_alloc_hash("cmac(aes)", &p->cmacaes, &p->sdesccmacaes); >> >> - if (rc) >> >> - goto err; >> >> + rc = cifs_alloc_hash("cmac(aes)", >> >> + &p->cmacaes[i], &p->sdesccmacaes[i]); >> >> >> >> + if (rc) >> >> + goto fail; >> >> + } >> >> return 0; >> >> -err: >> >> - cifs_free_hash(&p->hmacsha256, &p->sdeschmacsha256); >> >> + >> >> +fail: >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + cifs_free_hash(&p->hmacsha256[i], &p->sdeschmacsha256[i]); >> >> + cifs_free_hash(&p->cmacaes[i], &p->sdesccmacaes[i]); >> >> + } >> >> return rc; >> >> } >> >> >> >> @@ -74,27 +95,32 @@ int >> >> smb311_crypto_shash_allocate(struct TCP_Server_Info *server) { >> >> struct cifs_secmech *p = &server->secmech; >> >> - int rc = 0; >> >> + int i, rc = 0; >> >> >> >> - rc = cifs_alloc_hash("hmac(sha256)", >> >> - &p->hmacsha256, >> >> - &p->sdeschmacsha256); >> >> - if (rc) >> >> - return rc; >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + rc = cifs_alloc_hash("hmac(sha256)", >> >> + &p->hmacsha256[i], >> >> + &p->sdeschmacsha256[i]); >> >> + if (rc) >> >> + goto fail; >> >> >> >> - rc = cifs_alloc_hash("cmac(aes)", &p->cmacaes, &p->sdesccmacaes); >> >> - if (rc) >> >> - goto err; >> >> + rc = cifs_alloc_hash("cmac(aes)", >> >> + &p->cmacaes[i], &p->sdesccmacaes[i]); >> >> + if (rc) >> >> + goto fail; >> >> + } >> >> >> >> rc = cifs_alloc_hash("sha512", &p->sha512, &p->sdescsha512); >> >> if (rc) >> >> - goto err; >> >> + goto fail; >> >> >> >> return 0; >> >> >> >> -err: >> >> - cifs_free_hash(&p->cmacaes, &p->sdesccmacaes); >> >> - cifs_free_hash(&p->hmacsha256, &p->sdeschmacsha256); >> >> +fail: >> >> + for (i = CIFS_SECMECH_DIR_IN; i < CIFS_SECMECH_DIR_MAX; i++) { >> >> + cifs_free_hash(&p->hmacsha256[i], &p->sdeschmacsha256[i]); >> >> + cifs_free_hash(&p->cmacaes[i], &p->sdesccmacaes[i]); >> >> + } >> >> return rc; >> >> } >> >> >> >> @@ -219,7 +245,8 @@ smb2_find_smb_tcon(struct TCP_Server_Info >> >*server, __u64 ses_id, __u32 tid) >> >> } >> >> >> >> int >> >> -smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >> >*server) >> >> +smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >> >*server, >> >> + int direction) >> >> { >> >> int rc; >> >> unsigned char smb2_signature[SMB2_HMACSHA256_SIZE]; >> >> @@ -229,6 +256,7 @@ smb2_calc_signature(struct smb_rqst *rqst, >struct >> >TCP_Server_Info *server) >> >> struct cifs_ses *ses; >> >> struct shash_desc *shash; >> >> struct smb_rqst drqst; >> >> + struct crypto_shash *hmacsha256; >> >> >> >> ses = smb2_find_smb_ses(server, shdr->SessionId); >> >> if (!ses) { >> >> @@ -245,14 +273,16 @@ smb2_calc_signature(struct smb_rqst *rqst, >struct >> >TCP_Server_Info *server) >> >> return rc; >> >> } >> >> >> >> - rc = crypto_shash_setkey(server->secmech.hmacsha256, >> >> + hmacsha256 = server->secmech.hmacsha256[direction]; >> >> + >> >> + rc = crypto_shash_setkey(hmacsha256, >> >> ses->auth_key.response, >SMB2_NTLMV2_SESSKEY_SIZE); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with response\n", >> >__func__); >> >> return rc; >> >> } >> >> >> >> - shash = &server->secmech.sdeschmacsha256->shash; >> >> + shash = &server->secmech.sdeschmacsha256[direction]->shash; >> >> rc = crypto_shash_init(shash); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not init sha256", __func__); >> >> @@ -296,6 +326,8 @@ static int generate_key(struct cifs_ses *ses, struct >> >kvec label, >> >> unsigned char prfhash[SMB2_HMACSHA256_SIZE]; >> >> unsigned char *hashptr = prfhash; >> >> struct TCP_Server_Info *server = ses->server; >> >> + struct crypto_shash *hmacsha256; >> >> + struct sdesc *sdeschmacsha256; >> >> >> >> memset(prfhash, 0x0, SMB2_HMACSHA256_SIZE); >> >> memset(key, 0x0, key_size); >> >> @@ -306,55 +338,58 @@ static int generate_key(struct cifs_ses *ses, >struct >> >kvec label, >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_setkey(server->secmech.hmacsha256, >> >> + hmacsha256 = server- >> >>secmech.hmacsha256[CIFS_SECMECH_DIR_OUT]; >> >> + sdeschmacsha256 = server- >> >>secmech.sdeschmacsha256[CIFS_SECMECH_DIR_OUT]; >> >> + >> >> + rc = crypto_shash_setkey(hmacsha256, >> >> ses->auth_key.response, SMB2_NTLMV2_SESSKEY_SIZE); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not set with session key\n", >> >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_init(&server->secmech.sdeschmacsha256- >>shash); >> >> + rc = crypto_shash_init(&sdeschmacsha256->shash); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not init sign hmac\n", __func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_update(&server->secmech.sdeschmacsha256- >> >>shash, >> >> + rc = crypto_shash_update(&sdeschmacsha256->shash, >> >> i, 4); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with n\n", >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_update(&server->secmech.sdeschmacsha256- >> >>shash, >> >> + rc = crypto_shash_update(&sdeschmacsha256->shash, >> >> label.iov_base, label.iov_len); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with label\n", >> >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_update(&server->secmech.sdeschmacsha256- >> >>shash, >> >> + rc = crypto_shash_update(&sdeschmacsha256->shash, >> >> &zero, 1); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with zero\n", >> >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_update(&server->secmech.sdeschmacsha256- >> >>shash, >> >> + rc = crypto_shash_update(&sdeschmacsha256->shash, >> >> context.iov_base, context.iov_len); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with context\n", >> >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_update(&server->secmech.sdeschmacsha256- >> >>shash, >> >> + rc = crypto_shash_update(&sdeschmacsha256->shash, >> >> L, 4); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not update with L\n", >__func__); >> >> goto smb3signkey_ret; >> >> } >> >> >> >> - rc = crypto_shash_final(&server->secmech.sdeschmacsha256- >>shash, >> >> + rc = crypto_shash_final(&sdeschmacsha256->shash, >> >> hashptr); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not generate sha256 hash\n", >> >__func__); >> >> @@ -504,16 +539,18 @@ generate_smb311signingkey(struct cifs_ses >*ses) >> >> } >> >> >> >> int >> >> -smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >> >*server) >> >> +smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info >> >*server, >> >> + int direction) >> >> { >> >> int rc; >> >> unsigned char smb3_signature[SMB2_CMACAES_SIZE]; >> >> unsigned char *sigptr = smb3_signature; >> >> struct kvec *iov = rqst->rq_iov; >> >> struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr >*)iov[0].iov_base; >> >> - struct shash_desc *shash = &server->secmech.sdesccmacaes- >>shash; >> >> + struct shash_desc *shash; >> >> struct smb_rqst drqst; >> >> u8 key[SMB3_SIGN_KEY_SIZE]; >> >> + struct crypto_shash *cmacaes; >> >> >> >> rc = smb2_get_sign_key(shdr->SessionId, server, key); >> >> if (rc) >> >> @@ -522,8 +559,10 @@ smb3_calc_signature(struct smb_rqst *rqst, >struct >> >TCP_Server_Info *server) >> >> memset(smb3_signature, 0x0, SMB2_CMACAES_SIZE); >> >> memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE); >> >> >> >> - rc = crypto_shash_setkey(server->secmech.cmacaes, >> >> - key, SMB2_CMACAES_SIZE); >> >> + cmacaes = server->secmech.cmacaes[direction]; >> >> + shash = &server->secmech.sdesccmacaes[direction]->shash; >> >> + >> >> + rc = crypto_shash_setkey(cmacaes, key, SMB2_CMACAES_SIZE); >> >> if (rc) { >> >> cifs_server_dbg(VFS, "%s: Could not set key for cmac aes\n", >> >__func__); >> >> return rc; >> >> @@ -593,8 +632,7 @@ smb2_sign_rqst(struct smb_rqst *rqst, struct >> >TCP_Server_Info *server) >> >> return 0; >> >> } >> >> >> >> - rc = server->ops->calc_signature(rqst, server); >> >> - >> >> + rc = server->ops->calc_signature(rqst, server, >> >CIFS_SECMECH_DIR_OUT); >> >> return rc; >> >> } >> >> >> >> @@ -631,9 +669,7 @@ smb2_verify_signature(struct smb_rqst *rqst, >struct >> >TCP_Server_Info *server) >> >> >> >> memset(shdr->Signature, 0, SMB2_SIGNATURE_SIZE); >> >> >> >> - mutex_lock(&server->srv_mutex); >> >> - rc = server->ops->calc_signature(rqst, server); >> >> - mutex_unlock(&server->srv_mutex); >> >> + rc = server->ops->calc_signature(rqst, server, >CIFS_SECMECH_DIR_IN); >> > >> >Multiple threads may be calling smb2_verify_signature from >> >smb2_check_receive in separate threads (see compound_send_recv). >What >> >will prevent races on crypto data structures once the mutex around >> >calc_signature is removed? >> >> It's my bad I will fix it. In this case we need another mutex for the receiving >paths. I was looking at smb2_writev_callback() and smb2_readv_callback(), >they are called from cifsd and also use this mutex. This mutex does the most >damage in those two callback functions as it may block the receiving thread. >> >> Thanks, >> Long >> >> > >> >-- >> >Best regards, >> >Pavel Shilovsky > > > >-- >Thanks, > >Steve