| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 14 Jun 2017 19:34:42 +0200
From: Juergen Gross <jgross@...e.com>
To: Brian Gerst <brgerst@...il.com>
Cc: the arch/x86 maintainers <x86@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>,
"H . Peter Anvin" <hpa@...or.com>,
Andy Lutomirski <luto@...capital.net>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>
Subject: Re: [PATCH 3/3] x86/xen: Move paravirt IOPL switching to slow the
path
On 14/06/17 19:29, Brian Gerst wrote:
> On Wed, Jun 14, 2017 at 9:14 AM, Juergen Gross <jgross@...e.com> wrote:
>> On 14/06/17 14:40, Brian Gerst wrote:
>>> Since tasks using IOPL are very rare, move the switching code to the slow
>>> path for lower impact on normal tasks.
>>>
>>> Signed-off-by: Brian Gerst <brgerst@...il.com>
>>> ---
>>> arch/x86/include/asm/paravirt.h | 6 ++++++
>>> arch/x86/include/asm/processor.h | 1 +
>>> arch/x86/include/asm/thread_info.h | 5 ++++-
>>> arch/x86/include/asm/xen/hypervisor.h | 2 --
>>> arch/x86/kernel/ioport.c | 6 ++++++
>>> arch/x86/kernel/process.c | 8 ++++++++
>>> arch/x86/kernel/process_32.c | 9 ---------
>>> arch/x86/kernel/process_64.c | 11 -----------
>>> arch/x86/xen/enlighten_pv.c | 2 +-
>>> 9 files changed, 26 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>>> index 9a15739..2145dbd 100644
>>> --- a/arch/x86/include/asm/paravirt.h
>>> +++ b/arch/x86/include/asm/paravirt.h
>>> @@ -265,6 +265,12 @@ static inline void write_idt_entry(gate_desc *dt, int entry, const gate_desc *g)
>>> {
>>> PVOP_VCALL3(pv_cpu_ops.write_idt_entry, dt, entry, g);
>>> }
>>> +
>>> +static inline bool paravirt_iopl(void)
>>> +{
>>> + return pv_cpu_ops.set_iopl_mask != paravirt_nop;
>>> +}
>>> +
>>> static inline void set_iopl_mask(unsigned mask)
>>> {
>>> PVOP_VCALL1(pv_cpu_ops.set_iopl_mask, mask);
>>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>>> index 06c4795..4411d67 100644
>>> --- a/arch/x86/include/asm/processor.h
>>> +++ b/arch/x86/include/asm/processor.h
>>> @@ -523,6 +523,7 @@ static inline void load_sp0(struct tss_struct *tss,
>>> native_load_sp0(tss, thread);
>>> }
>>>
>>> +static inline bool paravirt_iopl(void) { return false; }
>>> static inline void set_iopl_mask(unsigned mask) { }
>>> #endif /* CONFIG_PARAVIRT */
>>>
>>> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>>> index e00e1bd..350f3d3 100644
>>> --- a/arch/x86/include/asm/thread_info.h
>>> +++ b/arch/x86/include/asm/thread_info.h
>>> @@ -79,6 +79,7 @@ struct thread_info {
>>> #define TIF_SIGPENDING 2 /* signal pending */
>>> #define TIF_NEED_RESCHED 3 /* rescheduling necessary */
>>> #define TIF_SINGLESTEP 4 /* reenable singlestep on user return*/
>>> +#define TIF_PV_IOPL 5 /* call hypervisor to change IOPL */
>>> #define TIF_SYSCALL_EMU 6 /* syscall emulation active */
>>> #define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */
>>> #define TIF_SECCOMP 8 /* secure computing */
>>> @@ -104,6 +105,7 @@ struct thread_info {
>>> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
>>> #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED)
>>> #define _TIF_SINGLESTEP (1 << TIF_SINGLESTEP)
>>> +#define _TIF_PV_IOPL (1 << TIF_PV_IOPL)
>>> #define _TIF_SYSCALL_EMU (1 << TIF_SYSCALL_EMU)
>>> #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT)
>>> #define _TIF_SECCOMP (1 << TIF_SECCOMP)
>>> @@ -141,7 +143,8 @@ struct thread_info {
>>>
>>> /* flags to check in __switch_to() */
>>> #define _TIF_WORK_CTXSW \
>>> - (_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP)
>>> + (_TIF_IO_BITMAP | _TIF_NOCPUID | _TIF_NOTSC | _TIF_BLOCKSTEP | \
>>> + _TIF_PV_IOPL)
>>>
>>> #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
>>> #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
>>> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
>>> index 39171b3..8b2d4be 100644
>>> --- a/arch/x86/include/asm/xen/hypervisor.h
>>> +++ b/arch/x86/include/asm/xen/hypervisor.h
>>> @@ -62,6 +62,4 @@ 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 */
>>> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
>>> index 9c3cf09..2051e7d 100644
>>> --- a/arch/x86/kernel/ioport.c
>>> +++ b/arch/x86/kernel/ioport.c
>>> @@ -126,6 +126,12 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
>>> regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
>>> (level << X86_EFLAGS_IOPL_BIT);
>>> t->iopl = level << X86_EFLAGS_IOPL_BIT;
>>> + if (paravirt_iopl()) {
>>> + if (level > 0)
>>> + set_thread_flag(TIF_PV_IOPL);
>>> + else
>>> + clear_thread_flag(TIF_PV_IOPL);
>>> + }
>>
>> You could get rid of paravirt_iopl() by adding a "setflag" boolean
>> parameter to set_iopl_mask(). Only the Xen variant would need above
>> thread flag manipulation.
>
> After the first two patches, the hook is a no-op except in the Xen
> case, so the thread flag change gets skipped except on Xen. Moving
> the code to Xen would work, but it would mean that any other
> hypervisor using that hook would also need to duplicate the thread
> flag changes. Granted, it looks like Xen is doing something unique
> here (running the guest kernel at CPL 1), so that probably won't be an
> issue.
Just create a service function which is doing the job. Xen should call
it and any other future non-standard user of set_iopl_mask(), too.
Juergen
Powered by blists - more mailing lists