[PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
Steve French
smfrench at gmail.com
Thu Mar 26 01:37:10 UTC 2020
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
* 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 at microsoft.com> wrote:
>
> >Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when
> >calculating SMB2/SMB3 signature on receiving packets
> >
> >ср, 25 мар. 2020 г. в 11:15, <longli at linuxonhyperv.com>:
> >>
> >> From: Long Li <longli at microsoft.com>
> >>
> >> 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 at microsoft.com>
> >> ---
> >> 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
More information about the samba-technical
mailing list