Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754537AbbBZSbk (ORCPT ); Thu, 26 Feb 2015 13:31:40 -0500 Received: from mail-la0-f49.google.com ([209.85.215.49]:34471 "EHLO mail-la0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752971AbbBZSbj (ORCPT ); Thu, 26 Feb 2015 13:31:39 -0500 MIME-Version: 1.0 In-Reply-To: References: <1424934412-33238-1-git-send-email-wangnan0@huawei.com> From: Andy Lutomirski Date: Thu, 26 Feb 2015 10:31:16 -0800 Message-ID: Subject: Re: [PATCH] x86, traps: maps all IDTs to fixmap area. To: Kees Cook 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: 5497 Lines: 148 On Thu, Feb 26, 2015 at 8:45 AM, Kees Cook wrote: > 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.) There's also the F00F workaround, which IIRC we get for free by using the fixmap, but that also shouldn't matter here. > > -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 -- Andy Lutomirski AMA Capital Management, LLC -- 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/