On 10/03, Tom Talpey wrote:
On 10/3/2022 10:53 AM, Enzo Matsumiya wrote:
On 10/03, Tom Talpey wrote:
On 10/2/2022 11:54 PM, Steve French wrote:
shash was not being initialized in one place in smb3_calc_signature
Suggested-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
I don't see the issue. The shash pointer is initialized in both
arms of the "if (allocate_crypto)" block.
True, but cifs_alloc_hash() returns 0 if *sdesc is not NULL, so
crypto_shash_setkey() got stack garbage as sdesc.
Sorry, I still don't get it.
if (allocate_crypto) {
rc = cifs_alloc_hash("cmac(aes)", &hash, &sdesc);
I think you're looking at an old HEAD. We've hit this bug after my
patch 10c6c7b0f060 "cifs: secmech: use shash_desc directly, remove sdesc"
which changes the above line to:
rc = cifs_alloc_hash("cmac(aes)", &shash);
In that patch, shash is the only struct to be initialized.
cifs_alloc_hash() is:
cifs_alloc_hash(const char *name, struct shash_desc **sdesc)
{
int rc = 0;
struct crypto_shash *alg = NULL;
if (*sdesc)
return 0;
...
Hence, sdesc having the stack garbage, cifs_alloc_hash returns 0 and
crypto_shash_setkey crashes.
if (rc)
return rc;
shash = &sdesc->shash;
} else {
hash = server->secmech.cmacaes;
shash = &server->secmech.sdesccmacaes->shash;
}
The "shash" pointer is initialized one line later.
And, "sdesc" is already initilized to NULL.
Steve's patch initializes "shash", but now you're referring to
sdesc, and it still looks correct to me. Confused...
But if you do want to add this, them smb2_calc_signature has
the same code.
True. Steve will you add it to this patch please?
FYI, smb2_calc_signature() also initializes sdesc, and not shash.
Does it not oops? Same code.
Tom.
