diff mbox series

cifs: Allocate crypto structures on the fly for calculating signatures of incoming packets

Message ID 1585696903-96794-1-git-send-email-longli@linuxonhyperv.com
State New
Headers show
Series cifs: Allocate crypto structures on the fly for calculating signatures of incoming packets | expand

Commit Message

Long Li March 31, 2020, 11:21 p.m. UTC 
From: Long Li <longli@microsoft.com>

CIFS uses pre-allocated crypto structures to calculate signatures for both
incoming and outgoing packets. In this way it doesn't need to allocate crypto
structures for every packet, but it requires a lock to prevent concurrent
access to crypto structures.

Remove the lock by allocating crypto structures on the fly for
incoming packets. At the same time, we can still use pre-allocated crypto
structures for outgoing packets, as they are already protected by transport
lock srv_mutex.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsglob.h      |  3 +-
 fs/cifs/smb2proto.h     |  6 ++-
 fs/cifs/smb2transport.c | 87 +++++++++++++++++++++++++----------------
 3 files changed, 60 insertions(+), 36 deletions(-)

Comments

Pavel Shilovsky April 3, 2020, 5:45 p.m. UTC | #1 
вт, 31 мар. 2020 г. в 16:22, <longli@linuxonhyperv.com>:
>
> From: Long Li <longli@microsoft.com>
>
> CIFS uses pre-allocated crypto structures to calculate signatures for both
> incoming and outgoing packets. In this way it doesn't need to allocate crypto
> structures for every packet, but it requires a lock to prevent concurrent
> access to crypto structures.
>
> Remove the lock by allocating crypto structures on the fly for
> incoming packets. At the same time, we can still use pre-allocated crypto
> structures for outgoing packets, as they are already protected by transport
> lock srv_mutex.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  fs/cifs/cifsglob.h      |  3 +-
>  fs/cifs/smb2proto.h     |  6 ++-
>  fs/cifs/smb2transport.c | 87 +++++++++++++++++++++++++----------------
>  3 files changed, 60 insertions(+), 36 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0d956360e984..7448e7202e7a 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -426,7 +426,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 *,
> +                               bool allocate_crypto);
>         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..087d5f14320b 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -55,9 +55,11 @@ 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,
> +                               bool allocate_crypto);
>  extern int smb3_calc_signature(struct smb_rqst *rqst,
> -                               struct TCP_Server_Info *server);
> +                               struct TCP_Server_Info *server,
> +                               bool allocate_crypto);
>  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..c01e19a3b112 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -40,14 +40,6 @@
>  #include "smb2status.h"
>  #include "smb2glob.h"
>
> -static int
> -smb2_crypto_shash_allocate(struct TCP_Server_Info *server)
> -{
> -       return cifs_alloc_hash("hmac(sha256)",
> -                              &server->secmech.hmacsha256,
> -                              &server->secmech.sdeschmacsha256);
> -}
> -
>  static int
>  smb3_crypto_shash_allocate(struct TCP_Server_Info *server)
>  {
> @@ -219,7 +211,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,
> +                       bool allocate_crypto)
>  {
>         int rc;
>         unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
> @@ -228,6 +221,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>         struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)iov[0].iov_base;
>         struct cifs_ses *ses;
>         struct shash_desc *shash;
> +       struct crypto_shash *hash;
> +       struct sdesc *sdesc = NULL;
>         struct smb_rqst drqst;
>
>         ses = smb2_find_smb_ses(server, shdr->SessionId);
> @@ -239,24 +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
>         memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
>         memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
>
> -       rc = smb2_crypto_shash_allocate(server);
> -       if (rc) {
> -               cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
> -               return rc;
> +       if (allocate_crypto) {
> +               rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
> +               if (rc) {
> +                       cifs_server_dbg(VFS,
> +                                       "%s: sha256 alloc failed\n", __func__);
> +                       return rc;
> +               }
> +               shash = &sdesc->shash;
> +       } else {
> +               hash = server->secmech.hmacsha256;
> +               shash = &server->secmech.sdeschmacsha256->shash;
>         }

smb2_crypto_shash_allocate() unconditionally allocated
server->secmech.hmacsha256 and server->secmech.sdeschmacsha256->shash.
Now the code doesn't allocate those variables at all. Unlike SMB3
where structures are allocated in during key generation, for SMB2 we
do it on demand in smb2_calc_signature(). So, the code above should be
changed to call smb2_crypto_shash_allocate() in "else" block.

--
Best regards,
Pavel Shilovsky
Long Li April 3, 2020, 11:11 p.m. UTC | #2 
>Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating
>signatures of incoming packets
>
>вт, 31 мар. 2020 г. в 16:22, <longli@linuxonhyperv.com>:
>>
>> From: Long Li <longli@microsoft.com>
>>
>> CIFS uses pre-allocated crypto structures to calculate signatures for
>> both incoming and outgoing packets. In this way it doesn't need to
>> allocate crypto structures for every packet, but it requires a lock to
>> prevent concurrent access to crypto structures.
>>
>> Remove the lock by allocating crypto structures on the fly for
>> incoming packets. At the same time, we can still use pre-allocated
>> crypto structures for outgoing packets, as they are already protected
>> by transport lock srv_mutex.
>>
>> Signed-off-by: Long Li <longli@microsoft.com>
>> ---
>>  fs/cifs/cifsglob.h      |  3 +-
>>  fs/cifs/smb2proto.h     |  6 ++-
>>  fs/cifs/smb2transport.c | 87
>> +++++++++++++++++++++++++----------------
>>  3 files changed, 60 insertions(+), 36 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index
>> 0d956360e984..7448e7202e7a 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -426,7 +426,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 *,
>> +                               bool allocate_crypto);
>>         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..087d5f14320b 100644
>> --- a/fs/cifs/smb2proto.h
>> +++ b/fs/cifs/smb2proto.h
>> @@ -55,9 +55,11 @@ 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,
>> +                               bool allocate_crypto);
>>  extern int smb3_calc_signature(struct smb_rqst *rqst,
>> -                               struct TCP_Server_Info *server);
>> +                               struct TCP_Server_Info *server,
>> +                               bool allocate_crypto);
>>  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..c01e19a3b112 100644
>> --- a/fs/cifs/smb2transport.c
>> +++ b/fs/cifs/smb2transport.c
>> @@ -40,14 +40,6 @@
>>  #include "smb2status.h"
>>  #include "smb2glob.h"
>>
>> -static int
>> -smb2_crypto_shash_allocate(struct TCP_Server_Info *server) -{
>> -       return cifs_alloc_hash("hmac(sha256)",
>> -                              &server->secmech.hmacsha256,
>> -                              &server->secmech.sdeschmacsha256);
>> -}
>> -
>>  static int
>>  smb3_crypto_shash_allocate(struct TCP_Server_Info *server)  { @@
>> -219,7 +211,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,
>> +                       bool allocate_crypto)
>>  {
>>         int rc;
>>         unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
>> @@ -228,6 +221,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
>TCP_Server_Info *server)
>>         struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)iov[0].iov_base;
>>         struct cifs_ses *ses;
>>         struct shash_desc *shash;
>> +       struct crypto_shash *hash;
>> +       struct sdesc *sdesc = NULL;
>>         struct smb_rqst drqst;
>>
>>         ses = smb2_find_smb_ses(server, shdr->SessionId); @@ -239,24
>> +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
>TCP_Server_Info *server)
>>         memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
>>         memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
>>
>> -       rc = smb2_crypto_shash_allocate(server);
>> -       if (rc) {
>> -               cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
>> -               return rc;
>> +       if (allocate_crypto) {
>> +               rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
>> +               if (rc) {
>> +                       cifs_server_dbg(VFS,
>> +                                       "%s: sha256 alloc failed\n", __func__);
>> +                       return rc;
>> +               }
>> +               shash = &sdesc->shash;
>> +       } else {
>> +               hash = server->secmech.hmacsha256;
>> +               shash = &server->secmech.sdeschmacsha256->shash;
>>         }
>
>smb2_crypto_shash_allocate() unconditionally allocated
>server->secmech.hmacsha256 and server->secmech.sdeschmacsha256-
>>shash.

I think they are allocated in smb311_crypto_shash_allocate(), through
=> smb311_crypto_shash_allocate
 => smb311_update_preauth_hash
 => compound_send_recv
 => cifs_send_recv
 => SMB2_negotiate

The function names are a little misleading...

>Now the code doesn't allocate those variables at all. Unlike SMB3 where
>structures are allocated in during key generation, for SMB2 we do it on
>demand in smb2_calc_signature(). So, the code above should be changed to
>call smb2_crypto_shash_allocate() in "else" block.
>
>--
>Best regards,
>Pavel Shilovsky
Pavel Shilovsky April 4, 2020, 12:04 a.m. UTC | #3 
пт, 3 апр. 2020 г. в 16:11, Long Li <longli@microsoft.com>:
>
> >Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating
> >signatures of incoming packets
> >
> >вт, 31 мар. 2020 г. в 16:22, <longli@linuxonhyperv.com>:
> >>
> >> From: Long Li <longli@microsoft.com>
> >>
> >> CIFS uses pre-allocated crypto structures to calculate signatures for
> >> both incoming and outgoing packets. In this way it doesn't need to
> >> allocate crypto structures for every packet, but it requires a lock to
> >> prevent concurrent access to crypto structures.
> >>
> >> Remove the lock by allocating crypto structures on the fly for
> >> incoming packets. At the same time, we can still use pre-allocated
> >> crypto structures for outgoing packets, as they are already protected
> >> by transport lock srv_mutex.
> >>
> >> Signed-off-by: Long Li <longli@microsoft.com>
> >> ---
> >>  fs/cifs/cifsglob.h      |  3 +-
> >>  fs/cifs/smb2proto.h     |  6 ++-
> >>  fs/cifs/smb2transport.c | 87
> >> +++++++++++++++++++++++++----------------
> >>  3 files changed, 60 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index
> >> 0d956360e984..7448e7202e7a 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -426,7 +426,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 *,
> >> +                               bool allocate_crypto);
> >>         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..087d5f14320b 100644
> >> --- a/fs/cifs/smb2proto.h
> >> +++ b/fs/cifs/smb2proto.h
> >> @@ -55,9 +55,11 @@ 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,
> >> +                               bool allocate_crypto);
> >>  extern int smb3_calc_signature(struct smb_rqst *rqst,
> >> -                               struct TCP_Server_Info *server);
> >> +                               struct TCP_Server_Info *server,
> >> +                               bool allocate_crypto);
> >>  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..c01e19a3b112 100644
> >> --- a/fs/cifs/smb2transport.c
> >> +++ b/fs/cifs/smb2transport.c
> >> @@ -40,14 +40,6 @@
> >>  #include "smb2status.h"
> >>  #include "smb2glob.h"
> >>
> >> -static int
> >> -smb2_crypto_shash_allocate(struct TCP_Server_Info *server) -{
> >> -       return cifs_alloc_hash("hmac(sha256)",
> >> -                              &server->secmech.hmacsha256,
> >> -                              &server->secmech.sdeschmacsha256);
> >> -}
> >> -
> >>  static int
> >>  smb3_crypto_shash_allocate(struct TCP_Server_Info *server)  { @@
> >> -219,7 +211,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,
> >> +                       bool allocate_crypto)
> >>  {
> >>         int rc;
> >>         unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
> >> @@ -228,6 +221,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
> >TCP_Server_Info *server)
> >>         struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)iov[0].iov_base;
> >>         struct cifs_ses *ses;
> >>         struct shash_desc *shash;
> >> +       struct crypto_shash *hash;
> >> +       struct sdesc *sdesc = NULL;
> >>         struct smb_rqst drqst;
> >>
> >>         ses = smb2_find_smb_ses(server, shdr->SessionId); @@ -239,24
> >> +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
> >TCP_Server_Info *server)
> >>         memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
> >>         memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
> >>
> >> -       rc = smb2_crypto_shash_allocate(server);
> >> -       if (rc) {
> >> -               cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
> >> -               return rc;
> >> +       if (allocate_crypto) {
> >> +               rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
> >> +               if (rc) {
> >> +                       cifs_server_dbg(VFS,
> >> +                                       "%s: sha256 alloc failed\n", __func__);
> >> +                       return rc;
> >> +               }
> >> +               shash = &sdesc->shash;
> >> +       } else {
> >> +               hash = server->secmech.hmacsha256;
> >> +               shash = &server->secmech.sdeschmacsha256->shash;
> >>         }
> >
> >smb2_crypto_shash_allocate() unconditionally allocated
> >server->secmech.hmacsha256 and server->secmech.sdeschmacsha256-
> >>shash.
>
> I think they are allocated in smb311_crypto_shash_allocate(), through
> => smb311_crypto_shash_allocate
>  => smb311_update_preauth_hash
>  => compound_send_recv
>  => cifs_send_recv
>  => SMB2_negotiate
>
> The function names are a little misleading...

smb311_crypto_shash_allocate() only allocate those structures for SMB
3.1.1 protocol version, see the code below:

797 int
798 smb311_update_preauth_hash(struct cifs_ses *ses, struct kvec *iov, int nvec)
799 {
800 >-------int i, rc;
801 >-------struct sdesc *d;
802 >-------struct smb2_sync_hdr *hdr;
803
804 >-------if (ses->server->tcpStatus == CifsGood) {
805 >------->-------/* skip non smb311 connections */
806 >------->-------if (ses->server->dialect != SMB311_PROT_ID)
807 >------->------->-------return 0;

--
Best regards,
Pavel Shilovsky
Pavel Shilovsky April 4, 2020, 12:16 a.m. UTC | #4 
пт, 3 апр. 2020 г. в 17:04, Pavel Shilovsky <piastryyy@gmail.com>:
>
> пт, 3 апр. 2020 г. в 16:11, Long Li <longli@microsoft.com>:
> >
> > >Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating
> > >signatures of incoming packets
> > >
> > >вт, 31 мар. 2020 г. в 16:22, <longli@linuxonhyperv.com>:
> > >>
> > >> From: Long Li <longli@microsoft.com>
> > >>
> > >> CIFS uses pre-allocated crypto structures to calculate signatures for
> > >> both incoming and outgoing packets. In this way it doesn't need to
> > >> allocate crypto structures for every packet, but it requires a lock to
> > >> prevent concurrent access to crypto structures.
> > >>
> > >> Remove the lock by allocating crypto structures on the fly for
> > >> incoming packets. At the same time, we can still use pre-allocated
> > >> crypto structures for outgoing packets, as they are already protected
> > >> by transport lock srv_mutex.
> > >>
> > >> Signed-off-by: Long Li <longli@microsoft.com>
> > >> ---
> > >>  fs/cifs/cifsglob.h      |  3 +-
> > >>  fs/cifs/smb2proto.h     |  6 ++-
> > >>  fs/cifs/smb2transport.c | 87
> > >> +++++++++++++++++++++++++----------------
> > >>  3 files changed, 60 insertions(+), 36 deletions(-)
> > >>
> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index
> > >> 0d956360e984..7448e7202e7a 100644
> > >> --- a/fs/cifs/cifsglob.h
> > >> +++ b/fs/cifs/cifsglob.h
> > >> @@ -426,7 +426,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 *,
> > >> +                               bool allocate_crypto);
> > >>         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..087d5f14320b 100644
> > >> --- a/fs/cifs/smb2proto.h
> > >> +++ b/fs/cifs/smb2proto.h
> > >> @@ -55,9 +55,11 @@ 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,
> > >> +                               bool allocate_crypto);
> > >>  extern int smb3_calc_signature(struct smb_rqst *rqst,
> > >> -                               struct TCP_Server_Info *server);
> > >> +                               struct TCP_Server_Info *server,
> > >> +                               bool allocate_crypto);
> > >>  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..c01e19a3b112 100644
> > >> --- a/fs/cifs/smb2transport.c
> > >> +++ b/fs/cifs/smb2transport.c
> > >> @@ -40,14 +40,6 @@
> > >>  #include "smb2status.h"
> > >>  #include "smb2glob.h"
> > >>
> > >> -static int
> > >> -smb2_crypto_shash_allocate(struct TCP_Server_Info *server) -{
> > >> -       return cifs_alloc_hash("hmac(sha256)",
> > >> -                              &server->secmech.hmacsha256,
> > >> -                              &server->secmech.sdeschmacsha256);
> > >> -}
> > >> -
> > >>  static int
> > >>  smb3_crypto_shash_allocate(struct TCP_Server_Info *server)  { @@
> > >> -219,7 +211,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,
> > >> +                       bool allocate_crypto)
> > >>  {
> > >>         int rc;
> > >>         unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
> > >> @@ -228,6 +221,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
> > >TCP_Server_Info *server)
> > >>         struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)iov[0].iov_base;
> > >>         struct cifs_ses *ses;
> > >>         struct shash_desc *shash;
> > >> +       struct crypto_shash *hash;
> > >> +       struct sdesc *sdesc = NULL;
> > >>         struct smb_rqst drqst;
> > >>
> > >>         ses = smb2_find_smb_ses(server, shdr->SessionId); @@ -239,24
> > >> +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
> > >TCP_Server_Info *server)
> > >>         memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
> > >>         memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
> > >>
> > >> -       rc = smb2_crypto_shash_allocate(server);
> > >> -       if (rc) {
> > >> -               cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
> > >> -               return rc;
> > >> +       if (allocate_crypto) {
> > >> +               rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
> > >> +               if (rc) {
> > >> +                       cifs_server_dbg(VFS,
> > >> +                                       "%s: sha256 alloc failed\n", __func__);
> > >> +                       return rc;
> > >> +               }
> > >> +               shash = &sdesc->shash;
> > >> +       } else {
> > >> +               hash = server->secmech.hmacsha256;
> > >> +               shash = &server->secmech.sdeschmacsha256->shash;
> > >>         }
> > >
> > >smb2_crypto_shash_allocate() unconditionally allocated
> > >server->secmech.hmacsha256 and server->secmech.sdeschmacsha256-
> > >>shash.
> >
> > I think they are allocated in smb311_crypto_shash_allocate(), through
> > => smb311_crypto_shash_allocate
> >  => smb311_update_preauth_hash
> >  => compound_send_recv
> >  => cifs_send_recv
> >  => SMB2_negotiate
> >
> > The function names are a little misleading...
>
> smb311_crypto_shash_allocate() only allocate those structures for SMB
> 3.1.1 protocol version, see the code below:
>
> 797 int
> 798 smb311_update_preauth_hash(struct cifs_ses *ses, struct kvec *iov, int nvec)
> 799 {
> 800 >-------int i, rc;
> 801 >-------struct sdesc *d;
> 802 >-------struct smb2_sync_hdr *hdr;
> 803
> 804 >-------if (ses->server->tcpStatus == CifsGood) {
> 805 >------->-------/* skip non smb311 connections */
> 806 >------->-------if (ses->server->dialect != SMB311_PROT_ID)
> 807 >------->------->-------return 0;
>

+ Aurelien

Ok, before negotiate tcpStatus is not CifsGood, so, the allocation
won't be skipped. I think this function should be no-op for all
protocols except SMB 3.1.1 to reflect the meaning. Other protocols
don't use preauth hash anyway.

@Aurelien, @everybody, what would be your thoughts about moving
protocol version check from IF block to the top of the function thus
skipping to allocate preauth hash for protocols that don't need it? In
this case Long's patch will require a change to keep
smb2_crypto_shash_allocate() and its invocation.

--
Best regards,
Pavel Shilovsky
Aurélien Aptel April 6, 2020, 1:05 p.m. UTC | #5 
Pavel Shilovsky <piastryyy@gmail.com> writes:
> + Aurelien
>
> Ok, before negotiate tcpStatus is not CifsGood, so, the allocation
> won't be skipped. I think this function should be no-op for all
> protocols except SMB 3.1.1 to reflect the meaning. Other protocols
> don't use preauth hash anyway.
>
> @Aurelien, @everybody, what would be your thoughts about moving
> protocol version check from IF block to the top of the function thus
> skipping to allocate preauth hash for protocols that don't need it? In
> this case Long's patch will require a change to keep
> smb2_crypto_shash_allocate() and its invocation.

Sounds good. I think we could go even further and do preauth crypto
alloc/dealloc all in the preauth hash compute function too and remove
the pointers from the server struct (except for the byte array with the
hash result). It only runs during negprot+session establishement which
is not a hot path at all and would keep the memory model simpler.

Cheers,
diff mbox series

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 0d956360e984..7448e7202e7a 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -426,7 +426,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 *,
+				bool allocate_crypto);
 	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..087d5f14320b 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -55,9 +55,11 @@  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,
+				bool allocate_crypto);
 extern int smb3_calc_signature(struct smb_rqst *rqst,
-				struct TCP_Server_Info *server);
+				struct TCP_Server_Info *server,
+				bool allocate_crypto);
 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..c01e19a3b112 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -40,14 +40,6 @@ 
 #include "smb2status.h"
 #include "smb2glob.h"
 
