Xen iopl fixes
Subsystems (4)
v4 cover Xen iopl fixes
Hi all-
For those who are seeing this for the first time: any 64-bit Xen PV domain with IO port access privileges (in practice, this means dom0 AFAIK) and any user programs that use iopl(3) (various old X drivers, presumably) is probably vulnerable to privilege escalations by unprivileged programs running in the same PV domain.
There's a long public description of the issue here:
http://xenbits.xen.org/xsa/advisory-171.html
Changes from v3:
- Add Jan's R-b.
- No longer embargoed
Changes from v2: Pretend v2 never happened...
Changes from v1: Use xen/hypervisor.h instead of xen-ops.h (Jan)
Andy Lutomirski (3):
selftests/x86: Add a iopl test
x86/iopl/64: Properly context-switch IOPL on Xen PV x86/iopl: Fix iopl capability check on Xen PV
arch/x86/include/asm/xen/hypervisor.h | 2 + arch/x86/kernel/ioport.c | 12 ++- arch/x86/kernel/process_64.c | 12 +++ arch/x86/xen/enlighten.c | 2 +- tools/testing/selftests/x86/Makefile | 2 +- tools/testing/selftests/x86/iopl.c | 135 ++++++++++++++++++++++++++++++++++ 6 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 tools/testing/selftests/x86/iopl.c
Signature · 1 line
2.5.0
PATCH v4 1/3 selftests/x86: Add a iopl test
This exercises two cases that are known to be buggy on Xen PV right now.
--- tools/testing/selftests/x86/Makefile | 2 +- tools/testing/selftests/x86/iopl.c | 135 +++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/x86/iopl.c
tools/testing/selftests/x86/Makefile +1 -1
@@ -4,7 +4,7 @@ include ../lib.mk .PHONY: all all_32 all_64 warn_32bit_failure clean -TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall +TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall iopl TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso \ test_FCMOV test_FCOMI test_FISTTP \ ldt_gdt \
tools/testing/selftests/x86/iopl.c new file +135 -0
135 lines not on this page · raw message
Signature · 1 line
2.5.0
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] selftests/x86: Add an iopl test
selftests/x86: Add an iopl test
Commit-ID: b08983015cdddca7e41c95f5054e2a8fb222a264 Gitweb: http://git.kernel.org/tip/b08983015cdddca7e41c95f5054e2a8fb222a264 Author: Andy Lutomirski <[email protected]> AuthorDate: Wed, 16 Mar 2016 14:14:20 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Mar 2016 09:49:26 +0100
selftests/x86: Add an iopl test
This exercises two cases that are known to be buggy on Xen PV right now.
--- tools/testing/selftests/x86/Makefile | 2 +- tools/testing/selftests/x86/iopl.c | 135 +++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-)
tools/testing/selftests/x86/Makefile +1 -1
@@ -5,7 +5,7 @@ include ../lib.mk .PHONY: all all_32 all_64 warn_32bit_failure clean TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall \ - check_initial_reg_state sigreturn ldt_gdt + check_initial_reg_state sigreturn ldt_gdt iopl TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \ test_FCMOV test_FCOMI test_FISTTP \ vdso_restorer
tools/testing/selftests/x86/iopl.c new file +135 -0
135 lines not on this page · raw message
PATCH v4 2/3 x86/iopl/64: Properly context-switch IOPL on Xen PV
In reply to Andy LutomirskiOn Xen PV, regs->flags doesn't reliably reflect IOPL and the exit-to-userspace code doesn't change IOPL. We need to context switch it manually.
I'm doing this without going through paravirt because this is specific to Xen PV. After the dust settles, we can merge this with the 32-bit code, tidy up the iopl syscall implementation, and remove the set_iopl pvop entirely.
Fixes XSA-171.
--- arch/x86/include/asm/xen/hypervisor.h | 2 ++ arch/x86/kernel/process_64.c | 12 ++++++++++++ arch/x86/xen/enlighten.c | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-)
arch/x86/include/asm/xen/hypervisor.h +2 -0
@@ -62,4 +62,6 @@ void xen_arch_register_cpu(int num);void xen_arch_unregister_cpu (int num);#endif +extern void xen_set_iopl_mask (unsigned mask); +#endif /* _ASM_X86_XEN_HYPERVISOR_H */
arch/x86/kernel/process_64.c +12 -0
@@ -48,6 +48,7 @@#include <asm/syscalls.h> #include <asm/debugreg.h> #include <asm/switch_to.h> +#include <asm/xen/hypervisor.h> asmlinkage extern void ret_from_fork(void);
@@ -411,6 +412,17 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))__switch_to_xtra (prev_p, next_p, tss); +#ifdef CONFIG_XEN +/* +* On Xen PV, IOPL bits in pt_regs->flags have no effect, and +* current_pt_regs()->flags may not match the current task's +* intended IOPL. We need to switch it manually. +*/ +if (unlikely (static_cpu_has (X86_FEATURE_XENPV ) && + prev->iopl != next->iopl)) +xen_set_iopl_mask (next->iopl); +#endif +if (static_cpu_has_bug (X86_BUG_SYSRET_SS_ATTRS )) {/* * AMD CPUs have a misfeature: SYSRET sets the SS selector but
arch/x86/xen/enlighten.c +1 -1
@@ -961,7 +961,7 @@ static void xen_load_sp0(struct tss_struct *tss, tss->x86_tss.sp0 = thread->sp0; } -static void xen_set_iopl_mask (unsigned mask) +void xen_set_iopl_mask (unsigned mask) {struct physdev_set_iopl set_iopl;
Signature · 1 line
2.5.0
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86/iopl/64: Properly context-switch IOPL on Xen PV
x86/iopl/64: Properly context-switch IOPL on Xen PV
Commit-ID: b7a584598aea7ca73140cb87b40319944dd3393f Gitweb: http://git.kernel.org/tip/b7a584598aea7ca73140cb87b40319944dd3393f Author: Andy Lutomirski <[email protected]> AuthorDate: Wed, 16 Mar 2016 14:14:21 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Mar 2016 09:49:26 +0100
x86/iopl/64: Properly context-switch IOPL on Xen PV
On Xen PV, regs->flags doesn't reliably reflect IOPL and the exit-to-userspace code doesn't change IOPL. We need to context switch it manually.
I'm doing this without going through paravirt because this is specific to Xen PV. After the dust settles, we can merge this with the 32-bit code, tidy up the iopl syscall implementation, and remove the set_iopl pvop entirely.
Fixes XSA-171.
--- arch/x86/include/asm/xen/hypervisor.h | 2 ++ arch/x86/kernel/process_64.c | 12 ++++++++++++ arch/x86/xen/enlighten.c | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-)
arch/x86/include/asm/xen/hypervisor.h +2 -0
@@ -62,4 +62,6 @@ void xen_arch_register_cpu(int num);void xen_arch_unregister_cpu (int num);#endif +extern void xen_set_iopl_mask (unsigned mask); +#endif /* _ASM_X86_XEN_HYPERVISOR_H */
arch/x86/kernel/process_64.c +12 -0
@@ -48,6 +48,7 @@#include <asm/syscalls.h> #include <asm/debugreg.h> #include <asm/switch_to.h> +#include <asm/xen/hypervisor.h> asmlinkage extern void ret_from_fork(void);
@@ -411,6 +412,17 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))__switch_to_xtra (prev_p, next_p, tss); +#ifdef CONFIG_XEN +/* +* On Xen PV, IOPL bits in pt_regs->flags have no effect, and +* current_pt_regs()->flags may not match the current task's +* intended IOPL. We need to switch it manually. +*/ +if (unlikely (static_cpu_has (X86_FEATURE_XENPV ) && + prev->iopl != next->iopl)) +xen_set_iopl_mask (next->iopl); +#endif +if (static_cpu_has_bug (X86_BUG_SYSRET_SS_ATTRS )) {/* * AMD CPUs have a misfeature: SYSRET sets the SS selector but
arch/x86/xen/enlighten.c +1 -1
@@ -961,7 +961,7 @@ static void xen_load_sp0(struct tss_struct *tss, tss->x86_tss.sp0 = thread->sp0; } -static void xen_set_iopl_mask (unsigned mask) +void xen_set_iopl_mask (unsigned mask) {struct physdev_set_iopl set_iopl;
Borislav Petkov
maintainerRe: [tip:x86/urgent] x86/iopl/64: Properly context-switch IOPL on Xen PV
Quoted 70 lines · tip-bot for Andy Lutomirski
Commit-ID: b7a584598aea7ca73140cb87b40319944dd3393f Gitweb: http://git.kernel.org/tip/b7a584598aea7ca73140cb87b40319944dd3393f Author: Andy Lutomirski <[email protected]> AuthorDate: Wed, 16 Mar 2016 14:14:21 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Mar 2016 09:49:26 +0100x86/iopl/64: Properly context-switch IOPL on Xen PV
On Xen PV,
regs->flagsdoesn't reliably reflect IOPL and the exit-to-userspace code doesn't change IOPL. We need to context switch it manually.I'm doing this without going through paravirt because this is specific to Xen PV. After the dust settles, we can merge this with the 32-bit code, tidy up the iopl syscall implementation, and remove the
set_ioplpvop entirely.Fixes XSA-171.
Reviewewd-by Jan Beulich <[email protected]>Signed-off-by Andy Lutomirski <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Boris Ostrovsky <[email protected]>Cc Borislav Petkov <[email protected]>Cc Brian Gerst <[email protected]>Cc David Vrabel <[email protected]>Cc Denys Vlasenko <[email protected]>Cc H. Peter Anvin <[email protected]>Cc Jan Beulich <[email protected]>Cc Linus Torvalds <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Link http://lkml.kernel.org/r/693c3bd7aeb4d3c27c92c622b7d0f554a458173c.1458162709.git.luto@kernel.org open hereSigned-off-by Ingo Molnar <[email protected]>--- arch/x86/include/asm/xen/hypervisor.h | 2 ++ arch/x86/kernel/process_64.c | 12 ++++++++++++ arch/x86/xen/enlighten.c | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-)arch/x86/include/asm/xen/hypervisor.h +2
-0@@ -62,4 +62,6 @@ void xen_arch_register_cpu(int num);void xen_arch_unregister_cpu (int num);#endif +extern void xen_set_iopl_mask (unsigned mask); +#endif /* _ASM_X86_XEN_HYPERVISOR_H */ arch/x86/kernel/process_64.c +1
-0@@ -48,6 +48,7 @@#include <asm/syscalls.h> #include <asm/debugreg.h> #include <asm/switch_to.h> +#include <asm/xen/hypervisor.h> asmlinkage extern void ret_from_fork(void);@@ -411,6 +412,17 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))__switch_to_xtra (prev_p, next_p, tss);
arch/x86/kernel/process_64.c __switch_to(struct task_struct *prev_p, struct task_struct *next_p) · line 424
Quoted 10 lines
arch/x86/kernel/process_64.c +10
-0+#ifdef CONFIG_XEN +/* +* On Xen PV, IOPL bits in pt_regs->flags have no effect, and +* current_pt_regs()->flags may not match the current task's +* intended IOPL. We need to switch it manually. +*/ +if (unlikely (static_cpu_has (X86_FEATURE_XENPV ) && + prev->iopl != next->iopl)) +xen_set_iopl_mask (next->iopl); +#endif
I'm wondering if it would've been cleaner if this was a arch_fixup_iopl_mask() defined in arch/x86/xen/enlighten.c and a stub otherwise.
This would save you the ifdeffery and the export of xen_set_iopl_mask()...
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.PATCH v4 3/3 x86/iopl: Fix iopl capability check on Xen PV
In reply to Andy Lutomirskiiopl(3) is supposed to work if iopl is already 3, even if unprivileged. This didn't work right on Xen PV. Fix it.
--- arch/x86/kernel/ioport.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
arch/x86/kernel/ioport.c +9 -3
@@ -96,9 +96,14 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on) SYSCALL_DEFINE1(iopl, unsigned int, level) {struct pt_regs *regs =current_pt_regs (); -unsigned int old = (regs->flags >>12 ) &3 ;struct thread_struct *t = ¤t->thread; +/* +* Careful: the IOPL bits in regs->flags are undefined under Xen PV +* and changing them has no effect. +*/ +unsigned int old = t->iopl >>X86_EFLAGS_IOPL_BIT ; +if (level >3 )return -EINVAL ;/* Trying to gain more privileges? */
@@ -106,8 +111,9 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)if (!capable (CAP_SYS_RAWIO ))return -EPERM ; } - regs->flags = (regs->flags & ~X86_EFLAGS_IOPL ) | (level <<12 ); - t->iopl = level <<12 ; + regs->flags = (regs->flags & ~X86_EFLAGS_IOPL ) | + (level <<X86_EFLAGS_IOPL_BIT ); + t->iopl = level <<X86_EFLAGS_IOPL_BIT ;set_iopl_mask (t->iopl);return 0 ;
Signature · 1 line
2.5.0
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86/iopl: Fix iopl capability check on Xen PV
x86/iopl: Fix iopl capability check on Xen PV
Commit-ID: c29016cf41fe9fa994a5ecca607cf5f1cd98801e Gitweb: http://git.kernel.org/tip/c29016cf41fe9fa994a5ecca607cf5f1cd98801e Author: Andy Lutomirski <[email protected]> AuthorDate: Wed, 16 Mar 2016 14:14:22 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Mar 2016 09:49:27 +0100
x86/iopl: Fix iopl capability check on Xen PV
iopl(3) is supposed to work if iopl is already 3, even if unprivileged. This didn't work right on Xen PV. Fix it.
--- arch/x86/kernel/ioport.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
arch/x86/kernel/ioport.c +9 -3
@@ -96,9 +96,14 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on) SYSCALL_DEFINE1(iopl, unsigned int, level) {struct pt_regs *regs =current_pt_regs (); -unsigned int old = (regs->flags >>12 ) &3 ;struct thread_struct *t = ¤t->thread; +/* +* Careful: the IOPL bits in regs->flags are undefined under Xen PV +* and changing them has no effect. +*/ +unsigned int old = t->iopl >>X86_EFLAGS_IOPL_BIT ; +if (level >3 )return -EINVAL ;/* Trying to gain more privileges? */
@@ -106,8 +111,9 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)if (!capable (CAP_SYS_RAWIO ))return -EPERM ; } - regs->flags = (regs->flags & ~X86_EFLAGS_IOPL ) | (level <<12 ); - t->iopl = level <<12 ; + regs->flags = (regs->flags & ~X86_EFLAGS_IOPL ) | + (level <<X86_EFLAGS_IOPL_BIT ); + t->iopl = level <<X86_EFLAGS_IOPL_BIT ;set_iopl_mask (t->iopl);return 0 ;