Re: [PATCH 22/23] x86/fpu: Don't restore the FPU state directly from userland in __fpu__restore_sig()
On 2018-11-08 10:25:24 [-0800], Andy Lutomirski wrote: > On Wed, Nov 7, 2018 at 11:49 AM Sebastian Andrzej Siewior > wrote: > > > > __fpu__restore_sig() restores the CPU's FPU state directly from > > userland. If we restore registers on return to userland then we can't > > load them directly from userland because a context switch/BH could > > destroy them. > > > > Restore the FPU registers after they have been copied from userland. > > __fpregs_changes_begin() ensures that they are not modified while beeing > > worked on. TIF_NEED_FPU_LOAD is clreared we want to keep our state, not > > the saved state. > > I'm conceptually okay with this change, but what happens if the > registers that are copied into the kernel are garbage? We used to > fail the restore and presumably kill the task. What happens now? What means garbage? Assume you mean something like memset(xstate, 0xff,) then this would happen: | [ cut here ] | Bad FPU state detected at __fpu__restore_sig+0x2a3/0x660, reinitializing FPU registers. | WARNING: CPU: 0 PID: 1687 at arch/x86/mm/extable.c:113 ex_handler_fprestore+0x60/0x70 | Modules linked in: | CPU: 0 PID: 1687 Comm: ssltc Not tainted 4.20.0-rc1+ #116 | Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.11.1-1 04/01/2014 | RIP: 0010:ex_handler_fprestore+0x60/0x70 | Code: 00 00 00 5d c3 48 0f ae 0d 0d 13 2c 01 b8 01 00 00 00 5d c3 48 89 c6 48 c7 c7 40 39 02 82 c6 05 c3 1d 2b 01 01 e8 0a 01 01 00 <0f> 0b eb b9 66 66 2e 0f 1f 84 00 00 7 | RSP: 0018:c9563cf8 EFLAGS: 00010282 | RAX: RBX: c9563d68 RCX: | RDX: 0046 RSI: RDI: 88003a1516c0 | RBP: c9563cf8 R08: R09: | R10: R11: R12: 000d | R13: R14: R15: | FS: 7f17678f1b80() GS:88003e80() knlGS: | CS: 0010 DS: ES: CR0: 80050033 | CR2: 7f1767dc2000 CR3: 3d0b6003 CR4: 00060ef0 | Call Trace: | fixup_exception+0x45/0x5c | do_general_protection+0x61/0x1a0 | general_protection+0x1e/0x30 | RIP: 0010:__fpu__restore_sig+0x2a3/0x660 | Code: 00 00 48 8b 95 48 ff ff ff 48 f7 d2 48 21 d0 0f 85 73 03 00 00 48 8b 85 48 ff ff ff 4c 89 f7 48 89 c2 48 c1 ea 20 48 0f ae 2f <65> 48 8b 04 25 00 4f 01 00 f0 80 60 e | RSP: 0018:c9563e18 EFLAGS: 00010246 | RAX: 0007 RBX: 7ffe7f19df40 RCX: 31d7 | RDX: RSI: 0200 RDI: 88003a152a40 | RBP: c9563ed0 R08: R09: | R10: 0001 R11: R12: | R13: 88003a1516c0 R14: 88003a152a40 R15: 88003a152a00 | ? __fpu__restore_sig+0x261/0x660 | ? trace_hardirqs_on+0x22/0x110 | fpu__restore_sig+0x28/0x40 | __ia32_sys_rt_sigreturn+0x218/0x2aa | do_syscall_64+0x50/0x1a0 | entry_SYSCALL_64_after_hwframe+0x49/0xbe | RIP: 0033:0x7f1767ca8a7b | Code: 31 d8 0f a4 ed 05 c5 79 7f 4c 24 30 01 fa 21 c6 c5 b9 72 d4 1f 31 d8 01 ea 0f ac ed 07 31 de c5 a9 73 fc 0c c5 d9 fe e4 89 d7 <03> 4c 24 08 31 c5 0f a4 d2 05 c4 c1 1 | RSP: 002b:7ffe7f19e340 EFLAGS: 0282 | RAX: 1c9f00ab RBX: 837732a0 RCX: 1bff4f26 | RDX: 37535a7c RSI: 979700a8 RDI: 37535a7c | RBP: 053caff3 R08: 5615ffd442a0 R09: 7f1767dc54c0 | R10: 7f1767dc6000 R11: 7ffe7f19e3c8 R12: 7f1767dc2000 | R13: 5615ff6c30a0 R14: 7f1767cab080 R15: 7f1767d95100 | irq event stamp: 12775 | hardirqs last enabled at (12774): [] vprintk_emit+0xac/0x270 | hardirqs last disabled at (12775): [] trace_hardirqs_off_thunk+0x1a/0x1c | softirqs last enabled at (12756): [] __do_softirq+0x3a2/0x4d1 | softirqs last disabled at (12760): [] __fpu__restore_sig+0x230/0x660 | ---[ end trace 163c456a84752e26 ]--- and the task continues. Before we had this: | BUG: GPF in non-whitelisted uaccess (non-canonical address?) | general protection fault: [#2] PREEMPT SMP DEBUG_PAGEALLOC PTI | CPU: 1 PID: 1687 Comm: ssltc Tainted: G D 4.20.0-rc1+ #120 | Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.11.1-1 04/01/2014 | RIP: 0010:__fpu__restore_sig+0x2fa/0x710 | Code: 34 8b 95 5c ff ff ff 85 d2 75 2a 4c 89 fa 48 f7 d2 48 21 d0 0f 85 b8 03 00 00 66 66 90 4c 89 fa 48 89 df 44 89 f8 48 c1 ea 20 <48> 0f ae 2f 31 db 66 66 90 eb 24 48 2 | RSP: 0018:c94abe20 EFLAGS: 00010246 | RAX: 0007 RBX: 7ffe5b4c1680 RCX: | RDX: RSI: 820544ad RDI: 7ffe5b4c1680 | RBP: c94abed0 R08: R09: 0001 | R10: R11: R12: 8800394440c0 | R13: 880039442d80 R14: 8800394440c0 R15: 0007 | FS: 7f81db2d4b80() GS:88003e88() knlGS: | CS: 0010 DS:
Re: [PATCH 22/23] x86/fpu: Don't restore the FPU state directly from userland in __fpu__restore_sig()
On 2018-11-08 10:25:24 [-0800], Andy Lutomirski wrote: > On Wed, Nov 7, 2018 at 11:49 AM Sebastian Andrzej Siewior > wrote: > > > > __fpu__restore_sig() restores the CPU's FPU state directly from > > userland. If we restore registers on return to userland then we can't > > load them directly from userland because a context switch/BH could > > destroy them. > > > > Restore the FPU registers after they have been copied from userland. > > __fpregs_changes_begin() ensures that they are not modified while beeing > > worked on. TIF_NEED_FPU_LOAD is clreared we want to keep our state, not > > the saved state. > > I'm conceptually okay with this change, but what happens if the > registers that are copied into the kernel are garbage? We used to > fail the restore and presumably kill the task. What happens now? What means garbage? Assume you mean something like memset(xstate, 0xff,) then this would happen: | [ cut here ] | Bad FPU state detected at __fpu__restore_sig+0x2a3/0x660, reinitializing FPU registers. | WARNING: CPU: 0 PID: 1687 at arch/x86/mm/extable.c:113 ex_handler_fprestore+0x60/0x70 | Modules linked in: | CPU: 0 PID: 1687 Comm: ssltc Not tainted 4.20.0-rc1+ #116 | Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.11.1-1 04/01/2014 | RIP: 0010:ex_handler_fprestore+0x60/0x70 | Code: 00 00 00 5d c3 48 0f ae 0d 0d 13 2c 01 b8 01 00 00 00 5d c3 48 89 c6 48 c7 c7 40 39 02 82 c6 05 c3 1d 2b 01 01 e8 0a 01 01 00 <0f> 0b eb b9 66 66 2e 0f 1f 84 00 00 7 | RSP: 0018:c9563cf8 EFLAGS: 00010282 | RAX: RBX: c9563d68 RCX: | RDX: 0046 RSI: RDI: 88003a1516c0 | RBP: c9563cf8 R08: R09: | R10: R11: R12: 000d | R13: R14: R15: | FS: 7f17678f1b80() GS:88003e80() knlGS: | CS: 0010 DS: ES: CR0: 80050033 | CR2: 7f1767dc2000 CR3: 3d0b6003 CR4: 00060ef0 | Call Trace: | fixup_exception+0x45/0x5c | do_general_protection+0x61/0x1a0 | general_protection+0x1e/0x30 | RIP: 0010:__fpu__restore_sig+0x2a3/0x660 | Code: 00 00 48 8b 95 48 ff ff ff 48 f7 d2 48 21 d0 0f 85 73 03 00 00 48 8b 85 48 ff ff ff 4c 89 f7 48 89 c2 48 c1 ea 20 48 0f ae 2f <65> 48 8b 04 25 00 4f 01 00 f0 80 60 e | RSP: 0018:c9563e18 EFLAGS: 00010246 | RAX: 0007 RBX: 7ffe7f19df40 RCX: 31d7 | RDX: RSI: 0200 RDI: 88003a152a40 | RBP: c9563ed0 R08: R09: | R10: 0001 R11: R12: | R13: 88003a1516c0 R14: 88003a152a40 R15: 88003a152a00 | ? __fpu__restore_sig+0x261/0x660 | ? trace_hardirqs_on+0x22/0x110 | fpu__restore_sig+0x28/0x40 | __ia32_sys_rt_sigreturn+0x218/0x2aa | do_syscall_64+0x50/0x1a0 | entry_SYSCALL_64_after_hwframe+0x49/0xbe | RIP: 0033:0x7f1767ca8a7b | Code: 31 d8 0f a4 ed 05 c5 79 7f 4c 24 30 01 fa 21 c6 c5 b9 72 d4 1f 31 d8 01 ea 0f ac ed 07 31 de c5 a9 73 fc 0c c5 d9 fe e4 89 d7 <03> 4c 24 08 31 c5 0f a4 d2 05 c4 c1 1 | RSP: 002b:7ffe7f19e340 EFLAGS: 0282 | RAX: 1c9f00ab RBX: 837732a0 RCX: 1bff4f26 | RDX: 37535a7c RSI: 979700a8 RDI: 37535a7c | RBP: 053caff3 R08: 5615ffd442a0 R09: 7f1767dc54c0 | R10: 7f1767dc6000 R11: 7ffe7f19e3c8 R12: 7f1767dc2000 | R13: 5615ff6c30a0 R14: 7f1767cab080 R15: 7f1767d95100 | irq event stamp: 12775 | hardirqs last enabled at (12774): [] vprintk_emit+0xac/0x270 | hardirqs last disabled at (12775): [] trace_hardirqs_off_thunk+0x1a/0x1c | softirqs last enabled at (12756): [] __do_softirq+0x3a2/0x4d1 | softirqs last disabled at (12760): [] __fpu__restore_sig+0x230/0x660 | ---[ end trace 163c456a84752e26 ]--- and the task continues. Before we had this: | BUG: GPF in non-whitelisted uaccess (non-canonical address?) | general protection fault: [#2] PREEMPT SMP DEBUG_PAGEALLOC PTI | CPU: 1 PID: 1687 Comm: ssltc Tainted: G D 4.20.0-rc1+ #120 | Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.11.1-1 04/01/2014 | RIP: 0010:__fpu__restore_sig+0x2fa/0x710 | Code: 34 8b 95 5c ff ff ff 85 d2 75 2a 4c 89 fa 48 f7 d2 48 21 d0 0f 85 b8 03 00 00 66 66 90 4c 89 fa 48 89 df 44 89 f8 48 c1 ea 20 <48> 0f ae 2f 31 db 66 66 90 eb 24 48 2 | RSP: 0018:c94abe20 EFLAGS: 00010246 | RAX: 0007 RBX: 7ffe5b4c1680 RCX: | RDX: RSI: 820544ad RDI: 7ffe5b4c1680 | RBP: c94abed0 R08: R09: 0001 | R10: R11: R12: 8800394440c0 | R13: 880039442d80 R14: 8800394440c0 R15: 0007 | FS: 7f81db2d4b80() GS:88003e88() knlGS: | CS: 0010 DS:
Re: [PATCH 22/23] x86/fpu: Don't restore the FPU state directly from userland in __fpu__restore_sig()
On Wed, Nov 7, 2018 at 11:49 AM Sebastian Andrzej Siewior wrote: > > __fpu__restore_sig() restores the CPU's FPU state directly from > userland. If we restore registers on return to userland then we can't > load them directly from userland because a context switch/BH could > destroy them. > > Restore the FPU registers after they have been copied from userland. > __fpregs_changes_begin() ensures that they are not modified while beeing > worked on. TIF_NEED_FPU_LOAD is clreared we want to keep our state, not > the saved state. I'm conceptually okay with this change, but what happens if the registers that are copied into the kernel are garbage? We used to fail the restore and presumably kill the task. What happens now?
Re: [PATCH 22/23] x86/fpu: Don't restore the FPU state directly from userland in __fpu__restore_sig()
On Wed, Nov 7, 2018 at 11:49 AM Sebastian Andrzej Siewior wrote: > > __fpu__restore_sig() restores the CPU's FPU state directly from > userland. If we restore registers on return to userland then we can't > load them directly from userland because a context switch/BH could > destroy them. > > Restore the FPU registers after they have been copied from userland. > __fpregs_changes_begin() ensures that they are not modified while beeing > worked on. TIF_NEED_FPU_LOAD is clreared we want to keep our state, not > the saved state. I'm conceptually okay with this change, but what happens if the registers that are copied into the kernel are garbage? We used to fail the restore and presumably kill the task. What happens now?
