| Message ID | CAH2r5mtzWgXbod2tdZsRvZuhBZsv=H9Vf2GA3Q_bQe0nHhjfiQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [smb311,client] fix oops in smb3_calc_signature | expand |
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@suse.de> > Signed-off-by: Steve French <stfrench@microsoft.com> > I don't see the issue. The shash pointer is initialized in both arms of the "if (allocate_crypto)" block. But if you do want to add this, them smb2_calc_signature has the same code. Tom.
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@suse.de> >>Signed-off-by: Steve French <stfrench@microsoft.com> >> > >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. >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? >Tom. Cheers, Enzo
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@suse.de> >>> Signed-off-by: Steve French <stfrench@microsoft.com> >>> >> >> 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); 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.
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@suse.de> >>>>Signed-off-by: Steve French <stfrench@microsoft.com> >>>> >>> >>>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.
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@suse.de> >>>>> Signed-off-by: Steve French <stfrench@microsoft.com> >>>>> >>>> >>>> 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. >
updated to include a similar initialization in smb2_calc_signature See attached. On Mon, Oct 3, 2022 at 10:27 AM Tom Talpey <tom@talpey.com> 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@suse.de> > >>>>> Signed-off-by: Steve French <stfrench@microsoft.com> > >>>>> > >>>> > >>>> 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. > >
On 10/3/2022 11:32 AM, Steve French wrote: > updated to include a similar initialization in smb2_calc_signature > Acked-by: Tom Talpey <tom@talpey.com> But I'd still love to see the tree this applies to! Tom. > See attached. > > On Mon, Oct 3, 2022 at 10:27 AM Tom Talpey <tom@talpey.com> 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@suse.de> >>>>>>> Signed-off-by: Steve French <stfrench@microsoft.com> >>>>>>> >>>>>> >>>>>> 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. >>> > > >
On 10/03, Tom Talpey 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@suse.de> >>>>>>Signed-off-by: Steve French <stfrench@microsoft.com> >>>>>> >>>>> >>>>>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? It's in cifs-2.6 for-next branch, HEAD 11b1c98d0986 (already has the fix for smb2_calc_signature). >Tom. Cheers, Enzo >>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. >>
On 10/3/2022 12:39 PM, Enzo Matsumiya wrote: > On 10/03, Tom Talpey 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@suse.de> >>>>>>> Signed-off-by: Steve French <stfrench@microsoft.com> >>>>>>> >>>>>> >>>>>> 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? > > It's in cifs-2.6 for-next branch, HEAD 11b1c98d0986 (already has the fix > for smb2_calc_signature). I can't see branches from the git.samba.org web interface. Are there any browsing alternatives to pulling a full repo? Using git on WSL is really, really slow. Tom. > > >> Tom. > > Cheers, > > Enzo > >>> 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. >>> >
On 10/03, Tom Talpey wrote: >On 10/3/2022 12:39 PM, Enzo Matsumiya wrote: >>On 10/03, Tom Talpey 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@suse.de> >>>>>>>>Signed-off-by: Steve French <stfrench@microsoft.com> >>>>>>>> >>>>>>> >>>>>>>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? >> >>It's in cifs-2.6 for-next branch, HEAD 11b1c98d0986 (already has the fix >>for smb2_calc_signature). > >I can't see branches from the git.samba.org web interface. https://git.samba.org/?p=sfrench/cifs-2.6.git;a=shortlog;h=refs/heads/for-next The branches are in the 'heads' section down below the summary page. Unintuitive, I'd say... Enzo >Are there any browsing alternatives to pulling a full repo? Using >git on WSL is really, really slow. > >Tom. > >> >> >>>Tom. >> >>Cheers, >> >>Enzo >> >>>>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. >>>> >>
From 65145a4b3b4435a70c0062af5c6cf225b25e9a5f Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Sun, 2 Oct 2022 22:09:45 -0500 Subject: [PATCH] fix oops in calculating shash_setkey shash was not being initialized in one place in smb3_calc_signature Suggested-by: Enzo Matsumiya <ematsumiya@suse.de> Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/smb2transport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c index dfcbcc0b86e4..64e12b0a5a3b 100644 --- a/fs/cifs/smb2transport.c +++ b/fs/cifs/smb2transport.c @@ -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
shash was not being initialized in one place in smb3_calc_signature Suggested-by: Enzo Matsumiya <ematsumiya@suse.de> Signed-off-by: Steve French <stfrench@microsoft.com>