* [PATCH] x86: pad assembly functions with INT3
@ 2018-05-07 21:37 Alexey Dobriyan
2018-05-07 21:41 ` hpa
` (2 more replies)
0 siblings, 3 replies; 34+ messages in thread
From: Alexey Dobriyan @ 2018-05-07 21:37 UTC (permalink / raw)
To: tglx, mingo, hpa; +Cc: linux-kernel, x86
Use INT3 instead of NOP. All that padding between functions is
an illegal area, no legitimate code should jump into it.
I've checked x86_64 allyesconfig disassembly, all changes looks sane:
INT3 is only used after RET or unconditional JMP.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
arch/x86/include/asm/linkage.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/include/asm/linkage.h
+++ b/arch/x86/include/asm/linkage.h
@@ -18,7 +18,7 @@
name:
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_ALIGNMENT_16)
-#define __ALIGN .p2align 4, 0x90
+#define __ALIGN .p2align 4, 0xCC
#define __ALIGN_STR __stringify(__ALIGN)
#endif
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-07 21:37 [PATCH] x86: pad assembly functions with INT3 Alexey Dobriyan @ 2018-05-07 21:41 ` hpa 2018-05-09 16:55 ` Alexey Dobriyan 2018-05-10 16:39 ` David Laight 2018-05-14 12:53 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions tip-bot for Alexey Dobriyan 2 siblings, 1 reply; 34+ messages in thread From: hpa @ 2018-05-07 21:41 UTC (permalink / raw) To: Alexey Dobriyan, tglx, mingo; +Cc: linux-kernel, x86 On May 7, 2018 2:37:55 PM PDT, Alexey Dobriyan <adobriyan@gmail.com> wrote: >Use INT3 instead of NOP. All that padding between functions is >an illegal area, no legitimate code should jump into it. > >I've checked x86_64 allyesconfig disassembly, all changes looks sane: >INT3 is only used after RET or unconditional JMP. > >Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> >--- > > arch/x86/include/asm/linkage.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >--- a/arch/x86/include/asm/linkage.h >+++ b/arch/x86/include/asm/linkage.h >@@ -18,7 +18,7 @@ > name: > > #if defined(CONFIG_X86_64) || defined(CONFIG_X86_ALIGNMENT_16) >-#define __ALIGN .p2align 4, 0x90 >+#define __ALIGN .p2align 4, 0xCC > #define __ALIGN_STR __stringify(__ALIGN) > #endif > Acked-by: H. Peter Anvin <h.peter.anvin@in tel.com> -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-07 21:41 ` hpa @ 2018-05-09 16:55 ` Alexey Dobriyan 2018-05-09 19:28 ` H. Peter Anvin 0 siblings, 1 reply; 34+ messages in thread From: Alexey Dobriyan @ 2018-05-09 16:55 UTC (permalink / raw) To: hpa; +Cc: tglx, mingo, linux-kernel, x86 On Mon, May 07, 2018 at 02:41:14PM -0700, hpa@zytor.com wrote: > >-#define __ALIGN .p2align 4, 0x90 > >+#define __ALIGN .p2align 4, 0xCC > > Acked-by: H. Peter Anvin <h.peter.anvin@in tel.com> Thanks! If someone knows how to control _compiler_ inter-function padding, please tell. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-09 16:55 ` Alexey Dobriyan @ 2018-05-09 19:28 ` H. Peter Anvin 0 siblings, 0 replies; 34+ messages in thread From: H. Peter Anvin @ 2018-05-09 19:28 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: tglx, mingo, linux-kernel, x86 On 05/09/18 09:55, Alexey Dobriyan wrote: > On Mon, May 07, 2018 at 02:41:14PM -0700, hpa@zytor.com wrote: >>> -#define __ALIGN .p2align 4, 0x90 >>> +#define __ALIGN .p2align 4, 0xCC >> >> Acked-by: H. Peter Anvin <h.peter.anvin@in tel.com> > > Thanks! > > If someone knows how to control _compiler_ inter-function padding, > please tell. > I think I have a gcc feature enhancement request for that. It can't be done without gcc's knowledge of when a fallthrough is happening (I presume you took that into account in your patch set.) -hpa ^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH] x86: pad assembly functions with INT3 2018-05-07 21:37 [PATCH] x86: pad assembly functions with INT3 Alexey Dobriyan 2018-05-07 21:41 ` hpa @ 2018-05-10 16:39 ` David Laight 2018-05-11 18:53 ` H. Peter Anvin 2018-05-14 12:53 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions tip-bot for Alexey Dobriyan 2 siblings, 1 reply; 34+ messages in thread From: David Laight @ 2018-05-10 16:39 UTC (permalink / raw) To: 'Alexey Dobriyan', tglx, mingo, hpa; +Cc: linux-kernel, x86 From: Alexey Dobriyan > Sent: 07 May 2018 22:38 > > Use INT3 instead of NOP. All that padding between functions is > an illegal area, no legitimate code should jump into it. > > I've checked x86_64 allyesconfig disassembly, all changes looks sane: > INT3 is only used after RET or unconditional JMP. I thought there was a performance penalty (on at least some cpu) depending on the number of and the actual instructions used for padding. I believe that is why gcc generates a small number of very long 'nop' instructions when padding code. David ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-10 16:39 ` David Laight @ 2018-05-11 18:53 ` H. Peter Anvin 2018-05-14 9:04 ` David Laight 0 siblings, 1 reply; 34+ messages in thread From: H. Peter Anvin @ 2018-05-11 18:53 UTC (permalink / raw) To: David Laight, 'Alexey Dobriyan', tglx, mingo; +Cc: linux-kernel, x86 On 05/10/18 09:39, David Laight wrote: > From: Alexey Dobriyan >> Sent: 07 May 2018 22:38 >> >> Use INT3 instead of NOP. All that padding between functions is >> an illegal area, no legitimate code should jump into it. >> >> I've checked x86_64 allyesconfig disassembly, all changes looks sane: >> INT3 is only used after RET or unconditional JMP. > > I thought there was a performance penalty (on at least some cpu) > depending on the number of and the actual instructions used for padding. > > I believe that is why gcc generates a small number of very long 'nop' > instructions when padding code. > There is a performance penalty for using NOP instructions *in the fallthrough case.* In the case where the padding is never supposed to be executed, which is what we're talking about here, it is irrelevant. I thought I had filed a gcc enhancement request, but I can't find it now, so I just filed this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85751 -hpa ^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH] x86: pad assembly functions with INT3 2018-05-11 18:53 ` H. Peter Anvin @ 2018-05-14 9:04 ` David Laight 2018-05-14 11:05 ` hpa 0 siblings, 1 reply; 34+ messages in thread From: David Laight @ 2018-05-14 9:04 UTC (permalink / raw) To: 'H. Peter Anvin', 'Alexey Dobriyan', tglx, mingo Cc: linux-kernel, x86 From: H. Peter Anvin > Sent: 11 May 2018 19:54 > > On 05/10/18 09:39, David Laight wrote: > > From: Alexey Dobriyan > >> Sent: 07 May 2018 22:38 > >> > >> Use INT3 instead of NOP. All that padding between functions is > >> an illegal area, no legitimate code should jump into it. > >> > >> I've checked x86_64 allyesconfig disassembly, all changes looks sane: > >> INT3 is only used after RET or unconditional JMP. > > > > I thought there was a performance penalty (on at least some cpu) > > depending on the number of and the actual instructions used for padding. > > > > I believe that is why gcc generates a small number of very long 'nop' > > instructions when padding code. > > > > There is a performance penalty for using NOP instructions *in the > fallthrough case.* In the case where the padding is never supposed to > be executed, which is what we're talking about here, it is irrelevant. Not completely irrelevant, the instructions stream gets fetched and decoded beyond the jmp/ret at the end of the function. At some point the cpu will decode the jmp/ret and fetch/decode from the target address. I guess it won't try to speculatively execute the 'pad' instructions - but you can never really tell! David ^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH] x86: pad assembly functions with INT3 2018-05-14 9:04 ` David Laight @ 2018-05-14 11:05 ` hpa 2018-05-15 6:54 ` Ingo Molnar 0 siblings, 1 reply; 34+ messages in thread From: hpa @ 2018-05-14 11:05 UTC (permalink / raw) To: David Laight, 'Alexey Dobriyan', tglx, mingo; +Cc: linux-kernel, x86 On May 14, 2018 2:04:38 AM PDT, David Laight <David.Laight@ACULAB.COM> wrote: >From: H. Peter Anvin >> Sent: 11 May 2018 19:54 >> >> On 05/10/18 09:39, David Laight wrote: >> > From: Alexey Dobriyan >> >> Sent: 07 May 2018 22:38 >> >> >> >> Use INT3 instead of NOP. All that padding between functions is >> >> an illegal area, no legitimate code should jump into it. >> >> >> >> I've checked x86_64 allyesconfig disassembly, all changes looks >sane: >> >> INT3 is only used after RET or unconditional JMP. >> > >> > I thought there was a performance penalty (on at least some cpu) >> > depending on the number of and the actual instructions used for >padding. >> > >> > I believe that is why gcc generates a small number of very long >'nop' >> > instructions when padding code. >> > >> >> There is a performance penalty for using NOP instructions *in the >> fallthrough case.* In the case where the padding is never supposed >to >> be executed, which is what we're talking about here, it is >irrelevant. > >Not completely irrelevant, the instructions stream gets fetched and >decoded >beyond the jmp/ret at the end of the function. >At some point the cpu will decode the jmp/ret and fetch/decode from the >target address. >I guess it won't try to speculatively execute the 'pad' instructions >- but you can never really tell! > > David The CPU doesn't speculate down past an unconditional control transfer. Doing so would be idiotic. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-14 11:05 ` hpa @ 2018-05-15 6:54 ` Ingo Molnar 2018-05-15 6:59 ` hpa 0 siblings, 1 reply; 34+ messages in thread From: Ingo Molnar @ 2018-05-15 6:54 UTC (permalink / raw) To: hpa Cc: David Laight, 'Alexey Dobriyan', tglx, mingo, linux-kernel, x86 * hpa@zytor.com <hpa@zytor.com> wrote: > > I guess it won't try to speculatively execute the 'pad' instructions - but you > > can never really tell! > > > > David > > The CPU doesn't speculate down past an unconditional control transfer. Doing so > would be idiotic. I think, when it comes to speculative execution, our general expectation that CPUs don't do idiotic things got somewhat weakened in the past year or so ... Thanks, Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86: pad assembly functions with INT3 2018-05-15 6:54 ` Ingo Molnar @ 2018-05-15 6:59 ` hpa 0 siblings, 0 replies; 34+ messages in thread From: hpa @ 2018-05-15 6:59 UTC (permalink / raw) To: Ingo Molnar Cc: David Laight, 'Alexey Dobriyan', tglx, mingo, linux-kernel, x86 On May 14, 2018 11:54:05 PM PDT, Ingo Molnar <mingo@kernel.org> wrote: > >* hpa@zytor.com <hpa@zytor.com> wrote: > >> > I guess it won't try to speculatively execute the 'pad' >instructions - but you >> > can never really tell! >> > >> > David >> >> The CPU doesn't speculate down past an unconditional control >transfer. Doing so >> would be idiotic. > >I think, when it comes to speculative execution, our general >expectation that CPUs >don't do idiotic things got somewhat weakened in the past year or so >... > >Thanks, > > Ingo Sort-of-kind-of... the things that have cropped up were reasonable consequences of designing under a set of assumptions which turned out to be incorrect. Speculating through an unconditional control transfer did not make sense under those assumptions either. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 34+ messages in thread
* [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-05-07 21:37 [PATCH] x86: pad assembly functions with INT3 Alexey Dobriyan 2018-05-07 21:41 ` hpa 2018-05-10 16:39 ` David Laight @ 2018-05-14 12:53 ` tip-bot for Alexey Dobriyan 2018-06-17 11:40 ` Mike Galbraith 2 siblings, 1 reply; 34+ messages in thread From: tip-bot for Alexey Dobriyan @ 2018-05-14 12:53 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, tglx, luto, peterz, jpoimboe, adobriyan, torvalds, dvlasenk, linux-kernel, h.peter.anvin, bp, brgerst, hpa Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a Author: Alexey Dobriyan <adobriyan@gmail.com> AuthorDate: Tue, 8 May 2018 00:37:55 +0300 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Mon, 14 May 2018 11:43:03 +0200 x86/asm: Pad assembly functions with INT3 instructions Use INT3 instead of NOP. All that padding between functions is an illegal area, no legitimate code should jump into it. I've checked x86_64 allyesconfig disassembly, all changes looks sane: INT3 is only used after RET or unconditional JMP. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> Acked-by: H. Peter Anvin <h.peter.anvin@intel.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Borislav Petkov <bp@alien8.de> Cc: Brian Gerst <brgerst@gmail.com> Cc: Denys Vlasenko <dvlasenk@redhat.com> Cc: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Link: http://lkml.kernel.org/r/20180507213755.GA32406@avx2 Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/include/asm/linkage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/include/asm/linkage.h b/arch/x86/include/asm/linkage.h index 14caa9d9fb7f..c0b70bc1e659 100644 --- a/arch/x86/include/asm/linkage.h +++ b/arch/x86/include/asm/linkage.h @@ -18,7 +18,7 @@ name: #if defined(CONFIG_X86_64) || defined(CONFIG_X86_ALIGNMENT_16) -#define __ALIGN .p2align 4, 0x90 +#define __ALIGN .p2align 4, 0xCC #define __ALIGN_STR __stringify(__ALIGN) #endif ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-05-14 12:53 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions tip-bot for Alexey Dobriyan @ 2018-06-17 11:40 ` Mike Galbraith 2018-06-17 12:00 ` Borislav Petkov 0 siblings, 1 reply; 34+ messages in thread From: Mike Galbraith @ 2018-06-17 11:40 UTC (permalink / raw) To: Alexey Dobriyan Cc: torvalds, tglx, mingo, jpoimboe, adobriyan, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, bp, linux-tip-commits On Mon, 2018-05-14 at 05:53 -0700, tip-bot for Alexey Dobriyan wrote: > Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a > Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a > Author: Alexey Dobriyan <adobriyan@gmail.com> > AuthorDate: Tue, 8 May 2018 00:37:55 +0300 > Committer: Ingo Molnar <mingo@kernel.org> > CommitDate: Mon, 14 May 2018 11:43:03 +0200 > > x86/asm: Pad assembly functions with INT3 instructions > > Use INT3 instead of NOP. All that padding between functions is > an illegal area, no legitimate code should jump into it. Is dinky patchlet suggesting cryptomgr is being naughty? (revert silences spew, but..) ... [ 21.041608] int3: 0000 [#1] SMP PTI [ 21.041754] CPU: 3 PID: 935 Comm: cryptomgr_test Tainted: G E 4.17.0.g075a1d3-tip-default #146 [ 21.041888] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 [ 21.042035] RIP: 0010:crypto_aegis128_aesni_enc_tail+0x74/0x80 [aegis128_aesni] [ 21.042171] Code: 38 dc ca 66 0f 38 dc d3 66 0f 38 dc de 66 0f ef e5 f3 0f 7f 27 f3 0f 7f 47 10 f3 0f 7f 4f 20 f3 0f 7f 57 30 f3 0f 7f 5f 40 cc <cc> cc cc cc cc cc cc cc cc cc cc cc 48 83 fe 10 0f 82 c3 03 00 00 [ 21.042333] RSP: 0018:ffff963f81ee79b8 EFLAGS: 00000246 [ 21.042485] RAX: ffffffffc0985950 RBX: 0000000000000001 RCX: ffff8a3ab90d6000 [ 21.042640] RDX: ffff8a3ab90d6000 RSI: 0000000000000001 RDI: ffff963f81ee7af0 [ 21.042792] RBP: ffff963f81ee7a90 R08: 0000000000000001 R09: ffff8a3ab90d6000 [ 21.042953] R10: c1267690ad7d2d9e R11: 00000000ffffffe0 R12: ffff8a3ab90d6000 [ 21.043100] R13: ffffffffc0987040 R14: ffff963f81ee7af0 R15: ffff8a3ab90d6000 [ 21.043250] FS: 0000000000000000(0000) GS:ffff8a3adecc0000(0000) knlGS:0000000000000000 [ 21.043405] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 21.043554] CR2: 00007f2e169c4010 CR3: 00000001f700a005 CR4: 00000000001606e0 [ 21.043704] Call Trace: [ 21.043854] ? crypto_aegis128_aesni_process_crypt+0x8a/0xc0 [aegis128_aesni] [ 21.044004] ? crypto_aegis128_aesni_crypt+0x238/0x440 [aegis128_aesni] [ 21.044156] ? crypto_aegis128_aesni_crypt+0x238/0x440 [aegis128_aesni] [ 21.044311] ? crypto_aegis128_aesni_encrypt+0x62/0xb0 [aegis128_aesni] [ 21.044454] ? crypto_aegis128_aesni_encrypt+0x62/0xb0 [aegis128_aesni] [ 21.044597] ? crypto_aead_setauthsize+0x23/0x40 [ 21.044739] ? __test_aead+0x632/0x15d0 [ 21.044884] ? crypto_aegis128_aesni_crypt+0x440/0x440 [aegis128_aesni] [ 21.045026] ? __test_aead+0x632/0x15d0 [ 21.045167] ? crypto_alloc_tfm+0x52/0xf0 [ 21.045308] ? crypto_acomp_scomp_free_ctx+0x30/0x30 [ 21.045449] ? crypto_create_tfm+0x32/0xe0 [ 21.045594] ? crypto_acomp_scomp_free_ctx+0x30/0x30 [ 21.045734] ? crypto_acomp_scomp_free_ctx+0x30/0x30 [ 21.045877] ? test_aead+0x21/0xa0 [ 21.046015] ? alg_test_aead+0x3f/0xa0 [ 21.046154] ? alg_test.part.13+0x170/0x370 [ 21.046291] ? pick_next_task_fair+0x134/0x5d0 [ 21.046426] ? __switch_to+0x92/0x4b0 [ 21.046565] ? finish_task_switch+0x7f/0x2d0 [ 21.046701] ? __schedule+0x2b8/0x860 [ 21.046833] ? crypto_acomp_scomp_free_ctx+0x30/0x30 [ 21.046963] ? cryptomgr_test+0x40/0x50 [ 21.047092] ? kthread+0x11e/0x140 [ 21.047221] ? kthread_associate_blkcg+0xb0/0xb0 [ 21.047350] ? ret_from_fork+0x3a/0x50 [ 21.047478] Modules linked in: aegis128_aesni(E+) snd_timer(E) crct10dif_pclmul(E) r8169(E) snd(E) crc32_pclmul(E) mii(E) iTCO_wdt(E) ghash_clmulni_intel(E) iTCO_vendor_support(E) pcbc(E) gpio_ich(E) aesni_intel(E) soundcore(E) aes_x86_64(E) lpc_ich(E) crypto_simd(E) mei_me(E) cryptd(E) mfd_core(E) i2c_i801(E) mei(E) glue_helper(E) pcspkr(E) thermal(E) intel_smartconnect(E) fan(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) grace(E) sunrpc(E) sch_fq_codel(E) sr_mod(E) cdrom(E) hid_logitech_hidpp(E) hid_logitech_dj(E) uas(E) usb_storage(E) hid_generic(E) usbhid(E) nouveau(E) wmi(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) xhci_pci(E) sysfillrect(E) sysimgblt(E) fb_sys_fops(E) ahci(E) ttm(E) ehci_pci(E) libahci(E) xhci_hcd(E) ehci_hcd(E) libata(E) drm(E) usbcore(E) video(E) button(E) sd_mod(E) [ 21.048064] vfat(E) fat(E) virtio_blk(E) virtio_mmio(E) virtio_pci(E) virtio_ring(E) virtio(E) ext4(E) crc32c_intel(E) crc16(E) mbcache(E) jbd2(E) loop(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) efivarfs(E) [ 21.048396] Dumping ftrace buffer: [ 21.048556] (ftrace buffer empty) [ 21.048726] ---[ end trace 8cdd2dd0a107e807 ]--- [ 21.048901] RIP: 0010:crypto_aegis128_aesni_enc_tail+0x74/0x80 [aegis128_aesni] [ 21.049051] Code: 38 dc ca 66 0f 38 dc d3 66 0f 38 dc de 66 0f ef e5 f3 0f 7f 27 f3 0f 7f 47 10 f3 0f 7f 4f 20 f3 0f 7f 57 30 f3 0f 7f 5f 40 cc <cc> cc cc cc cc cc cc cc cc cc cc cc 48 83 fe 10 0f 82 c3 03 00 00 [ 21.049224] RSP: 0018:ffff963f81ee79b8 EFLAGS: 00000246 [ 21.049390] RAX: ffffffffc0985950 RBX: 0000000000000001 RCX: ffff8a3ab90d6000 [ 21.049579] RDX: ffff8a3ab90d6000 RSI: 0000000000000001 RDI: ffff963f81ee7af0 [ 21.049782] RBP: ffff963f81ee7a90 R08: 0000000000000001 R09: ffff8a3ab90d6000 [ 21.049978] R10: c1267690ad7d2d9e R11: 00000000ffffffe0 R12: ffff8a3ab90d6000 [ 21.050179] R13: ffffffffc0987040 R14: ffff963f81ee7af0 R15: ffff8a3ab90d6000 [ 21.050377] FS: 0000000000000000(0000) GS:ffff8a3adecc0000(0000) knlGS:0000000000000000 [ 21.050579] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 21.050777] CR2: 00007f2e169c4010 CR3: 00000001f700a005 CR4: 00000000001606e0 [ 21.050981] BUG: sleeping function called from invalid context at ./include/linux/percpu-rwsem.h:34 [ 21.051183] in_atomic(): 1, irqs_disabled(): 0, pid: 935, name: cryptomgr_test [ 21.051390] CPU: 3 PID: 935 Comm: cryptomgr_test Tainted: G D E 4.17.0.g075a1d3-tip-default #146 [ 21.051592] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 [ 21.051799] Call Trace: [ 21.052002] dump_stack+0x85/0xcb [ 21.052207] ___might_sleep+0xd8/0x130 [ 21.052412] exit_signals+0x21/0x1c0 [ 21.052612] do_exit+0xa0/0xb60 [ 21.052808] ? cryptomgr_test+0x40/0x50 [ 21.052999] ? kthread+0x11e/0x140 [ 21.053176] rewind_stack_do_exit+0x17/0x20 [ 21.053354] note: cryptomgr_test[935] exited with preempt_count 2 ... [ 200.214958] WARNING: CPU: 7 PID: 601 at crypto/algapi.c:369 crypto_wait_for_test+0x4c/0x60 [ 200.214960] Modules linked in: fuse(E) devlink(E) ebtable_filter(E) ebtables(E) xt_comment(E) xt_physdev(E) br_netfilter(E) nfnetlink_cthelper(E) nfnetlink(E) af_packet(E) bridge(E) stp(E) llc(E) iscsi_ibft(E) iscsi_boot_sysfs(E) msr(E) ip6t_REJECT(E) nf_conntrack_ipv6(E) nf_defrag_ipv6(E) ipt_REJECT(E) xt_pkttype(E) xt_tcpudp(E) iptable_filter(E) bpfilter(E) ip6table_mangle(E) nf_conntrack_netbios_ns(E) nf_conntrack_broadcast(E) nf_conntrack_ipv4(E) nf_defrag_ipv4(E) ip_tables(E) xt_conntrack(E) nf_conntrack(E) libcrc32c(E) ip6table_filter(E) ip6_tables(E) x_tables(E) nls_iso8859_1(E) nls_cp437(E) joydev(E) snd_hda_codec_hdmi(E) snd_hda_codec_realtek(E) snd_hda_codec_generic(E) snd_hda_intel(E) snd_hda_codec(E) snd_hda_core(E) snd_hwdep(E) snd_pcm(E) aegis128_aesni(E+) snd_timer(E) crct10dif_pclmul(E) [ 200.215086] r8169(E) snd(E) crc32_pclmul(E) mii(E) iTCO_wdt(E) ghash_clmulni_intel(E) iTCO_vendor_support(E) pcbc(E) gpio_ich(E) aesni_intel(E) soundcore(E) aes_x86_64(E) lpc_ich(E) crypto_simd(E) mei_me(E) cryptd(E) mfd_core(E) i2c_i801(E) mei(E) glue_helper(E) pcspkr(E) thermal(E) intel_smartconnect(E) fan(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) grace(E) sunrpc(E) sch_fq_codel(E) sr_mod(E) cdrom(E) hid_logitech_hidpp(E) hid_logitech_dj(E) uas(E) usb_storage(E) hid_generic(E) usbhid(E) nouveau(E) wmi(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) xhci_pci(E) sysfillrect(E) sysimgblt(E) fb_sys_fops(E) ahci(E) ttm(E) ehci_pci(E) libahci(E) xhci_hcd(E) ehci_hcd(E) libata(E) drm(E) usbcore(E) video(E) button(E) sd_mod(E) vfat(E) fat(E) virtio_blk(E) virtio_mmio(E) virtio_pci(E) virtio_ring(E) [ 200.215188] virtio(E) ext4(E) crc32c_intel(E) crc16(E) mbcache(E) jbd2(E) loop(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) efivarfs(E) [ 200.215216] CPU: 7 PID: 601 Comm: systemd-udevd Kdump: loaded Tainted: G D W E 4.17.0.g075a1d3-tip-default #146 [ 200.215222] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 [ 200.215230] RIP: 0010:crypto_wait_for_test+0x4c/0x60 [ 200.215234] Code: c0 75 2b 48 8d bb b8 00 00 00 31 f6 e8 2d fe ff ff 48 8d bb a8 01 00 00 e8 61 13 40 00 85 c0 75 09 48 89 df 5b e9 54 e5 ff ff <0f> 0b eb f3 0f 0b eb ef 66 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f [ 200.215303] RSP: 0018:ffff963f826cfc88 EFLAGS: 00010286 [ 200.215310] RAX: 00000000fffffe00 RBX: ffff8a3ab18cb400 RCX: 0000000000000002 [ 200.215316] RDX: 0000000000000000 RSI: 000000009d980d40 RDI: ffff8a3ab18cb5b0 [ 200.215321] RBP: 0000000000000000 R08: 0000000000000000 R09: 000000000000024f [ 200.215327] R10: 0000000000000355 R11: 00000000003d0900 R12: 0000000000000000 [ 200.215333] R13: ffffffffc0988000 R14: 0000000000000002 R15: ffff8a3ab02a7f80 [ 200.215340] FS: 00007fe89d980d40(0000) GS:ffff8a3adedc0000(0000) knlGS:0000000000000000 [ 200.215346] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 200.215351] CR2: 00007f83fc010e08 CR3: 00000003f1abe006 CR4: 00000000001606e0 [ 200.215356] Call Trace: [ 200.215367] crypto_register_alg+0x52/0x60 [ 200.215376] crypto_register_aeads+0x35/0xa0 [ 200.215383] ? 0xffffffffc0325000 [ 200.215391] do_one_initcall+0x46/0x1e9 [ 200.215400] ? __vunmap+0x76/0xb0 [ 200.215408] do_init_module+0x5b/0x203 [ 200.215415] load_module+0x19d3/0x1f50 [ 200.215422] ? __do_sys_finit_module+0xb7/0xd0 [ 200.215427] __do_sys_finit_module+0xb7/0xd0 [ 200.215433] do_syscall_64+0x60/0x180 [ 200.215438] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 200.215442] RIP: 0033:0x7fe89c807139 [ 200.215444] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 2f 0d 2c 00 f7 d8 64 89 01 48 [ 200.215528] RSP: 002b:00007fff4d130458 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 200.215549] RAX: ffffffffffffffda RBX: 000055b492f18880 RCX: 00007fe89c807139 [ 200.215551] RDX: 0000000000000000 RSI: 00007fe89d14383d RDI: 0000000000000016 [ 200.215554] RBP: 00007fe89d14383d R08: 0000000000000000 R09: 000055b492ecd480 [ 200.215581] R10: 0000000000000016 R11: 0000000000000246 R12: 0000000000020000 [ 200.215583] R13: 000055b492fa55e0 R14: 0000000000000000 R15: 0000000000000000 [ 200.215587] ---[ end trace 8cdd2dd0a107e808 ]--- ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 11:40 ` Mike Galbraith @ 2018-06-17 12:00 ` Borislav Petkov 2018-06-17 13:38 ` Mike Galbraith 0 siblings, 1 reply; 34+ messages in thread From: Borislav Petkov @ 2018-06-17 12:00 UTC (permalink / raw) To: Mike Galbraith Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, Jun 17, 2018 at 01:40:13PM +0200, Mike Galbraith wrote: > On Mon, 2018-05-14 at 05:53 -0700, tip-bot for Alexey Dobriyan wrote: > > Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a > > Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a > > Author: Alexey Dobriyan <adobriyan@gmail.com> > > AuthorDate: Tue, 8 May 2018 00:37:55 +0300 > > Committer: Ingo Molnar <mingo@kernel.org> > > CommitDate: Mon, 14 May 2018 11:43:03 +0200 > > > > x86/asm: Pad assembly functions with INT3 instructions > > > > Use INT3 instead of NOP. All that padding between functions is > > an illegal area, no legitimate code should jump into it. > > Is dinky patchlet suggesting cryptomgr is being naughty? > > (revert silences spew, but..) > > ... > [ 21.041608] int3: 0000 [#1] SMP PTI > [ 21.041754] CPU: 3 PID: 935 Comm: cryptomgr_test Tainted: G E 4.17.0.g075a1d3-tip-default #146 > [ 21.041888] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 > [ 21.042035] RIP: 0010:crypto_aegis128_aesni_enc_tail+0x74/0x80 [aegis128_aesni] > [ 21.042171] Code: 38 dc ca 66 0f 38 dc d3 66 0f 38 dc de 66 0f ef e5 f3 0f 7f 27 f3 0f 7f 47 10 f3 0f 7f 4f 20 f3 0f 7f 57 30 f3 0f 7f 5f 40 cc <cc> cc cc cc cc cc cc cc cc cc cc cc 48 83 fe 10 0f 82 c3 03 00 00 Looks like it misses a RET: --- From 6ac281ee69f4cb5b581d5f49662fb56b6326155a Mon Sep 17 00:00:00 2001 From: Borislav Petkov <bp@suse.de> Date: Sun, 17 Jun 2018 13:57:42 +0200 Subject: [PATCH] x86/crypto: Add a missing RET crypto_aegis128_aesni_enc_tail() needs to return too. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/crypto/aegis128-aesni-asm.S | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S index 9254e0b6cc06..717bf0776421 100644 --- a/arch/x86/crypto/aegis128-aesni-asm.S +++ b/arch/x86/crypto/aegis128-aesni-asm.S @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) movdqu STATE3, 0x40(STATEP) FRAME_END + ret ENDPROC(crypto_aegis128_aesni_enc_tail) .macro decrypt_block a s0 s1 s2 s3 s4 i -- 2.17.0.582.gccdcbd54c -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 12:00 ` Borislav Petkov @ 2018-06-17 13:38 ` Mike Galbraith 2018-06-17 14:02 ` Mike Galbraith 2018-06-19 11:27 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions David Laight 0 siblings, 2 replies; 34+ messages in thread From: Mike Galbraith @ 2018-06-17 13:38 UTC (permalink / raw) To: Borislav Petkov Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, 2018-06-17 at 14:00 +0200, Borislav Petkov wrote: > On Sun, Jun 17, 2018 at 01:40:13PM +0200, Mike Galbraith wrote: > > On Mon, 2018-05-14 at 05:53 -0700, tip-bot for Alexey Dobriyan wrote: > > > Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a > > > Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a > > > Author: Alexey Dobriyan <adobriyan@gmail.com> > > > AuthorDate: Tue, 8 May 2018 00:37:55 +0300 > > > Committer: Ingo Molnar <mingo@kernel.org> > > > CommitDate: Mon, 14 May 2018 11:43:03 +0200 > > > > > > x86/asm: Pad assembly functions with INT3 instructions > > > > > > Use INT3 instead of NOP. All that padding between functions is > > > an illegal area, no legitimate code should jump into it. > > > > Is dinky patchlet suggesting cryptomgr is being naughty? > > > > (revert silences spew, but..) > > > > ... > > [ 21.041608] int3: 0000 [#1] SMP PTI > > [ 21.041754] CPU: 3 PID: 935 Comm: cryptomgr_test Tainted: G E 4.17.0.g075a1d3-tip-default #146 > > [ 21.041888] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 > > [ 21.042035] RIP: 0010:crypto_aegis128_aesni_enc_tail+0x74/0x80 [aegis128_aesni] > > [ 21.042171] Code: 38 dc ca 66 0f 38 dc d3 66 0f 38 dc de 66 0f ef e5 f3 0f 7f 27 f3 0f 7f 47 10 f3 0f 7f 4f 20 f3 0f 7f 57 30 f3 0f 7f 5f 40 cc <cc> cc cc cc cc cc cc cc cc cc cc cc 48 83 fe 10 0f 82 c3 03 00 00 > > Looks like it misses a RET: Bingo. [ 28.751069] RIP: 0010:crypto_aegis128l_aesni_enc_tail+0xcd/0xd0 [aegis128l_aesni] Next next next.. > --- > From 6ac281ee69f4cb5b581d5f49662fb56b6326155a Mon Sep 17 00:00:00 2001 > From: Borislav Petkov <bp@suse.de> > Date: Sun, 17 Jun 2018 13:57:42 +0200 > Subject: [PATCH] x86/crypto: Add a missing RET > > crypto_aegis128_aesni_enc_tail() needs to return too. > > Signed-off-by: Borislav Petkov <bp@suse.de> > --- > arch/x86/crypto/aegis128-aesni-asm.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S > index 9254e0b6cc06..717bf0776421 100644 > --- a/arch/x86/crypto/aegis128-aesni-asm.S > +++ b/arch/x86/crypto/aegis128-aesni-asm.S > @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) > movdqu STATE3, 0x40(STATEP) > > FRAME_END > + ret > ENDPROC(crypto_aegis128_aesni_enc_tail) > > .macro decrypt_block a s0 s1 s2 s3 s4 i > -- > 2.17.0.582.gccdcbd54c > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 13:38 ` Mike Galbraith @ 2018-06-17 14:02 ` Mike Galbraith 2018-06-17 19:47 ` Borislav Petkov 2018-06-19 11:27 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions David Laight 1 sibling, 1 reply; 34+ messages in thread From: Mike Galbraith @ 2018-06-17 14:02 UTC (permalink / raw) To: Borislav Petkov Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, 2018-06-17 at 15:38 +0200, Mike Galbraith wrote: > On Sun, 2018-06-17 at 14:00 +0200, Borislav Petkov wrote: > > On Sun, Jun 17, 2018 at 01:40:13PM +0200, Mike Galbraith wrote: > > > On Mon, 2018-05-14 at 05:53 -0700, tip-bot for Alexey Dobriyan wrote: > > > > Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a > > > > Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a > > > > Author: Alexey Dobriyan <adobriyan@gmail.com> > > > > AuthorDate: Tue, 8 May 2018 00:37:55 +0300 > > > > Committer: Ingo Molnar <mingo@kernel.org> > > > > CommitDate: Mon, 14 May 2018 11:43:03 +0200 > > > > > > > > x86/asm: Pad assembly functions with INT3 instructions > > > > > > > > Use INT3 instead of NOP. All that padding between functions is > > > > an illegal area, no legitimate code should jump into it. > > > > > > Is dinky patchlet suggesting cryptomgr is being naughty? > > > > > > (revert silences spew, but..) > > > > > > ... > > > [ 21.041608] int3: 0000 [#1] SMP PTI > > > [ 21.041754] CPU: 3 PID: 935 Comm: cryptomgr_test Tainted: G E 4.17.0.g075a1d3-tip-default #146 > > > [ 21.041888] Hardware name: MEDION MS-7848/MS-7848, BIOS M7848W08.20C 09/23/2013 > > > [ 21.042035] RIP: 0010:crypto_aegis128_aesni_enc_tail+0x74/0x80 [aegis128_aesni] > > > [ 21.042171] Code: 38 dc ca 66 0f 38 dc d3 66 0f 38 dc de 66 0f ef e5 f3 0f 7f 27 f3 0f 7f 47 10 f3 0f 7f 4f 20 f3 0f 7f 57 30 f3 0f 7f 5f 40 cc <cc> cc cc cc cc cc cc cc cc cc cc cc 48 83 fe 10 0f 82 c3 03 00 00 > > > > Looks like it misses a RET: > > Bingo. > > [ 28.751069] RIP: 0010:crypto_aegis128l_aesni_enc_tail+0xcd/0xd0 [aegis128l_aesni] > > Next next next.. (/me does that.. all better) From 6ac281ee69f4cb5b581d5f49662fb56b6326155a Mon Sep 17 00:00:00 2001 From: Borislav Petkov <bp@suse.de> Date: Sun, 17 Jun 2018 13:57:42 +0200 Subject: [PATCH] x86/crypto: Add a missing RET crypto_aegis128_aesni_enc_tail() needs to return too. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/crypto/aegis128-aesni-asm.S | 1 + arch/x86/crypto/aegis128l-aesni-asm.S | 1 + arch/x86/crypto/aegis256-aesni-asm.S | 1 + arch/x86/crypto/morus1280-avx2-asm.S | 1 + arch/x86/crypto/morus1280-sse2-asm.S | 1 + arch/x86/crypto/morus640-sse2-asm.S | 1 + 6 files changed, 6 insertions(+) --- a/arch/x86/crypto/aegis128-aesni-asm.S +++ b/arch/x86/crypto/aegis128-aesni-asm.S @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) movdqu STATE3, 0x40(STATEP) FRAME_END + ret ENDPROC(crypto_aegis128_aesni_enc_tail) .macro decrypt_block a s0 s1 s2 s3 s4 i --- a/arch/x86/crypto/aegis128l-aesni-asm.S +++ b/arch/x86/crypto/aegis128l-aesni-asm.S @@ -645,6 +645,7 @@ ENTRY(crypto_aegis128l_aesni_enc_tail) state_store0 FRAME_END + ret ENDPROC(crypto_aegis128l_aesni_enc_tail) /* --- a/arch/x86/crypto/aegis256-aesni-asm.S +++ b/arch/x86/crypto/aegis256-aesni-asm.S @@ -543,6 +543,7 @@ ENTRY(crypto_aegis256_aesni_enc_tail) state_store0 FRAME_END + ret ENDPROC(crypto_aegis256_aesni_enc_tail) /* --- a/arch/x86/crypto/morus1280-avx2-asm.S +++ b/arch/x86/crypto/morus1280-avx2-asm.S @@ -453,6 +453,7 @@ ENTRY(crypto_morus1280_avx2_enc_tail) vmovdqu STATE4, (4 * 32)(%rdi) FRAME_END + ret ENDPROC(crypto_morus1280_avx2_enc_tail) /* --- a/arch/x86/crypto/morus1280-sse2-asm.S +++ b/arch/x86/crypto/morus1280-sse2-asm.S @@ -652,6 +652,7 @@ ENTRY(crypto_morus1280_sse2_enc_tail) movdqu STATE4_HI, (9 * 16)(%rdi) FRAME_END + ret ENDPROC(crypto_morus1280_sse2_enc_tail) /* --- a/arch/x86/crypto/morus640-sse2-asm.S +++ b/arch/x86/crypto/morus640-sse2-asm.S @@ -437,6 +437,7 @@ ENTRY(crypto_morus640_sse2_enc_tail) movdqu STATE4, (4 * 16)(%rdi) FRAME_END + ret ENDPROC(crypto_morus640_sse2_enc_tail) /* ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 14:02 ` Mike Galbraith @ 2018-06-17 19:47 ` Borislav Petkov 2018-06-18 2:34 ` Mike Galbraith 0 siblings, 1 reply; 34+ messages in thread From: Borislav Petkov @ 2018-06-17 19:47 UTC (permalink / raw) To: Mike Galbraith Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, Jun 17, 2018 at 04:02:58PM +0200, Mike Galbraith wrote: > (/me does that.. all better) > > From 6ac281ee69f4cb5b581d5f49662fb56b6326155a Mon Sep 17 00:00:00 2001 > From: Borislav Petkov <bp@suse.de> > Date: Sun, 17 Jun 2018 13:57:42 +0200 > Subject: [PATCH] x86/crypto: Add a missing RET > > crypto_aegis128_aesni_enc_tail() needs to return too. > > Signed-off-by: Borislav Petkov <bp@suse.de> [ Mike: took care of the other tail calls. ] Signed-off-by: Mike Galbraith <efault@gmx.de> > --- > arch/x86/crypto/aegis128-aesni-asm.S | 1 + > arch/x86/crypto/aegis128l-aesni-asm.S | 1 + > arch/x86/crypto/aegis256-aesni-asm.S | 1 + > arch/x86/crypto/morus1280-avx2-asm.S | 1 + > arch/x86/crypto/morus1280-sse2-asm.S | 1 + > arch/x86/crypto/morus640-sse2-asm.S | 1 + > 6 files changed, 6 insertions(+) ... -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 19:47 ` Borislav Petkov @ 2018-06-18 2:34 ` Mike Galbraith 2018-06-23 10:36 ` [PATCH] x86/crypto: Add missing RETs Borislav Petkov 0 siblings, 1 reply; 34+ messages in thread From: Mike Galbraith @ 2018-06-18 2:34 UTC (permalink / raw) To: Borislav Petkov Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, 2018-06-17 at 21:47 +0200, Borislav Petkov wrote: > On Sun, Jun 17, 2018 at 04:02:58PM +0200, Mike Galbraith wrote: > > (/me does that.. all better) > > > > From 6ac281ee69f4cb5b581d5f49662fb56b6326155a Mon Sep 17 00:00:00 2001 > > From: Borislav Petkov <bp@suse.de> > > Date: Sun, 17 Jun 2018 13:57:42 +0200 > > Subject: [PATCH] x86/crypto: Add a missing RET > > > > crypto_aegis128_aesni_enc_tail() needs to return too. > > > > Signed-off-by: Borislav Petkov <bp@suse.de> > > [ Mike: took care of the other tail calls. ] > Signed-off-by: Mike Galbraith <efault@gmx.de> I didn't think a sign-off was needed... it was your brain that wiggled my fingers after all ;-) -Mike ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] x86/crypto: Add missing RETs 2018-06-18 2:34 ` Mike Galbraith @ 2018-06-23 10:36 ` Borislav Petkov 2018-06-23 17:30 ` Ondrej Mosnáček ` (2 more replies) 0 siblings, 3 replies; 34+ messages in thread From: Borislav Petkov @ 2018-06-23 10:36 UTC (permalink / raw) To: linux-crypto Cc: Mike Galbraith, Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu Lemme send a proper patch now... --- From: Borislav Petkov <bp@suse.de> Date: Sun, 17 Jun 2018 13:57:42 +0200 Subject: [PATCH] x86/crypto: Add missing RETs Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms otherwise they run into INT3 padding due to 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") leading to spurious debug exceptions. Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/crypto/aegis128-aesni-asm.S | 1 + arch/x86/crypto/aegis128l-aesni-asm.S | 1 + arch/x86/crypto/aegis256-aesni-asm.S | 1 + arch/x86/crypto/morus1280-avx2-asm.S | 1 + arch/x86/crypto/morus1280-sse2-asm.S | 1 + arch/x86/crypto/morus640-sse2-asm.S | 1 + 6 files changed, 6 insertions(+) diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S index 9254e0b6cc06..717bf0776421 100644 --- a/arch/x86/crypto/aegis128-aesni-asm.S +++ b/arch/x86/crypto/aegis128-aesni-asm.S @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) movdqu STATE3, 0x40(STATEP) FRAME_END + ret ENDPROC(crypto_aegis128_aesni_enc_tail) .macro decrypt_block a s0 s1 s2 s3 s4 i diff --git a/arch/x86/crypto/aegis128l-aesni-asm.S b/arch/x86/crypto/aegis128l-aesni-asm.S index 9263c344f2c7..4eda2b8db9e1 100644 --- a/arch/x86/crypto/aegis128l-aesni-asm.S +++ b/arch/x86/crypto/aegis128l-aesni-asm.S @@ -645,6 +645,7 @@ ENTRY(crypto_aegis128l_aesni_enc_tail) state_store0 FRAME_END + ret ENDPROC(crypto_aegis128l_aesni_enc_tail) /* diff --git a/arch/x86/crypto/aegis256-aesni-asm.S b/arch/x86/crypto/aegis256-aesni-asm.S index 1d977d515bf9..32aae8397268 100644 --- a/arch/x86/crypto/aegis256-aesni-asm.S +++ b/arch/x86/crypto/aegis256-aesni-asm.S @@ -543,6 +543,7 @@ ENTRY(crypto_aegis256_aesni_enc_tail) state_store0 FRAME_END + ret ENDPROC(crypto_aegis256_aesni_enc_tail) /* diff --git a/arch/x86/crypto/morus1280-avx2-asm.S b/arch/x86/crypto/morus1280-avx2-asm.S index 37d422e77931..07653d4582a6 100644 --- a/arch/x86/crypto/morus1280-avx2-asm.S +++ b/arch/x86/crypto/morus1280-avx2-asm.S @@ -453,6 +453,7 @@ ENTRY(crypto_morus1280_avx2_enc_tail) vmovdqu STATE4, (4 * 32)(%rdi) FRAME_END + ret ENDPROC(crypto_morus1280_avx2_enc_tail) /* diff --git a/arch/x86/crypto/morus1280-sse2-asm.S b/arch/x86/crypto/morus1280-sse2-asm.S index 1fe637c7be9d..bd1aa1b60869 100644 --- a/arch/x86/crypto/morus1280-sse2-asm.S +++ b/arch/x86/crypto/morus1280-sse2-asm.S @@ -652,6 +652,7 @@ ENTRY(crypto_morus1280_sse2_enc_tail) movdqu STATE4_HI, (9 * 16)(%rdi) FRAME_END + ret ENDPROC(crypto_morus1280_sse2_enc_tail) /* diff --git a/arch/x86/crypto/morus640-sse2-asm.S b/arch/x86/crypto/morus640-sse2-asm.S index 71c72a0a0862..efa02816d921 100644 --- a/arch/x86/crypto/morus640-sse2-asm.S +++ b/arch/x86/crypto/morus640-sse2-asm.S @@ -437,6 +437,7 @@ ENTRY(crypto_morus640_sse2_enc_tail) movdqu STATE4, (4 * 16)(%rdi) FRAME_END + ret ENDPROC(crypto_morus640_sse2_enc_tail) /* -- 2.17.0.582.gccdcbd54c -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-23 10:36 ` [PATCH] x86/crypto: Add missing RETs Borislav Petkov @ 2018-06-23 17:30 ` Ondrej Mosnáček 2018-06-24 7:11 ` Ingo Molnar 2018-07-01 13:19 ` Herbert Xu 2 siblings, 0 replies; 34+ messages in thread From: Ondrej Mosnáček @ 2018-06-23 17:30 UTC (permalink / raw) To: bp Cc: linux-crypto, efault, adobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, Linux Kernel Mailing List, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu so 23. 6. 2018 o 12:36 Borislav Petkov <bp@alien8.de> napísal(a): > > Lemme send a proper patch now... > > --- > From: Borislav Petkov <bp@suse.de> > Date: Sun, 17 Jun 2018 13:57:42 +0200 > Subject: [PATCH] x86/crypto: Add missing RETs > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > otherwise they run into INT3 padding due to > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > leading to spurious debug exceptions. > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > Signed-off-by: Borislav Petkov <bp@suse.de> Oh, thanks for fixing that! Acked-by: Ondrej Mosnacek <omosnacek@gmail.com> Cheers, Ondrej > --- > arch/x86/crypto/aegis128-aesni-asm.S | 1 + > arch/x86/crypto/aegis128l-aesni-asm.S | 1 + > arch/x86/crypto/aegis256-aesni-asm.S | 1 + > arch/x86/crypto/morus1280-avx2-asm.S | 1 + > arch/x86/crypto/morus1280-sse2-asm.S | 1 + > arch/x86/crypto/morus640-sse2-asm.S | 1 + > 6 files changed, 6 insertions(+) > > diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S > index 9254e0b6cc06..717bf0776421 100644 > --- a/arch/x86/crypto/aegis128-aesni-asm.S > +++ b/arch/x86/crypto/aegis128-aesni-asm.S > @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) > movdqu STATE3, 0x40(STATEP) > > FRAME_END > + ret > ENDPROC(crypto_aegis128_aesni_enc_tail) > > .macro decrypt_block a s0 s1 s2 s3 s4 i > diff --git a/arch/x86/crypto/aegis128l-aesni-asm.S b/arch/x86/crypto/aegis128l-aesni-asm.S > index 9263c344f2c7..4eda2b8db9e1 100644 > --- a/arch/x86/crypto/aegis128l-aesni-asm.S > +++ b/arch/x86/crypto/aegis128l-aesni-asm.S > @@ -645,6 +645,7 @@ ENTRY(crypto_aegis128l_aesni_enc_tail) > state_store0 > > FRAME_END > + ret > ENDPROC(crypto_aegis128l_aesni_enc_tail) > > /* > diff --git a/arch/x86/crypto/aegis256-aesni-asm.S b/arch/x86/crypto/aegis256-aesni-asm.S > index 1d977d515bf9..32aae8397268 100644 > --- a/arch/x86/crypto/aegis256-aesni-asm.S > +++ b/arch/x86/crypto/aegis256-aesni-asm.S > @@ -543,6 +543,7 @@ ENTRY(crypto_aegis256_aesni_enc_tail) > state_store0 > > FRAME_END > + ret > ENDPROC(crypto_aegis256_aesni_enc_tail) > > /* > diff --git a/arch/x86/crypto/morus1280-avx2-asm.S b/arch/x86/crypto/morus1280-avx2-asm.S > index 37d422e77931..07653d4582a6 100644 > --- a/arch/x86/crypto/morus1280-avx2-asm.S > +++ b/arch/x86/crypto/morus1280-avx2-asm.S > @@ -453,6 +453,7 @@ ENTRY(crypto_morus1280_avx2_enc_tail) > vmovdqu STATE4, (4 * 32)(%rdi) > > FRAME_END > + ret > ENDPROC(crypto_morus1280_avx2_enc_tail) > > /* > diff --git a/arch/x86/crypto/morus1280-sse2-asm.S b/arch/x86/crypto/morus1280-sse2-asm.S > index 1fe637c7be9d..bd1aa1b60869 100644 > --- a/arch/x86/crypto/morus1280-sse2-asm.S > +++ b/arch/x86/crypto/morus1280-sse2-asm.S > @@ -652,6 +652,7 @@ ENTRY(crypto_morus1280_sse2_enc_tail) > movdqu STATE4_HI, (9 * 16)(%rdi) > > FRAME_END > + ret > ENDPROC(crypto_morus1280_sse2_enc_tail) > > /* > diff --git a/arch/x86/crypto/morus640-sse2-asm.S b/arch/x86/crypto/morus640-sse2-asm.S > index 71c72a0a0862..efa02816d921 100644 > --- a/arch/x86/crypto/morus640-sse2-asm.S > +++ b/arch/x86/crypto/morus640-sse2-asm.S > @@ -437,6 +437,7 @@ ENTRY(crypto_morus640_sse2_enc_tail) > movdqu STATE4, (4 * 16)(%rdi) > > FRAME_END > + ret > ENDPROC(crypto_morus640_sse2_enc_tail) > > /* > -- > 2.17.0.582.gccdcbd54c > > -- > Regards/Gruss, > Boris. > > Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-23 10:36 ` [PATCH] x86/crypto: Add missing RETs Borislav Petkov 2018-06-23 17:30 ` Ondrej Mosnáček @ 2018-06-24 7:11 ` Ingo Molnar 2018-06-24 7:12 ` Thomas Gleixner 2018-06-24 10:44 ` Alexey Dobriyan 2018-07-01 13:19 ` Herbert Xu 2 siblings, 2 replies; 34+ messages in thread From: Ingo Molnar @ 2018-06-24 7:11 UTC (permalink / raw) To: Borislav Petkov Cc: linux-crypto, Mike Galbraith, Alexey Dobriyan, torvalds, tglx, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu * Borislav Petkov <bp@alien8.de> wrote: > Lemme send a proper patch now... > > --- > From: Borislav Petkov <bp@suse.de> > Date: Sun, 17 Jun 2018 13:57:42 +0200 > Subject: [PATCH] x86/crypto: Add missing RETs > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > otherwise they run into INT3 padding due to > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > leading to spurious debug exceptions. > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. Note that 51bad67ffbce has been zapped because it caused too many problems like this, but the explicit RETs make sense nevertheless. When applying the patch please don't include the SHA1 of the non-upstream (and probably never-upstream) commit. Thanks, Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-24 7:11 ` Ingo Molnar @ 2018-06-24 7:12 ` Thomas Gleixner 2018-06-24 10:15 ` Borislav Petkov 2018-06-24 10:44 ` Alexey Dobriyan 1 sibling, 1 reply; 34+ messages in thread From: Thomas Gleixner @ 2018-06-24 7:12 UTC (permalink / raw) To: Ingo Molnar Cc: Borislav Petkov, linux-crypto, Mike Galbraith, Alexey Dobriyan, torvalds, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu On Sun, 24 Jun 2018, Ingo Molnar wrote: > * Borislav Petkov <bp@alien8.de> wrote: > > > Lemme send a proper patch now... > > > > --- > > From: Borislav Petkov <bp@suse.de> > > Date: Sun, 17 Jun 2018 13:57:42 +0200 > > Subject: [PATCH] x86/crypto: Add missing RETs > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > otherwise they run into INT3 padding due to > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > leading to spurious debug exceptions. > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > Note that 51bad67ffbce has been zapped because it caused too many problems like > this, but the explicit RETs make sense nevertheless. When applying the patch > please don't include the SHA1 of the non-upstream (and probably never-upstream) > commit. We should really have something like that exactly to catch cases like this. Thanks, tglx ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-24 7:12 ` Thomas Gleixner @ 2018-06-24 10:15 ` Borislav Petkov 0 siblings, 0 replies; 34+ messages in thread From: Borislav Petkov @ 2018-06-24 10:15 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, linux-crypto, Mike Galbraith, Alexey Dobriyan, torvalds, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu On Sun, Jun 24, 2018 at 09:12:35AM +0200, Thomas Gleixner wrote: > We should really have something like that exactly to catch cases like this. Sounds like a good use case for the snake language. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-24 7:11 ` Ingo Molnar 2018-06-24 7:12 ` Thomas Gleixner @ 2018-06-24 10:44 ` Alexey Dobriyan 2018-06-25 7:24 ` Ingo Molnar 1 sibling, 1 reply; 34+ messages in thread From: Alexey Dobriyan @ 2018-06-24 10:44 UTC (permalink / raw) To: Ingo Molnar Cc: Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote: > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > otherwise they run into INT3 padding due to > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > leading to spurious debug exceptions. > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > Note that 51bad67ffbce has been zapped because it caused too many problems like > this, but the explicit RETs make sense nevertheless. So commit which found real bug(s) was zapped. OK ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-24 10:44 ` Alexey Dobriyan @ 2018-06-25 7:24 ` Ingo Molnar 2018-06-25 13:19 ` Josh Poimboeuf 0 siblings, 1 reply; 34+ messages in thread From: Ingo Molnar @ 2018-06-25 7:24 UTC (permalink / raw) To: Alexey Dobriyan Cc: Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra * Alexey Dobriyan <adobriyan@gmail.com> wrote: > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote: > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > > otherwise they run into INT3 padding due to > > > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > > > leading to spurious debug exceptions. > > > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > > > Note that 51bad67ffbce has been zapped because it caused too many problems like > > this, but the explicit RETs make sense nevertheless. > > So commit which found real bug(s) was zapped. > > OK No, what happened is that the commit was first moved into WIP.x86/debug showing its work-in-progress status, because it was incomplete and caused bugs: https://lore.kernel.org/lkml/20180518073644.GA8593@gmail.com/T/#u ... and finally, after weeks of inaction I zapped it because I didn't see progress and you didn't answer my question. If a fixed patch with updated tooling to detect these crashes before they occur on live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just incomplete in the current form. Thanks, Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-25 7:24 ` Ingo Molnar @ 2018-06-25 13:19 ` Josh Poimboeuf 2018-06-26 6:49 ` Ingo Molnar 0 siblings, 1 reply; 34+ messages in thread From: Josh Poimboeuf @ 2018-06-25 13:19 UTC (permalink / raw) To: Ingo Molnar Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote: > > * Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote: > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > > > otherwise they run into INT3 padding due to > > > > > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > > > > > leading to spurious debug exceptions. > > > > > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > > > > > Note that 51bad67ffbce has been zapped because it caused too many problems like > > > this, but the explicit RETs make sense nevertheless. > > > > So commit which found real bug(s) was zapped. > > > > OK > > No, what happened is that the commit was first moved into WIP.x86/debug showing > its work-in-progress status, because it was incomplete and caused bugs: > > https://lore.kernel.org/lkml/20180518073644.GA8593@gmail.com/T/#u > > ... and finally, after weeks of inaction I zapped it because I didn't see progress > and you didn't answer my question. > > If a fixed patch with updated tooling to detect these crashes before they occur on > live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just > incomplete in the current form. Hm, what happened to the objtool patch to detect these at build time? Did it not work? https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble -- Josh ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-25 13:19 ` Josh Poimboeuf @ 2018-06-26 6:49 ` Ingo Molnar 2018-06-26 12:31 ` Josh Poimboeuf 0 siblings, 1 reply; 34+ messages in thread From: Ingo Molnar @ 2018-06-26 6:49 UTC (permalink / raw) To: Josh Poimboeuf Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote: > > > > * Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > > > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote: > > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > > > > otherwise they run into INT3 padding due to > > > > > > > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > > > > > > > leading to spurious debug exceptions. > > > > > > > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > > > > > > > Note that 51bad67ffbce has been zapped because it caused too many problems like > > > > this, but the explicit RETs make sense nevertheless. > > > > > > So commit which found real bug(s) was zapped. > > > > > > OK > > > > No, what happened is that the commit was first moved into WIP.x86/debug showing > > its work-in-progress status, because it was incomplete and caused bugs: > > > > https://lore.kernel.org/lkml/20180518073644.GA8593@gmail.com/T/#u > > > > ... and finally, after weeks of inaction I zapped it because I didn't see progress > > and you didn't answer my question. > > > > If a fixed patch with updated tooling to detect these crashes before they occur on > > live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just > > incomplete in the current form. > > Hm, what happened to the objtool patch to detect these at build time? > Did it not work? > > https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble So that's still incomplete in that doesn't analyze the 32-bit build yet, right? Thanks, Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-26 6:49 ` Ingo Molnar @ 2018-06-26 12:31 ` Josh Poimboeuf 2018-07-05 7:58 ` Ingo Molnar 0 siblings, 1 reply; 34+ messages in thread From: Josh Poimboeuf @ 2018-06-26 12:31 UTC (permalink / raw) To: Ingo Molnar Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra On Tue, Jun 26, 2018 at 08:49:30AM +0200, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote: > > > > > > * Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > > > > > On Sun, Jun 24, 2018 at 09:11:05AM +0200, Ingo Molnar wrote: > > > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > > > > > otherwise they run into INT3 padding due to > > > > > > > > > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > > > > > > > > > leading to spurious debug exceptions. > > > > > > > > > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > > > > > > > > > Note that 51bad67ffbce has been zapped because it caused too many problems like > > > > > this, but the explicit RETs make sense nevertheless. > > > > > > > > So commit which found real bug(s) was zapped. > > > > > > > > OK > > > > > > No, what happened is that the commit was first moved into WIP.x86/debug showing > > > its work-in-progress status, because it was incomplete and caused bugs: > > > > > > https://lore.kernel.org/lkml/20180518073644.GA8593@gmail.com/T/#u > > > > > > ... and finally, after weeks of inaction I zapped it because I didn't see progress > > > and you didn't answer my question. > > > > > > If a fixed patch with updated tooling to detect these crashes before they occur on > > > live systems is submitted we'll reconsider - it didn't get NAK-ed, it's just > > > incomplete in the current form. > > > > Hm, what happened to the objtool patch to detect these at build time? > > Did it not work? > > > > https://lore.kernel.org/lkml/20180517134934.eog2fgoby5azq5a7@treble > > So that's still incomplete in that doesn't analyze the 32-bit build yet, right? We could do INT3s on 64-bit and NOPs on 32-bit. Or, possibly even better, we could just keep NOPs everywhere and instead make objtool smart enough to detect function fallthroughs. That should be pretty easy, actually. It already does it for C files. Something like the below should work, though it's still got a few issues: a) objtool is currently disabled for crypto code because it doesn't yet understand crypto stack re-alignments (which really needs fixing anyway); and b) it complains about the blank xen hypercalls falling through. Those aren't actual functions anyway, so we should probably annotate those somehow so that objtool ignores them anyway. I'm a bit swamped at the moment but I can fix those once I get a little more bandwidth. I at least verified that this patch caught the crypto missing RETs. diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile index a450ad573dcb..a2c52eec2863 100644 --- a/arch/x86/crypto/Makefile +++ b/arch/x86/crypto/Makefile @@ -3,8 +3,6 @@ # Arch-specific CryptoAPI modules. # -OBJECT_FILES_NON_STANDARD := y - avx_supported := $(call as-instr,vpxor %xmm0$(comma)%xmm0$(comma)%xmm0,yes,no) avx2_supported := $(call as-instr,vpgatherdd %ymm0$(comma)(%eax$(comma)%ymm1\ $(comma)4)$(comma)%ymm2,yes,no) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 2928939b98ec..f740fd828cba 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1798,13 +1798,14 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, while (1) { next_insn = next_insn_same_sec(file, insn); - if (file->c_file && func && insn->func && func != insn->func->pfunc) { + if (func && insn->func && func != insn->func->pfunc) { WARN("%s() falls through to next function %s()", func->name, insn->func->name); return 1; } - func = insn->func ? insn->func->pfunc : NULL; + if (insn->type != INSN_NOP) + func = insn->func ? insn->func->pfunc : NULL; if (func && insn->ignore) { WARN_FUNC("BUG: why am I validating an ignored function?", ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-26 12:31 ` Josh Poimboeuf @ 2018-07-05 7:58 ` Ingo Molnar 2018-07-06 14:06 ` Josh Poimboeuf 0 siblings, 1 reply; 34+ messages in thread From: Ingo Molnar @ 2018-07-05 7:58 UTC (permalink / raw) To: Josh Poimboeuf Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > So that's still incomplete in that doesn't analyze the 32-bit build yet, right? > > We could do INT3s on 64-bit and NOPs on 32-bit. > > Or, possibly even better, we could just keep NOPs everywhere and instead > make objtool smart enough to detect function fallthroughs. That should > be pretty easy, actually. It already does it for C files. > > Something like the below should work, though it's still got a few > issues: > > a) objtool is currently disabled for crypto code because it doesn't > yet understand crypto stack re-alignments (which really needs > fixing anyway); and > > b) it complains about the blank xen hypercalls falling through. Those > aren't actual functions anyway, so we should probably annotate > those somehow so that objtool ignores them anyway. > > I'm a bit swamped at the moment but I can fix those once I get a little > more bandwidth. I at least verified that this patch caught the crypto > missing RETs. Great, I'd be perfectly fine with such an approach. Also, if we have that then we could re-apply Alexey's patch and switch to INT3 (only on 64-bit kernels) without any trouble, because objtool should detect any execution flow bugs before the INT3 could trigger, right? I.e. any INT3 fault would show a combination of *both* an objtool bug and a probable code flow bug - which I suspect would warrant crashing the box ... Thanks, Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-07-05 7:58 ` Ingo Molnar @ 2018-07-06 14:06 ` Josh Poimboeuf 2018-07-06 14:57 ` Ingo Molnar 0 siblings, 1 reply; 34+ messages in thread From: Josh Poimboeuf @ 2018-07-06 14:06 UTC (permalink / raw) To: Ingo Molnar Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra On Thu, Jul 05, 2018 at 09:58:15AM +0200, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > So that's still incomplete in that doesn't analyze the 32-bit build yet, right? > > > > We could do INT3s on 64-bit and NOPs on 32-bit. > > > > Or, possibly even better, we could just keep NOPs everywhere and instead > > make objtool smart enough to detect function fallthroughs. That should > > be pretty easy, actually. It already does it for C files. > > > > Something like the below should work, though it's still got a few > > issues: > > > > a) objtool is currently disabled for crypto code because it doesn't > > yet understand crypto stack re-alignments (which really needs > > fixing anyway); and > > > > b) it complains about the blank xen hypercalls falling through. Those > > aren't actual functions anyway, so we should probably annotate > > those somehow so that objtool ignores them anyway. > > > > I'm a bit swamped at the moment but I can fix those once I get a little > > more bandwidth. I at least verified that this patch caught the crypto > > missing RETs. > > Great, I'd be perfectly fine with such an approach. > > Also, if we have that then we could re-apply Alexey's patch and switch to INT3 > (only on 64-bit kernels) without any trouble, because objtool should detect any > execution flow bugs before the INT3 could trigger, right? > > I.e. any INT3 fault would show a combination of *both* an objtool bug and a > probable code flow bug - which I suspect would warrant crashing the box ... Sounds good to me. I can take Alexey's patch and submit a 64-bit version of it, along with the relevant objtool changes (though it may still be a few weeks before I get the chance). -- Josh ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-07-06 14:06 ` Josh Poimboeuf @ 2018-07-06 14:57 ` Ingo Molnar 0 siblings, 0 replies; 34+ messages in thread From: Ingo Molnar @ 2018-07-06 14:57 UTC (permalink / raw) To: Josh Poimboeuf Cc: Alexey Dobriyan, Borislav Petkov, linux-crypto, Mike Galbraith, torvalds, tglx, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits, Herbert Xu, Peter Zijlstra * Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > Great, I'd be perfectly fine with such an approach. > > > > Also, if we have that then we could re-apply Alexey's patch and switch to INT3 > > (only on 64-bit kernels) without any trouble, because objtool should detect any > > execution flow bugs before the INT3 could trigger, right? > > > > I.e. any INT3 fault would show a combination of *both* an objtool bug and a > > probable code flow bug - which I suspect would warrant crashing the box ... > > Sounds good to me. I can take Alexey's patch and submit a 64-bit > version of it, along with the relevant objtool changes (though it may > still be a few weeks before I get the chance). Sounds good to me, thanks! Ingo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-06-23 10:36 ` [PATCH] x86/crypto: Add missing RETs Borislav Petkov 2018-06-23 17:30 ` Ondrej Mosnáček 2018-06-24 7:11 ` Ingo Molnar @ 2018-07-01 13:19 ` Herbert Xu 2018-07-01 15:24 ` Ondrej Mosnáček 2 siblings, 1 reply; 34+ messages in thread From: Herbert Xu @ 2018-07-01 13:19 UTC (permalink / raw) To: Borislav Petkov Cc: linux-crypto, Mike Galbraith, Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote: > Lemme send a proper patch now... > > --- > From: Borislav Petkov <bp@suse.de> > Date: Sun, 17 Jun 2018 13:57:42 +0200 > Subject: [PATCH] x86/crypto: Add missing RETs > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > otherwise they run into INT3 padding due to > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > leading to spurious debug exceptions. > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > Signed-off-by: Borislav Petkov <bp@suse.de> Patch applied. 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] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-07-01 13:19 ` Herbert Xu @ 2018-07-01 15:24 ` Ondrej Mosnáček 2018-07-01 15:45 ` Herbert Xu 0 siblings, 1 reply; 34+ messages in thread From: Ondrej Mosnáček @ 2018-07-01 15:24 UTC (permalink / raw) To: Herbert Xu Cc: bp, linux-crypto, efault, Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, Linux Kernel Mailing List, dvlasenk, h.peter.anvin, linux-tip-commits ne 1. 7. 2018 o 15:20 Herbert Xu <herbert@gondor.apana.org.au> napísal(a): > > On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote: > > Lemme send a proper patch now... > > > > --- > > From: Borislav Petkov <bp@suse.de> > > Date: Sun, 17 Jun 2018 13:57:42 +0200 > > Subject: [PATCH] x86/crypto: Add missing RETs > > > > Add explicit RETs to the tail calls of AEGIS and MORUS crypto algorithms > > otherwise they run into INT3 padding due to > > > > 51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions") > > > > leading to spurious debug exceptions. > > > > Mike Galbraith <efault@gmx.de> took care of all the remaining callsites. > > > > Signed-off-by: Borislav Petkov <bp@suse.de> > > Patch applied. Thanks. Hi Herbert, I can see you applied this patch to your cryptodev-2.6 tree (which I believe is for the next release). Shouldn't this go into the crypto-2.6 tree so it gets into 4.18-rcX? I'm not sure, but it seems to me that it qualifies as a (potentially serious?) bug fix. Also, I think you accidentally extracted the wrong part of the e-mail as the commit message... Thanks, Ondrej ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] x86/crypto: Add missing RETs 2018-07-01 15:24 ` Ondrej Mosnáček @ 2018-07-01 15:45 ` Herbert Xu 0 siblings, 0 replies; 34+ messages in thread From: Herbert Xu @ 2018-07-01 15:45 UTC (permalink / raw) To: Ondrej Mosnáček Cc: bp, linux-crypto, efault, Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, Linux Kernel Mailing List, dvlasenk, h.peter.anvin, linux-tip-commits On Sun, Jul 01, 2018 at 05:24:39PM +0200, Ondrej Mosnáček wrote: > > I can see you applied this patch to your cryptodev-2.6 tree (which I > believe is for the next release). Shouldn't this go into the > crypto-2.6 tree so it gets into 4.18-rcX? I'm not sure, but it seems > to me that it qualifies as a (potentially serious?) bug fix. > > Also, I think you accidentally extracted the wrong part of the e-mail > as the commit message... Thanks, it should all be fixed now. -- 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] 34+ messages in thread
* RE: [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions 2018-06-17 13:38 ` Mike Galbraith 2018-06-17 14:02 ` Mike Galbraith @ 2018-06-19 11:27 ` David Laight 1 sibling, 0 replies; 34+ messages in thread From: David Laight @ 2018-06-19 11:27 UTC (permalink / raw) To: 'Mike Galbraith', Borislav Petkov Cc: Alexey Dobriyan, torvalds, tglx, mingo, jpoimboe, luto, peterz, brgerst, hpa, linux-kernel, dvlasenk, h.peter.anvin, linux-tip-commits From: Mike Galbraith > Sent: 17 June 2018 14:39 ... > > > Is dinky patchlet suggesting cryptomgr is being naughty? ... > > diff --git a/arch/x86/crypto/aegis128-aesni-asm.S b/arch/x86/crypto/aegis128-aesni-asm.S > > index 9254e0b6cc06..717bf0776421 100644 > > --- a/arch/x86/crypto/aegis128-aesni-asm.S > > +++ b/arch/x86/crypto/aegis128-aesni-asm.S > > @@ -535,6 +535,7 @@ ENTRY(crypto_aegis128_aesni_enc_tail) > > movdqu STATE3, 0x40(STATEP) > > > > FRAME_END > > + ret > > ENDPROC(crypto_aegis128_aesni_enc_tail) ... How much of the rest of the associated crypyo changes were actually tested? I can't image the code DTRT without the 'ret'. David ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2018-07-06 14:57 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-07 21:37 [PATCH] x86: pad assembly functions with INT3 Alexey Dobriyan 2018-05-07 21:41 ` hpa 2018-05-09 16:55 ` Alexey Dobriyan 2018-05-09 19:28 ` H. Peter Anvin 2018-05-10 16:39 ` David Laight 2018-05-11 18:53 ` H. Peter Anvin 2018-05-14 9:04 ` David Laight 2018-05-14 11:05 ` hpa 2018-05-15 6:54 ` Ingo Molnar 2018-05-15 6:59 ` hpa 2018-05-14 12:53 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions tip-bot for Alexey Dobriyan 2018-06-17 11:40 ` Mike Galbraith 2018-06-17 12:00 ` Borislav Petkov 2018-06-17 13:38 ` Mike Galbraith 2018-06-17 14:02 ` Mike Galbraith 2018-06-17 19:47 ` Borislav Petkov 2018-06-18 2:34 ` Mike Galbraith 2018-06-23 10:36 ` [PATCH] x86/crypto: Add missing RETs Borislav Petkov 2018-06-23 17:30 ` Ondrej Mosnáček 2018-06-24 7:11 ` Ingo Molnar 2018-06-24 7:12 ` Thomas Gleixner 2018-06-24 10:15 ` Borislav Petkov 2018-06-24 10:44 ` Alexey Dobriyan 2018-06-25 7:24 ` Ingo Molnar 2018-06-25 13:19 ` Josh Poimboeuf 2018-06-26 6:49 ` Ingo Molnar 2018-06-26 12:31 ` Josh Poimboeuf 2018-07-05 7:58 ` Ingo Molnar 2018-07-06 14:06 ` Josh Poimboeuf 2018-07-06 14:57 ` Ingo Molnar 2018-07-01 13:19 ` Herbert Xu 2018-07-01 15:24 ` Ondrej Mosnáček 2018-07-01 15:45 ` Herbert Xu 2018-06-19 11:27 ` [tip:x86/pti] x86/asm: Pad assembly functions with INT3 instructions David Laight
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.