-static int
-smb2_crypto_shash_allocate(struct TCP_Server_Info *server)
-{
-	return cifs_alloc_hash("hmac(sha256)",
-			       &server->secmech.hmacsha256,
-			       &server->secmech.sdeschmacsha256);
-}
-
 static int
 smb3_crypto_shash_allocate(struct TCP_Server_Info *server)
 {
@@ -219,7 +211,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,
+			bool allocate_crypto)
 {
 	int rc;
 	unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
@@ -228,6 +221,8 @@  smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)iov[0].iov_base;
 	struct cifs_ses *ses;
 	struct shash_desc *shash;
+	struct crypto_shash *hash;
+	struct sdesc *sdesc = NULL;
 	struct smb_rqst drqst;
 
 	ses = smb2_find_smb_ses(server, shdr->SessionId);
@@ -239,24 +234,32 @@  smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
 	memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
 
-	rc = smb2_crypto_shash_allocate(server);
-	if (rc) {
-		cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
-		return rc;
+	if (allocate_crypto) {
+		rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
+		if (rc) {
+			cifs_server_dbg(VFS,
+					"%s: sha256 alloc failed\n", __func__);
+			return rc;
+		}
+		shash = &sdesc->shash;
+	} else {
+		hash = server->secmech.hmacsha256;
+		shash = &server->secmech.sdeschmacsha256->shash;
 	}
 
-	rc = crypto_shash_setkey(server->secmech.hmacsha256,
-				 ses->auth_key.response, SMB2_NTLMV2_SESSKEY_SIZE);
+	rc = crypto_shash_setkey(hash, ses->auth_key.response,
+			SMB2_NTLMV2_SESSKEY_SIZE);
 	if (rc) {
-		cifs_server_dbg(VFS, "%s: Could not update with response\n", __func__);
-		return rc;
+		cifs_server_dbg(VFS,
+				"%s: Could not update with response\n",
+				__func__);
+		goto out;
 	}
 
-	shash = &server->secmech.sdeschmacsha256->shash;
 	rc = crypto_shash_init(shash);
 	if (rc) {
 		cifs_server_dbg(VFS, "%s: Could not init sha256", __func__);
-		return rc;
+		goto out;
 	}
 
 	/*
@@ -271,9 +274,10 @@  smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 		rc = crypto_shash_update(shash, iov[0].iov_base,
 					 iov[0].iov_len);
 		if (rc) {
-			cifs_server_dbg(VFS, "%s: Could not update with payload\n",
-				 __func__);
-			return rc;
+			cifs_server_dbg(VFS,
+					"%s: Could not update with payload\n",
+					__func__);
+			goto out;
 		}
 		drqst.rq_iov++;
 		drqst.rq_nvec--;
@@ -283,6 +287,9 @@  smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	if (!rc)
 		memcpy(shdr->Signature, sigptr, SMB2_SIGNATURE_SIZE);
 
+out:
+	if (allocate_crypto)
+		cifs_free_hash(&hash, &sdesc);
 	return rc;
 }
 
@@ -504,14 +511,17 @@  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,
+			bool allocate_crypto)
 {
 	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 crypto_shash *hash;
+	struct sdesc *sdesc = NULL;
 	struct smb_rqst drqst;
 	u8 key[SMB3_SIGN_KEY_SIZE];
 
@@ -519,14 +529,24 @@  smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	if (rc)
 		return 0;
 
+	if (allocate_crypto) {
+		rc = cifs_alloc_hash("cmac(aes)", &hash, &sdesc);
+		if (rc)
+			return rc;
+
+		shash = &sdesc->shash;
+	} else {
+		hash = server->secmech.cmacaes;
+		shash = &server->secmech.sdesccmacaes->shash;
+	}
+
 	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);
+	rc = crypto_shash_setkey(hash, key, SMB2_CMACAES_SIZE);
 	if (rc) {
 		cifs_server_dbg(VFS, "%s: Could not set key for cmac aes\n", __func__);
-		return rc;
+		goto out;
 	}
 
 	/*
@@ -537,7 +557,7 @@  smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	rc = crypto_shash_init(shash);
 	if (rc) {
 		cifs_server_dbg(VFS, "%s: Could not init cmac aes\n", __func__);
-		return rc;
+		goto out;
 	}
 
 	/*
@@ -554,7 +574,7 @@  smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 		if (rc) {
 			cifs_server_dbg(VFS, "%s: Could not update with payload\n",
 				 __func__);
-			return rc;
+			goto out;
 		}
 		drqst.rq_iov++;
 		drqst.rq_nvec--;
@@ -564,6 +584,9 @@  smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 	if (!rc)
 		memcpy(shdr->Signature, sigptr, SMB2_SIGNATURE_SIZE);
 
+out:
+	if (allocate_crypto)
+		cifs_free_hash(&hash, &sdesc);
 	return rc;
 }
 
@@ -593,7 +616,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, false);
 
 	return rc;
 }
@@ -631,9 +654,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, true);
 
 	if (rc)
 		return rc;