* [PATCH] x86, traps: maps all IDTs to fixmap area. @ 2015-02-26 7:06 Wang Nan 2015-02-26 15:17 ` Andy Lutomirski 0 siblings, 1 reply; 6+ messages in thread From: Wang Nan @ 2015-02-26 7:06 UTC (permalink / raw) To: masami.hiramatsu.pt, rostedt Cc: mingo, hpa, tglx, x86, luto, oleg, dave.hansen, linux-kernel, lizefan 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 <wangnan0@huawei.com> --- 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. --- 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 <asm/pgalloc.h> #include <asm/proto.h> -/* 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 <asm/processor-flags.h> @@ -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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86, traps: maps all IDTs to fixmap area. 2015-02-26 7:06 [PATCH] x86, traps: maps all IDTs to fixmap area Wang Nan @ 2015-02-26 15:17 ` Andy Lutomirski 2015-02-26 16:45 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Andy Lutomirski @ 2015-02-26 15:17 UTC (permalink / raw) To: Wang Nan, Kees Cook Cc: Masami Hiramatsu, Steven Rostedt, Ingo Molnar, H. Peter Anvin, Thomas Gleixner, X86 ML, Oleg Nesterov, Dave Hansen, linux-kernel@vger.kernel.org, Li Zefan On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan <wangnan0@huawei.com> 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 <wangnan0@huawei.com> > --- > > 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] > > --- > 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 <asm/pgalloc.h> > #include <asm/proto.h> > > -/* 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 <asm/processor-flags.h> > @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86, traps: maps all IDTs to fixmap area. 2015-02-26 15:17 ` Andy Lutomirski @ 2015-02-26 16:45 ` Kees Cook 2015-02-26 18:31 ` Andy Lutomirski 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2015-02-26 16:45 UTC (permalink / raw) 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 On Thu, Feb 26, 2015 at 7:17 AM, Andy Lutomirski <luto@amacapital.net> wrote: > On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan <wangnan0@huawei.com> 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 <wangnan0@huawei.com> >> --- >> >> 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 <asm/pgalloc.h> >> #include <asm/proto.h> >> >> -/* 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 <asm/processor-flags.h> >> @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86, traps: maps all IDTs to fixmap area. 2015-02-26 16:45 ` Kees Cook @ 2015-02-26 18:31 ` Andy Lutomirski 2015-02-27 5:16 ` Wang Nan 0 siblings, 1 reply; 6+ messages in thread From: Andy Lutomirski @ 2015-02-26 18:31 UTC (permalink / raw) 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 On Thu, Feb 26, 2015 at 8:45 AM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Feb 26, 2015 at 7:17 AM, Andy Lutomirski <luto@amacapital.net> wrote: >> On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan <wangnan0@huawei.com> 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 <wangnan0@huawei.com> >>> --- >>> >>> 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 <asm/pgalloc.h> >>> #include <asm/proto.h> >>> >>> -/* 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 <asm/processor-flags.h> >>> @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86, traps: maps all IDTs to fixmap area. 2015-02-26 18:31 ` Andy Lutomirski @ 2015-02-27 5:16 ` Wang Nan 2015-02-27 17:41 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Wang Nan @ 2015-02-27 5:16 UTC (permalink / raw) To: Andy Lutomirski, Kees Cook Cc: Masami Hiramatsu, Steven Rostedt, Ingo Molnar, H. Peter Anvin, Thomas Gleixner, X86 ML, Oleg Nesterov, Dave Hansen, linux-kernel@vger.kernel.org, Li Zefan On 2015/2/27 2:31, Andy Lutomirski wrote: > On Thu, Feb 26, 2015 at 8:45 AM, Kees Cook <keescook@chromium.org> wrote: >> On Thu, Feb 26, 2015 at 7:17 AM, Andy Lutomirski <luto@amacapital.net> wrote: >>> On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan <wangnan0@huawei.com> 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 <wangnan0@huawei.com> >>>> --- >>>> >>>> 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. > What about a flaw module triggering the F00F bug in kernel space? Instead of kernel panic, the system will hang. I think tis should be a case for which my patch can help. However, the trigger condition is critical. >> >> -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 <asm/pgalloc.h> >>>> #include <asm/proto.h> >>>> >>>> -/* 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 <asm/processor-flags.h> >>>> @@ -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 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86, traps: maps all IDTs to fixmap area. 2015-02-27 5:16 ` Wang Nan @ 2015-02-27 17:41 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2015-02-27 17:41 UTC (permalink / raw) To: Wang Nan Cc: Andy Lutomirski, Masami Hiramatsu, Steven Rostedt, Ingo Molnar, H. Peter Anvin, Thomas Gleixner, X86 ML, Oleg Nesterov, Dave Hansen, linux-kernel@vger.kernel.org, Li Zefan On Thu, Feb 26, 2015 at 9:16 PM, Wang Nan <wangnan0@huawei.com> wrote: > On 2015/2/27 2:31, Andy Lutomirski wrote: >> On Thu, Feb 26, 2015 at 8:45 AM, Kees Cook <keescook@chromium.org> wrote: >>> On Thu, Feb 26, 2015 at 7:17 AM, Andy Lutomirski <luto@amacapital.net> wrote: >>>> On Wed, Feb 25, 2015 at 11:06 PM, Wang Nan <wangnan0@huawei.com> 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 <wangnan0@huawei.com> >>>>> --- >>>>> >>>>> 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. >> > > What about a flaw module triggering the F00F bug in kernel space? Instead of > kernel panic, the system will hang. I think tis should be a case for which > my patch can help. However, the trigger condition is critical. If it solves a real problem, I have no objection. I just wanted to be sure we weren't trivially using up a fixmap entry. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-27 17:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-26 7:06 [PATCH] x86, traps: maps all IDTs to fixmap area Wang Nan 2015-02-26 15:17 ` Andy Lutomirski 2015-02-26 16:45 ` Kees Cook 2015-02-26 18:31 ` Andy Lutomirski 2015-02-27 5:16 ` Wang Nan 2015-02-27 17:41 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox