All of lore.kernel.org
 help / color / mirror / Atom feed
* [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
* [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: [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
* 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
* 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
* [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-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: [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
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.