updated to include a similar initialization in smb2_calc_signature
See attached.
On Mon, Oct 3, 2022 at 10:27 AM Tom Talpey <tom@xxxxxxxxxx> wrote:
>
> On 10/3/2022 11:20 AM, Enzo Matsumiya wrote:
> > 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"
>
> Aha. But I'm looking at Steve's current cifs-2.6. Where should
> I be looking?
>
>
> Tom.
>
>
> > 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.
> >
--
Thanks,
Steve
From 11b1c98d09861c65d7815f1dc4631e4ed6eb44a1 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@xxxxxxxxxxxxx>
Date: Sun, 2 Oct 2022 22:09:45 -0500
Subject: [PATCH] smb3: fix oops in calculating shash_setkey
shash was not being initialized in one place in smb3_calc_signature
and smb2_calc_signature
Reviewed-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/cifs/smb2transport.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index dfcbcc0b86e4..8e3f26e6f6b9 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -215,7 +215,7 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
struct kvec *iov = rqst->rq_iov;
struct smb2_hdr *shdr = (struct smb2_hdr *)iov[0].iov_base;
struct cifs_ses *ses;
- struct shash_desc *shash;
+ struct shash_desc *shash = NULL;
struct smb_rqst drqst;
ses = smb2_find_smb_ses(server, le64_to_cpu(shdr->SessionId));
@@ -535,7 +535,7 @@ smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
unsigned char *sigptr = smb3_signature;
struct kvec *iov = rqst->rq_iov;
struct smb2_hdr *shdr = (struct smb2_hdr *)iov[0].iov_base;
- struct shash_desc *shash;
+ struct shash_desc *shash = NULL;
struct smb_rqst drqst;
u8 key[SMB3_SIGN_KEY_SIZE];
--
2.34.1
