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 <[email protected]>
---
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
On May 7, 2018 2:37:55 PM PDT, Alexey Dobriyan <[email protected]> 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 <[email protected]>
>---
>
> 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.
On Mon, May 07, 2018 at 02:41:14PM -0700, [email protected] 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.
On 05/09/18 09:55, Alexey Dobriyan wrote:
> On Mon, May 07, 2018 at 02:41:14PM -0700, [email protected] 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
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
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
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
On May 14, 2018 2:04:38 AM PDT, David Laight <[email protected]> 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.
Commit-ID: 51bad67ffbce0aaa44579f84ef5d05597054ec6a
Gitweb: https://git.kernel.org/tip/51bad67ffbce0aaa44579f84ef5d05597054ec6a
Author: Alexey Dobriyan <[email protected]>
AuthorDate: Tue, 8 May 2018 00:37:55 +0300
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: H. Peter Anvin <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/20180507213755.GA32406@avx2
Signed-off-by: Ingo Molnar <[email protected]>
---
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
* [email protected] <[email protected]> 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
On May 14, 2018 11:54:05 PM PDT, Ingo Molnar <[email protected]> wrote:
>
>* [email protected] <[email protected]> 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.
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 <[email protected]>
> AuthorDate: Tue, 8 May 2018 00:37:55 +0300
> Committer: Ingo Molnar <[email protected]>
> 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 ]---
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 <[email protected]>
> > AuthorDate: Tue, 8 May 2018 00:37:55 +0300
> > Committer: Ingo Molnar <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
---
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.
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 <[email protected]>
> > > AuthorDate: Tue, 8 May 2018 00:37:55 +0300
> > > Committer: Ingo Molnar <[email protected]>
> > > 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > > > AuthorDate: Tue, 8 May 2018 00:37:55 +0300
> > > > Committer: Ingo Molnar <[email protected]>
> > > > 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 <[email protected]>
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 <[email protected]>
---
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)
/*
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 <[email protected]>
> 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 <[email protected]>
[ Mike: took care of the other tail calls. ]
Signed-off-by: Mike Galbraith <[email protected]>
> ---
> 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.
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 <[email protected]>
> > 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 <[email protected]>
>
> [ Mike: took care of the other tail calls. ]
> Signed-off-by: Mike Galbraith <[email protected]>
I didn't think a sign-off was needed... it was your brain that wiggled
my fingers after all ;-)
-Mike
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
Lemme send a proper patch now...
---
From: Borislav Petkov <[email protected]>
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 <[email protected]> took care of all the remaining callsites.
Signed-off-by: Borislav Petkov <[email protected]>
---
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.
so 23. 6. 2018 o 12:36 Borislav Petkov <[email protected]> napísal(a):
>
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> 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 <[email protected]> took care of all the remaining callsites.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Oh, thanks for fixing that!
Acked-by: Ondrej Mosnacek <[email protected]>
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.
* Borislav Petkov <[email protected]> wrote:
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> 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 <[email protected]> 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
On Sun, 24 Jun 2018, Ingo Molnar wrote:
> * Borislav Petkov <[email protected]> wrote:
>
> > Lemme send a proper patch now...
> >
> > ---
> > From: Borislav Petkov <[email protected]>
> > 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 <[email protected]> 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
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.
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 <[email protected]> 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
* Alexey Dobriyan <[email protected]> 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 <[email protected]> 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/[email protected]/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
On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
>
> * Alexey Dobriyan <[email protected]> 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 <[email protected]> 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/[email protected]/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
* Josh Poimboeuf <[email protected]> wrote:
> On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> >
> > * Alexey Dobriyan <[email protected]> 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 <[email protected]> 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/[email protected]/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
On Tue, Jun 26, 2018 at 08:49:30AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > On Mon, Jun 25, 2018 at 09:24:38AM +0200, Ingo Molnar wrote:
> > >
> > > * Alexey Dobriyan <[email protected]> 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 <[email protected]> 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/[email protected]/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?",
On Sat, Jun 23, 2018 at 12:36:22PM +0200, Borislav Petkov wrote:
> Lemme send a proper patch now...
>
> ---
> From: Borislav Petkov <[email protected]>
> 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 <[email protected]> took care of all the remaining callsites.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
ne 1. 7. 2018 o 15:20 Herbert Xu <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> took care of all the remaining callsites.
> >
> > Signed-off-by: Borislav Petkov <[email protected]>
>
> 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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* Josh Poimboeuf <[email protected]> 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
On Thu, Jul 05, 2018 at 09:58:15AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> 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
* Josh Poimboeuf <[email protected]> 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