Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753984AbbBZQp6 (ORCPT ); Thu, 26 Feb 2015 11:45:58 -0500 Received: from mail-oi0-f53.google.com ([209.85.218.53]:44574 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751597AbbBZQp4 (ORCPT ); Thu, 26 Feb 2015 11:45:56 -0500 MIME-Version: 1.0 In-Reply-To: References: <1424934412-33238-1-git-send-email-wangnan0@huawei.com> Date: Thu, 26 Feb 2015 08:45:55 -0800 X-Google-Sender-Auth: YzbwWV7C7itbmoWzUJcbX3yyGMA Message-ID: Subject: Re: [PATCH] x86, traps: maps all IDTs to fixmap area. From: Kees Cook To: Andy Lutomirski Cc: Wang Nan , Masami Hiramatsu , Steven Rostedt , Ingo Molnar , "H. Peter Anvin" , Thomas Gleixner , X86 ML , Oleg Nesterov , Dave Hansen , "linux-kernel@vger.kernel.org" , Li Zefan Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5111 Lines: 137 On Thu, Feb 26, 2015 at 7:17 AM, Andy Lutomirski wrote: > On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan wrote: >> The reason why mapping idt_table to fixmap area should also be applied >> to debug_idt_table and trace_idt_table. This patch does same thing for >> all IDTs. >> >> Signed-off-by: Wang Nan >> --- >> >> I believe trace_idt_table and debug_idt_table should be symmetrical with >> idt_table. However, Like my previous patch 'x86, traps: install gates >> using IST after cpu_init()', I'm not sure whether this is a practical >> fix. > > It shouldn't matter, since we should never enter userspace with these > IDTs loaded. > > --Andy > > [patch kept below for Kees' benefit] Is there a reason to use fixmap entries for these IDTs? Or rather, is there a situation where these IDTs are ever visible to userspace? (The reason to use the fixmap is to hide their "true" location from userspace.) -Kees > >> >> --- >> arch/x86/include/asm/fixmap.h | 6 ++++++ >> arch/x86/kernel/tracepoint.c | 2 +- >> arch/x86/kernel/traps.c | 13 +++++++++++-- >> arch/x86/xen/mmu.c | 6 ++++++ >> 4 files changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h >> index f80d700..79550f4 100644 >> --- a/arch/x86/include/asm/fixmap.h >> +++ b/arch/x86/include/asm/fixmap.h >> @@ -90,6 +90,12 @@ enum fixed_addresses { >> FIX_IO_APIC_BASE_END = FIX_IO_APIC_BASE_0 + MAX_IO_APICS - 1, >> #endif >> FIX_RO_IDT, /* Virtual mapping for read-only IDT */ >> +#ifdef CONFIG_X86_64 >> + FIX_RO_DEBUG_IDT, /* Virtual mapping for read-only debug_idt_table */ >> +#endif >> +#ifdef CONFIG_TRACING >> + FIX_RO_TRACE_IDT, /* Virtual mapping for read-only trace_idt_table */ >> +#endif >> #ifdef CONFIG_X86_32 >> FIX_KMAP_BEGIN, /* reserved pte's for temporary kernel mappings */ >> FIX_KMAP_END = FIX_KMAP_BEGIN+(KM_TYPE_NR*NR_CPUS)-1, >> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c >> index 1c113db..296e130 100644 >> --- a/arch/x86/kernel/tracepoint.c >> +++ b/arch/x86/kernel/tracepoint.c >> @@ -12,7 +12,7 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0); >> struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1, >> (unsigned long) trace_idt_table }; >> >> -/* No need to be aligned, but done to keep all IDTs defined the same way. */ >> +/* Must be page-aligned because the real IDT is used in a fixmap. */ >> gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss; >> >> static int trace_irq_vector_refcount; >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c >> index cf7898e..6d88c37 100644 >> --- a/arch/x86/kernel/traps.c >> +++ b/arch/x86/kernel/traps.c >> @@ -67,7 +67,7 @@ >> #include >> #include >> >> -/* No need to be aligned, but done to keep all IDTs defined the same way. */ >> +/* Must be page-aligned because the real IDT is used in a fixmap. */ >> gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss; >> #else >> #include >> @@ -998,9 +998,18 @@ void __init trap_init(void) >> * Set the IDT descriptor to a fixed read-only location, so that the >> * "sidt" instruction will not leak the location of the kernel, and >> * to defend the IDT against arbitrary memory write vulnerabilities. >> - * It will be reloaded in cpu_init() */ >> + * It will be reloaded in cpu_init() >> + */ >> __set_fixmap(FIX_RO_IDT, __pa_symbol(idt_table), PAGE_KERNEL_RO); >> idt_descr.address = fix_to_virt(FIX_RO_IDT); >> +#ifdef CONFIG_X86_64 >> + __set_fixmap(FIX_RO_DEBUG_IDT, __pa_symbol(debug_idt_table), PAGE_KERNEL_RO); >> + debug_idt_descr.address = fix_to_virt(FIX_RO_DEBUG_IDT); >> +#endif >> +#ifdef CONFIG_TRACING >> + __set_fixmap(FIX_RO_TRACE_IDT, __pa_symbol(trace_idt_table), PAGE_KERNEL_RO); >> + trace_idt_descr.address = fix_to_virt(FIX_RO_TRACE_IDT); >> +#endif >> >> /* >> * Should be a barrier for any external CPU state: >> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c >> index adca9e2..1fd4a4c 100644 >> --- a/arch/x86/xen/mmu.c >> +++ b/arch/x86/xen/mmu.c >> @@ -1984,6 +1984,12 @@ static void xen_set_fixmap(unsigned idx, phys_addr_t phys, pgprot_t prot) >> switch (idx) { >> case FIX_BTMAP_END ... FIX_BTMAP_BEGIN: >> case FIX_RO_IDT: >> +#ifdef CONFIG_X86_64 >> + case FIX_RO_DEBUG_IDT: >> +#endif >> +#ifdef CONFIG_TRACING >> + case FIX_RO_TRACE_IDT: >> +#endif >> #ifdef CONFIG_X86_32 >> case FIX_WP_TEST: >> # ifdef CONFIG_HIGHMEM >> -- >> 1.8.4 >> > > > > -- > Andy Lutomirski > AMA Capital Management, LLC -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/