From: Pavel Shilovsky <piastryyy@gmail.com>
To: Long Li <longli@microsoft.com>
Cc: Steve French <sfrench@samba.org>,
linux-cifs <linux-cifs@vger.kernel.org>,
samba-technical <samba-technical@lists.samba.org>,
Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cifs: Remove locking in smb2_verify_signature() when calculating SMB2/SMB3 signature on receiving packets
Date: Wed, 25 Mar 2020 14:39:30 -0700 [thread overview]
Message-ID: <CAKywueQWcjWP7FNG0y-gGOSV5S1kSKJfWpspwXVzY9S2oHh+zQ@mail.gmail.com> (raw)
In-Reply-To: <1585159997-115196-1-git-send-email-longli@linuxonhyperv.com>
ср, 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
next prev parent reply other threads:[~2020-03-25 21:39 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKywueQWcjWP7FNG0y-gGOSV5S1kSKJfWpspwXVzY9S2oHh+zQ@mail.gmail.com \
--to=piastryyy@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=longli@microsoft.com \
--cc=samba-technical@lists.samba.org \
--cc=sfrench@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).