* SHA1-MB algorithm broken on latest kernel @ 2016-05-12 23:31 Megha Dey 2016-05-13 3:10 ` Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: Megha Dey @ 2016-05-12 23:31 UTC (permalink / raw) To: Herbert Xu; +Cc: linux-crypto, linux-kernel Hi, When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I observe a panic. After having a quick look, on reverting the following patches, I am able to complete the booting process. aec4d0e301f17bb143341c82cc44685b8af0b945 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. The r10 to r15 registers are used in sha1_x8_avx2.S, which is called from sha1_mb_mgr_flush_avx2.S. I do not think the functionality of the SHA1-MB crypto algorithm has been tested after applying these changes. (I am not sure if any of the other crypto algorithms have been affected by these changes). Thanks, Megha ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: SHA1-MB algorithm broken on latest kernel 2016-05-12 23:31 SHA1-MB algorithm broken on latest kernel Megha Dey @ 2016-05-13 3:10 ` Herbert Xu 2016-05-13 5:51 ` Ingo Molnar 0 siblings, 1 reply; 11+ messages in thread From: Herbert Xu @ 2016-05-13 3:10 UTC (permalink / raw) To: Megha Dey; +Cc: linux-crypto, linux-kernel, Josh Poimboeuf, Ingo Molnar On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > Hi, > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > observe a panic. > > After having a quick look, on reverting the following patches, I am able > to complete the booting process. > aec4d0e301f17bb143341c82cc44685b8af0b945 > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > from sha1_mb_mgr_flush_avx2.S. > > I do not think the functionality of the SHA1-MB crypto algorithm has > been tested after applying these changes. (I am not sure if any of the > other crypto algorithms have been affected by these changes). Josh, Ingo: Any ideas on this? Should we revert? Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: SHA1-MB algorithm broken on latest kernel 2016-05-13 3:10 ` Herbert Xu @ 2016-05-13 5:51 ` Ingo Molnar 2016-05-13 17:32 ` Megha Dey 0 siblings, 1 reply; 11+ messages in thread From: Ingo Molnar @ 2016-05-13 5:51 UTC (permalink / raw) To: Herbert Xu; +Cc: Megha Dey, linux-crypto, linux-kernel, Josh Poimboeuf * Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > Hi, > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > observe a panic. > > > > After having a quick look, on reverting the following patches, I am able > > to complete the booting process. > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > from sha1_mb_mgr_flush_avx2.S. > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > been tested after applying these changes. (I am not sure if any of the > > other crypto algorithms have been affected by these changes). > > Josh, Ingo: > > Any ideas on this? Should we revert? Yeah, I think so - although another option would be to standardize sha1_x8_avx2() - the problem is that it is a function that clobbers registers without saving/restoring them, breaking the C function ABI. I realize it's written in assembly, but unless there are strong performance reasons to deviate from the regular calling convention it might make sense to fix that. Do any warnings get generated after the revert, if you enable CONFIG_STACK_VALIDATION=y? Thanks, Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: SHA1-MB algorithm broken on latest kernel 2016-05-13 5:51 ` Ingo Molnar @ 2016-05-13 17:32 ` Megha Dey 2016-05-16 14:44 ` Josh Poimboeuf 0 siblings, 1 reply; 11+ messages in thread From: Megha Dey @ 2016-05-13 17:32 UTC (permalink / raw) To: Ingo Molnar; +Cc: Herbert Xu, linux-crypto, linux-kernel, Josh Poimboeuf On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > Hi, > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > observe a panic. > > > > > > After having a quick look, on reverting the following patches, I am able > > > to complete the booting process. > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > been tested after applying these changes. (I am not sure if any of the > > > other crypto algorithms have been affected by these changes). > > > > Josh, Ingo: > > > > Any ideas on this? Should we revert? > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > - the problem is that it is a function that clobbers registers without > saving/restoring them, breaking the C function ABI. I realize it's written in > assembly, but unless there are strong performance reasons to deviate from the > regular calling convention it might make sense to fix that. > > Do any warnings get generated after the revert, if you enable > CONFIG_STACK_VALIDATION=y? After the revert and enabling CONFIG_STACK_VALIDATION: arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > Thanks, > > Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: SHA1-MB algorithm broken on latest kernel 2016-05-13 17:32 ` Megha Dey @ 2016-05-16 14:44 ` Josh Poimboeuf 2016-05-16 18:31 ` Megha Dey 0 siblings, 1 reply; 11+ messages in thread From: Josh Poimboeuf @ 2016-05-16 14:44 UTC (permalink / raw) To: Megha Dey; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > Hi, > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > observe a panic. > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > to complete the booting process. > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > been tested after applying these changes. (I am not sure if any of the > > > > other crypto algorithms have been affected by these changes). > > > > > > Josh, Ingo: > > > > > > Any ideas on this? Should we revert? > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > - the problem is that it is a function that clobbers registers without > > saving/restoring them, breaking the C function ABI. I realize it's written in > > assembly, but unless there are strong performance reasons to deviate from the > > regular calling convention it might make sense to fix that. > > > > Do any warnings get generated after the revert, if you enable > > CONFIG_STACK_VALIDATION=y? > > After the revert and enabling CONFIG_STACK_VALIDATION: > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup Megha, Sorry for breaking it. I completely missed the fact that the function calls sha1_x8_avx2() which clobbers registers. If the performance penalty isn't too bad, I'll submit a patch to standardize sha1_x8_avx2() to follow the C ABI. Do you have any tips for testing this code? I've tried using the tcrypt module, but no luck. -- Josh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: SHA1-MB algorithm broken on latest kernel 2016-05-16 14:44 ` Josh Poimboeuf @ 2016-05-16 18:31 ` Megha Dey 2016-05-16 20:16 ` [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI Josh Poimboeuf 0 siblings, 1 reply; 11+ messages in thread From: Megha Dey @ 2016-05-16 18:31 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote: > On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > > Hi, > > > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > > observe a panic. > > > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > > to complete the booting process. > > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > > been tested after applying these changes. (I am not sure if any of the > > > > > other crypto algorithms have been affected by these changes). > > > > > > > > Josh, Ingo: > > > > > > > > Any ideas on this? Should we revert? > > > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > > - the problem is that it is a function that clobbers registers without > > > saving/restoring them, breaking the C function ABI. I realize it's written in > > > assembly, but unless there are strong performance reasons to deviate from the > > > regular calling convention it might make sense to fix that. > > > > > > Do any warnings get generated after the revert, if you enable > > > CONFIG_STACK_VALIDATION=y? > > > > After the revert and enabling CONFIG_STACK_VALIDATION: > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > Megha, > > Sorry for breaking it. I completely missed the fact that the function > calls sha1_x8_avx2() which clobbers registers. > > If the performance penalty isn't too bad, I'll submit a patch to > standardize sha1_x8_avx2() to follow the C ABI. > > Do you have any tips for testing this code? I've tried using the tcrypt > module, but no luck. > Josh, Build the kernel with the following configs: CONFIG_CRYPTO_SHA1_MB=y CONFIG_CRYPTO_TEST=m CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n There was a kernel panic while booting. So if after applying your new patch, we are able to get complete the boot, then we are good. Could you please send a copy of the patch, I could test it on my end too. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI 2016-05-16 18:31 ` Megha Dey @ 2016-05-16 20:16 ` Josh Poimboeuf 2016-05-16 21:39 ` Megha Dey 0 siblings, 1 reply; 11+ messages in thread From: Josh Poimboeuf @ 2016-05-16 20:16 UTC (permalink / raw) To: Megha Dey; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote: > On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote: > > On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > > > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > > > Hi, > > > > > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > > > observe a panic. > > > > > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > > > to complete the booting process. > > > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > > > been tested after applying these changes. (I am not sure if any of the > > > > > > other crypto algorithms have been affected by these changes). > > > > > > > > > > Josh, Ingo: > > > > > > > > > > Any ideas on this? Should we revert? > > > > > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > > > - the problem is that it is a function that clobbers registers without > > > > saving/restoring them, breaking the C function ABI. I realize it's written in > > > > assembly, but unless there are strong performance reasons to deviate from the > > > > regular calling convention it might make sense to fix that. > > > > > > > > Do any warnings get generated after the revert, if you enable > > > > CONFIG_STACK_VALIDATION=y? > > > > > > After the revert and enabling CONFIG_STACK_VALIDATION: > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > > > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > > > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > > > Megha, > > > > Sorry for breaking it. I completely missed the fact that the function > > calls sha1_x8_avx2() which clobbers registers. > > > > If the performance penalty isn't too bad, I'll submit a patch to > > standardize sha1_x8_avx2() to follow the C ABI. > > > > Do you have any tips for testing this code? I've tried using the tcrypt > > module, but no luck. > > > Josh, > Build the kernel with the following configs: > CONFIG_CRYPTO_SHA1_MB=y > CONFIG_CRYPTO_TEST=m > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n > There was a kernel panic while booting. > So if after applying your new patch, we are able to get complete the > boot, then we are good. > > Could you please send a copy of the patch, I could test it on my end > too. Thanks. I was able to run the tests, though I didn't see a panic. Can you test with this patch? ---- From: Josh Poimboeuf <jpoimboe@redhat.com> Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI Megha Day reported a kernel panic in crypto code. The problem is that sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring them. Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions"), those registers were saved and restored by the callers of the function. I removed them with that commit because I didn't realize sha1_x8_avx2() clobbered them. Fix the potential undefined behavior associated with clobbering the registers and make the behavior less surprising by changing the registers to be callee saved/restored to conform with the C function call ABI. Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none of the callers rely on it being saved, and it's not a callee-saved register in the C ABI. Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions") Cc: stable@vger.kernel.org # 4.6 Reported-by: Megha Dey <megha.dey@linux.intel.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S index 8e1b477..c9dae1c 100644 --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S @@ -296,7 +296,11 @@ W14 = TMP_ # ENTRY(sha1_x8_avx2) - push RSP_SAVE + # save callee-saved clobbered registers to comply with C function ABI + push %r12 + push %r13 + push %r14 + push %r15 #save rsp mov %rsp, RSP_SAVE @@ -446,7 +450,12 @@ lloop: ## Postamble mov RSP_SAVE, %rsp - pop RSP_SAVE + + # restore callee-saved clobbered registers + pop %r15 + pop %r14 + pop %r13 + pop %r12 ret ENDPROC(sha1_x8_avx2) -- 2.4.11 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI 2016-05-16 20:16 ` [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI Josh Poimboeuf @ 2016-05-16 21:39 ` Megha Dey 2016-05-16 21:46 ` Josh Poimboeuf 0 siblings, 1 reply; 11+ messages in thread From: Megha Dey @ 2016-05-16 21:39 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote: > On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote: > > On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote: > > > On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > > > > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > > > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > > > > Hi, > > > > > > > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > > > > observe a panic. > > > > > > > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > > > > to complete the booting process. > > > > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > > > > been tested after applying these changes. (I am not sure if any of the > > > > > > > other crypto algorithms have been affected by these changes). > > > > > > > > > > > > Josh, Ingo: > > > > > > > > > > > > Any ideas on this? Should we revert? > > > > > > > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > > > > - the problem is that it is a function that clobbers registers without > > > > > saving/restoring them, breaking the C function ABI. I realize it's written in > > > > > assembly, but unless there are strong performance reasons to deviate from the > > > > > regular calling convention it might make sense to fix that. > > > > > > > > > > Do any warnings get generated after the revert, if you enable > > > > > CONFIG_STACK_VALIDATION=y? > > > > > > > > After the revert and enabling CONFIG_STACK_VALIDATION: > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > > > > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > > > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > > > > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > > > > > Megha, > > > > > > Sorry for breaking it. I completely missed the fact that the function > > > calls sha1_x8_avx2() which clobbers registers. > > > > > > If the performance penalty isn't too bad, I'll submit a patch to > > > standardize sha1_x8_avx2() to follow the C ABI. > > > > > > Do you have any tips for testing this code? I've tried using the tcrypt > > > module, but no luck. > > > > > Josh, > > Build the kernel with the following configs: > > CONFIG_CRYPTO_SHA1_MB=y > > CONFIG_CRYPTO_TEST=m > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n > > There was a kernel panic while booting. > > So if after applying your new patch, we are able to get complete the > > boot, then we are good. > > > > Could you please send a copy of the patch, I could test it on my end > > too. > > Thanks. I was able to run the tests, though I didn't see a panic. Can > you test with this patch? > > ---- > > From: Josh Poimboeuf <jpoimboe@redhat.com> > Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI > > Megha Day reported a kernel panic in crypto code. The problem is that > sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring > them. > > Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in > sha-mb functions"), those registers were saved and restored by the > callers of the function. I removed them with that commit because I > didn't realize sha1_x8_avx2() clobbered them. > > Fix the potential undefined behavior associated with clobbering the > registers and make the behavior less surprising by changing the > registers to be callee saved/restored to conform with the C function > call ABI. > > Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none > of the callers rely on it being saved, and it's not a callee-saved > register in the C ABI. > > Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions") > Cc: stable@vger.kernel.org # 4.6 > Reported-by: Megha Dey <megha.dey@linux.intel.com> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > --- > arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > index 8e1b477..c9dae1c 100644 > --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > @@ -296,7 +296,11 @@ W14 = TMP_ > # > ENTRY(sha1_x8_avx2) > > - push RSP_SAVE > + # save callee-saved clobbered registers to comply with C function ABI > + push %r12 > + push %r13 > + push %r14 > + push %r15 > > #save rsp > mov %rsp, RSP_SAVE > @@ -446,7 +450,12 @@ lloop: > ## Postamble > > mov RSP_SAVE, %rsp > - pop RSP_SAVE > + > + # restore callee-saved clobbered registers > + pop %r15 > + pop %r14 > + pop %r13 > + pop %r12 > > ret > ENDPROC(sha1_x8_avx2) Hi Josh, I don't see the panic and am able to boot. However, I am not able to see the tests running. You said you were able to insert the tcrypt module and test this right? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI 2016-05-16 21:39 ` Megha Dey @ 2016-05-16 21:46 ` Josh Poimboeuf 2016-05-16 23:07 ` Megha Dey 0 siblings, 1 reply; 11+ messages in thread From: Josh Poimboeuf @ 2016-05-16 21:46 UTC (permalink / raw) To: Megha Dey; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Mon, May 16, 2016 at 02:39:06PM -0700, Megha Dey wrote: > On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote: > > On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote: > > > On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote: > > > > On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > > > > > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > > > > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > > > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > > > > > observe a panic. > > > > > > > > > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > > > > > to complete the booting process. > > > > > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > > > > > been tested after applying these changes. (I am not sure if any of the > > > > > > > > other crypto algorithms have been affected by these changes). > > > > > > > > > > > > > > Josh, Ingo: > > > > > > > > > > > > > > Any ideas on this? Should we revert? > > > > > > > > > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > > > > > - the problem is that it is a function that clobbers registers without > > > > > > saving/restoring them, breaking the C function ABI. I realize it's written in > > > > > > assembly, but unless there are strong performance reasons to deviate from the > > > > > > regular calling convention it might make sense to fix that. > > > > > > > > > > > > Do any warnings get generated after the revert, if you enable > > > > > > CONFIG_STACK_VALIDATION=y? > > > > > > > > > > After the revert and enabling CONFIG_STACK_VALIDATION: > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > > > > > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > > > > > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > > > > > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > > > > > > > Megha, > > > > > > > > Sorry for breaking it. I completely missed the fact that the function > > > > calls sha1_x8_avx2() which clobbers registers. > > > > > > > > If the performance penalty isn't too bad, I'll submit a patch to > > > > standardize sha1_x8_avx2() to follow the C ABI. > > > > > > > > Do you have any tips for testing this code? I've tried using the tcrypt > > > > module, but no luck. > > > > > > > Josh, > > > Build the kernel with the following configs: > > > CONFIG_CRYPTO_SHA1_MB=y > > > CONFIG_CRYPTO_TEST=m > > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n > > > There was a kernel panic while booting. > > > So if after applying your new patch, we are able to get complete the > > > boot, then we are good. > > > > > > Could you please send a copy of the patch, I could test it on my end > > > too. > > > > Thanks. I was able to run the tests, though I didn't see a panic. Can > > you test with this patch? > > > > ---- > > > > From: Josh Poimboeuf <jpoimboe@redhat.com> > > Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI > > > > Megha Day reported a kernel panic in crypto code. The problem is that > > sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring > > them. > > > > Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in > > sha-mb functions"), those registers were saved and restored by the > > callers of the function. I removed them with that commit because I > > didn't realize sha1_x8_avx2() clobbered them. > > > > Fix the potential undefined behavior associated with clobbering the > > registers and make the behavior less surprising by changing the > > registers to be callee saved/restored to conform with the C function > > call ABI. > > > > Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none > > of the callers rely on it being saved, and it's not a callee-saved > > register in the C ABI. > > > > Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions") > > Cc: stable@vger.kernel.org # 4.6 > > Reported-by: Megha Dey <megha.dey@linux.intel.com> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > > --- > > arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > index 8e1b477..c9dae1c 100644 > > --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > @@ -296,7 +296,11 @@ W14 = TMP_ > > # > > ENTRY(sha1_x8_avx2) > > > > - push RSP_SAVE > > + # save callee-saved clobbered registers to comply with C function ABI > > + push %r12 > > + push %r13 > > + push %r14 > > + push %r15 > > > > #save rsp > > mov %rsp, RSP_SAVE > > @@ -446,7 +450,12 @@ lloop: > > ## Postamble > > > > mov RSP_SAVE, %rsp > > - pop RSP_SAVE > > + > > + # restore callee-saved clobbered registers > > + pop %r15 > > + pop %r14 > > + pop %r13 > > + pop %r12 > > > > ret > > ENDPROC(sha1_x8_avx2) > > Hi Josh, > I don't see the panic and am able to boot. However, I am not able to see > the tests running. You said you were able to insert the tcrypt module > and test this right? I didn't insert tcrypt manually, but I set the config flags you suggested, plus I added a few printks, and was at least able to verify that this code ran during boot without any crypto test errors or other warnings being reported. -- Josh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI 2016-05-16 21:46 ` Josh Poimboeuf @ 2016-05-16 23:07 ` Megha Dey 2016-05-17 6:31 ` Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: Megha Dey @ 2016-05-16 23:07 UTC (permalink / raw) To: Josh Poimboeuf; +Cc: Ingo Molnar, Herbert Xu, linux-crypto, linux-kernel On Mon, 2016-05-16 at 16:46 -0500, Josh Poimboeuf wrote: > On Mon, May 16, 2016 at 02:39:06PM -0700, Megha Dey wrote: > > On Mon, 2016-05-16 at 15:16 -0500, Josh Poimboeuf wrote: > > > On Mon, May 16, 2016 at 11:31:12AM -0700, Megha Dey wrote: > > > > On Mon, 2016-05-16 at 09:44 -0500, Josh Poimboeuf wrote: > > > > > On Fri, May 13, 2016 at 10:32:26AM -0700, Megha Dey wrote: > > > > > > On Fri, 2016-05-13 at 07:51 +0200, Ingo Molnar wrote: > > > > > > > * Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > > > > > > > > > On Thu, May 12, 2016 at 04:31:06PM -0700, Megha Dey wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > When booting latest kernel with the CONFIG_CRYPTO_SHA1_MB enabled, I > > > > > > > > > observe a panic. > > > > > > > > > > > > > > > > > > After having a quick look, on reverting the following patches, I am able > > > > > > > > > to complete the booting process. > > > > > > > > > aec4d0e301f17bb143341c82cc44685b8af0b945 > > > > > > > > > 8691ccd764f9ecc69a6812dfe76214c86ac9ba06 > > > > > > > > > 68874ac3304ade7ed5ebb12af00d6b9bbbca0a16 > > > > > > > > > > > > > > > > > > Of the 3 patches, aec4d0e301f17bb143341c82cc44685b8af0b945 seems wrong. > > > > > > > > > The r10 to r15 registers are used in sha1_x8_avx2.S, which is called > > > > > > > > > from sha1_mb_mgr_flush_avx2.S. > > > > > > > > > > > > > > > > > > I do not think the functionality of the SHA1-MB crypto algorithm has > > > > > > > > > been tested after applying these changes. (I am not sure if any of the > > > > > > > > > other crypto algorithms have been affected by these changes). > > > > > > > > > > > > > > > > Josh, Ingo: > > > > > > > > > > > > > > > > Any ideas on this? Should we revert? > > > > > > > > > > > > > > Yeah, I think so - although another option would be to standardize sha1_x8_avx2() > > > > > > > - the problem is that it is a function that clobbers registers without > > > > > > > saving/restoring them, breaking the C function ABI. I realize it's written in > > > > > > > assembly, but unless there are strong performance reasons to deviate from the > > > > > > > regular calling convention it might make sense to fix that. > > > > > > > > > > > > > > Do any warnings get generated after the revert, if you enable > > > > > > > CONFIG_STACK_VALIDATION=y? > > > > > > > > > > > > After the revert and enabling CONFIG_STACK_VALIDATION: > > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_flush_avx2.o: warning: objtool: > > > > > > sha1_mb_mgr_flush_avx2()+0x20d: call without frame pointer save/setup > > > > > > > > > > > > arch/x86/crypto/sha1-mb/sha1_mb_mgr_submit_avx2.o: warning: objtool: > > > > > > sha1_mb_mgr_submit_avx2()+0x115: call without frame pointer save/setup > > > > > > > > > > Megha, > > > > > > > > > > Sorry for breaking it. I completely missed the fact that the function > > > > > calls sha1_x8_avx2() which clobbers registers. > > > > > > > > > > If the performance penalty isn't too bad, I'll submit a patch to > > > > > standardize sha1_x8_avx2() to follow the C ABI. > > > > > > > > > > Do you have any tips for testing this code? I've tried using the tcrypt > > > > > module, but no luck. > > > > > > > > > Josh, > > > > Build the kernel with the following configs: > > > > CONFIG_CRYPTO_SHA1_MB=y > > > > CONFIG_CRYPTO_TEST=m > > > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n > > > > There was a kernel panic while booting. > > > > So if after applying your new patch, we are able to get complete the > > > > boot, then we are good. > > > > > > > > Could you please send a copy of the patch, I could test it on my end > > > > too. > > > > > > Thanks. I was able to run the tests, though I didn't see a panic. Can > > > you test with this patch? > > > > > > ---- > > > > > > From: Josh Poimboeuf <jpoimboe@redhat.com> > > > Subject: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI > > > > > > Megha Day reported a kernel panic in crypto code. The problem is that > > > sha1_x8_avx2() clobbers registers r12-r15 without saving and restoring > > > them. > > > > > > Before commit aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in > > > sha-mb functions"), those registers were saved and restored by the > > > callers of the function. I removed them with that commit because I > > > didn't realize sha1_x8_avx2() clobbered them. > > > > > > Fix the potential undefined behavior associated with clobbering the > > > registers and make the behavior less surprising by changing the > > > registers to be callee saved/restored to conform with the C function > > > call ABI. > > > > > > Also, rdx (aka RSP_SAVE) doesn't need to be saved: I verified that none > > > of the callers rely on it being saved, and it's not a callee-saved > > > register in the C ABI. > > > > > > Fixes: aec4d0e301f1 ("x86/asm/crypto: Simplify stack usage in sha-mb functions") > > > Cc: stable@vger.kernel.org # 4.6 > > > Reported-by: Megha Dey <megha.dey@linux.intel.com> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > > > --- > > > arch/x86/crypto/sha-mb/sha1_x8_avx2.S | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > > index 8e1b477..c9dae1c 100644 > > > --- a/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > > +++ b/arch/x86/crypto/sha-mb/sha1_x8_avx2.S > > > @@ -296,7 +296,11 @@ W14 = TMP_ > > > # > > > ENTRY(sha1_x8_avx2) > > > > > > - push RSP_SAVE > > > + # save callee-saved clobbered registers to comply with C function ABI > > > + push %r12 > > > + push %r13 > > > + push %r14 > > > + push %r15 > > > > > > #save rsp > > > mov %rsp, RSP_SAVE > > > @@ -446,7 +450,12 @@ lloop: > > > ## Postamble > > > > > > mov RSP_SAVE, %rsp > > > - pop RSP_SAVE > > > + > > > + # restore callee-saved clobbered registers > > > + pop %r15 > > > + pop %r14 > > > + pop %r13 > > > + pop %r12 > > > > > > ret > > > ENDPROC(sha1_x8_avx2) > > > > Hi Josh, > > I don't see the panic and am able to boot. However, I am not able to see > > the tests running. You said you were able to insert the tcrypt module > > and test this right? > > I didn't insert tcrypt manually, but I set the config flags you > suggested, plus I added a few printks, and was at least able to verify > that this code ran during boot without any crypto test errors or other > warnings being reported. > ok. I tried at my end too using some printk's and nothing seems to be broken. Also, if you intend to use the same commit message, I spell my name as Megha Dey and not Day :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI 2016-05-16 23:07 ` Megha Dey @ 2016-05-17 6:31 ` Herbert Xu 0 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2016-05-17 6:31 UTC (permalink / raw) To: Megha Dey; +Cc: Josh Poimboeuf, Ingo Molnar, linux-crypto, linux-kernel On Mon, May 16, 2016 at 04:07:53PM -0700, Megha Dey wrote: > > ok. I tried at my end too using some printk's and nothing seems to be > broken. Also, if you intend to use the same commit message, I spell my > name as Megha Dey and not Day :) Patch applied with the name corrected. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-05-17 6:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-12 23:31 SHA1-MB algorithm broken on latest kernel Megha Dey 2016-05-13 3:10 ` Herbert Xu 2016-05-13 5:51 ` Ingo Molnar 2016-05-13 17:32 ` Megha Dey 2016-05-16 14:44 ` Josh Poimboeuf 2016-05-16 18:31 ` Megha Dey 2016-05-16 20:16 ` [PATCH] crypto/sha1-mb: make sha1_x8_avx2() conform to C function ABI Josh Poimboeuf 2016-05-16 21:39 ` Megha Dey 2016-05-16 21:46 ` Josh Poimboeuf 2016-05-16 23:07 ` Megha Dey 2016-05-17 6:31 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).