* [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
@ 2020-03-25 18:13 longli
2020-03-25 21:39 ` Pavel Shilovsky
2020-03-26 9:56 ` Aurélien Aptel
0 siblings, 2 replies; 8+ messages in thread
From: longli @ 2020-03-25 18:13 UTC (permalink / raw)
To: Steve French, linux-cifs, samba-technical, linux-kernel; +Cc: Long Li
From: Long Li <longli@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@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);
if (rc)
return rc;
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-25 18:13 [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets longli
@ 2020-03-25 21:39 ` Pavel Shilovsky
2020-03-25 23:29 ` Long Li
2020-03-26 9:56 ` Aurélien Aptel
1 sibling, 1 reply; 8+ messages in thread
From: Pavel Shilovsky @ 2020-03-25 21:39 UTC (permalink / raw)
To: Long Li; +Cc: Steve French, linux-cifs, samba-technical, Kernel Mailing List
ср, 25 мар. 2020 г. в 11:15, <longli@linuxonhyperv.com>:
>
> From: Long Li <longli@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@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?
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-25 21:39 ` Pavel Shilovsky
@ 2020-03-25 23:29 ` Long Li
2020-03-26 1:37 ` Steve French
0 siblings, 1 reply; 8+ messages in thread
From: Long Li @ 2020-03-25 23:29 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Steve French, linux-cifs, samba-technical, Kernel Mailing List
>Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when
>calculating SMB2/SMB3 signature on receiving packets
>
>ср, 25 мар. 2020 г. в 11:15, <longli@linuxonhyperv.com>:
>>
>> From: Long Li <longli@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@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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-25 23:29 ` Long Li
@ 2020-03-26 1:37 ` Steve French
2020-03-26 1:57 ` Long Li
0 siblings, 1 reply; 8+ messages in thread
From: Steve French @ 2020-03-26 1:37 UTC (permalink / raw)
To: Long Li
Cc: Pavel Shilovsky, Steve French, linux-cifs, samba-technical,
Kernel Mailing List
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@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@linuxonhyperv.com>:
> >>
> >> From: Long Li <longli@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@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
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-26 1:37 ` Steve French
@ 2020-03-26 1:57 ` Long Li
0 siblings, 0 replies; 8+ messages in thread
From: Long Li @ 2020-03-26 1:57 UTC (permalink / raw)
To: Steve French
Cc: Pavel Shilovsky, Steve French, linux-cifs, samba-technical,
Kernel Mailing List
>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@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@linuxonhyperv.com>:
>> >>
>> >> From: Long Li <longli@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@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-25 18:13 [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets longli
2020-03-25 21:39 ` Pavel Shilovsky
@ 2020-03-26 9:56 ` Aurélien Aptel
2020-03-27 5:41 ` Long Li
1 sibling, 1 reply; 8+ messages in thread
From: Aurélien Aptel @ 2020-03-26 9:56 UTC (permalink / raw)
To: longli, Steve French, linux-cifs, samba-technical, linux-kernel; +Cc: Long Li
longli@linuxonhyperv.com writes:
> 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.
Something I've often wondered: why do we keep crypto state in the server
structure instead of creating it as needed in the caller stack (thus
avoiding the need for locks). AFAIK there's no state that need to be
kept between signing/encrypting calls beside the access to keys. Is it that
expensive to create/release?
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-26 9:56 ` Aurélien Aptel
@ 2020-03-27 5:41 ` Long Li
2020-03-27 11:05 ` Aurélien Aptel
0 siblings, 1 reply; 8+ messages in thread
From: Long Li @ 2020-03-27 5:41 UTC (permalink / raw)
To: Aurélien Aptel, longli, Steve French, linux-cifs,
samba-technical, linux-kernel
>Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when
>calculating SMB2/SMB3 signature on receiving packets
>
>longli@linuxonhyperv.com writes:
>> 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.
>
>Something I've often wondered: why do we keep crypto state in the server
>structure instead of creating it as needed in the caller stack (thus avoiding the
>need for locks). AFAIK there's no state that need to be kept between
>signing/encrypting calls beside the access to keys. Is it that expensive to
>create/release?
My guess is that crypto_alloc_shash() is a heavy call?
>
>Cheers,
>--
>Aurélien Aptel / SUSE Labs Samba Team
>GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software
>Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
>GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
2020-03-27 5:41 ` Long Li
@ 2020-03-27 11:05 ` Aurélien Aptel
0 siblings, 0 replies; 8+ messages in thread
From: Aurélien Aptel @ 2020-03-27 11:05 UTC (permalink / raw)
To: Long Li, longli, Steve French, linux-cifs, samba-technical, linux-kernel
Long Li <longli@microsoft.com> writes:
>>need for locks). AFAIK there's no state that need to be kept between
>>signing/encrypting calls beside the access to keys. Is it that expensive to
>>create/release?
>
> My guess is that crypto_alloc_shash() is a heavy call?
AFAIK there's no IO, just some memory allocation. Could be faster than
locking. Something to look into, maybe...
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-27 11:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 18:13 [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets longli
2020-03-25 21:39 ` Pavel Shilovsky
2020-03-25 23:29 ` Long Li
2020-03-26 1:37 ` Steve French
2020-03-26 1:57 ` Long Li
2020-03-26 9:56 ` Aurélien Aptel
2020-03-27 5:41 ` Long Li
2020-03-27 11:05 ` Aurélien Aptel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).