RE: [PATCH] cifs: Allocate crypto structures on the fly for calculating signatures of incoming packets

[Long Li]

>Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating
>signatures of incoming packets
>
>ÐÑ, 31 ÐÐÑ. 2020 Ð. Ð 16:22, <longli@xxxxxxxxxxxxxxxxx>:
>>
>> From: Long Li <longli@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> ---
>>  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