* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 18:34 [PATCH v5] x86: make sure IDT is page aligned Kees Cook
@ 2013-07-16 18:58 ` Steven Rostedt
2013-07-16 20:21 ` Yinghai Lu
2013-07-16 22:33 ` [tip:x86/urgent] x86: Make " tip-bot for Kees Cook
2 siblings, 0 replies; 17+ messages in thread
From: Steven Rostedt @ 2013-07-16 18:58 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Yinghai Lu, Seiji Aguchi, Fenghua Yu, Frederic Weisbecker,
Paul E. McKenney, Suresh Siddha, PaX Team
On Tue, 2013-07-16 at 11:34 -0700, Kees Cook wrote:
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
>
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: PaX Team <pageexec@gmail.com>
I like it just because it removes those nasty #ifdef blocks for x86_64
or not.
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
> ---
> v5:
> - add comments to all IDTs about alignment reasoning, suggested by Linus
> v4:
> - rework using __page_aligned_bss, suggested by Yinghai LU
> - move all the other IDT variables as well, suggested by HPA
> v3:
> - merge 32-bit and 64-bit idt_table definition
> v2:
> - 32-bit was already aligned
> ---
> arch/x86/kernel/head_64.S | 15 ---------------
> arch/x86/kernel/tracepoint.c | 6 ++----
> arch/x86/kernel/traps.c | 12 ++++++------
> 3 files changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..e1aabdb 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>
> #include "../../x86/xen/xen-head.S"
>
> - .section .bss, "aw", @nobits
> - .align L1_CACHE_BYTES
> -ENTRY(idt_table)
> - .skip IDT_ENTRIES * 16
> -
> - .align L1_CACHE_BYTES
> -ENTRY(debug_idt_table)
> - .skip IDT_ENTRIES * 16
> -
> -#ifdef CONFIG_TRACING
> - .align L1_CACHE_BYTES
> -ENTRY(trace_idt_table)
> - .skip IDT_ENTRIES * 16
> -#endif
> -
> __PAGE_ALIGNED_BSS
> NEXT_PAGE(empty_zero_page)
> .skip PAGE_SIZE
> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
> index 4e584a8..1c113db 100644
> --- a/arch/x86/kernel/tracepoint.c
> +++ b/arch/x86/kernel/tracepoint.c
> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
> struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
> (unsigned long) trace_idt_table };
>
> -#ifndef CONFIG_X86_64
> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
> - = { { { { 0, 0 } } }, };
> -#endif
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> static int trace_irq_vector_refcount;
> static DEFINE_MUTEX(irq_vector_mutex);
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index b0865e8..1b23a1c 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -63,19 +63,19 @@
> #include <asm/x86_init.h>
> #include <asm/pgalloc.h>
> #include <asm/proto.h>
> +
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss;
> #else
> #include <asm/processor-flags.h>
> #include <asm/setup.h>
>
> asmlinkage int system_call(void);
> -
> -/*
> - * The IDT has to be page-aligned to simplify the Pentium
> - * F0 0F bug workaround.
> - */
> -gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
> #endif
>
> +/* Must be page-aligned because the real IDT is used in a fixmap. */
> +gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
> +
> DECLARE_BITMAP(used_vectors, NR_VECTORS);
> EXPORT_SYMBOL_GPL(used_vectors);
>
> --
> 1.7.9.5
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 18:34 [PATCH v5] x86: make sure IDT is page aligned Kees Cook
2013-07-16 18:58 ` Steven Rostedt
@ 2013-07-16 20:21 ` Yinghai Lu
2013-07-16 20:28 ` Kees Cook
2013-07-16 22:33 ` [tip:x86/urgent] x86: Make " tip-bot for Kees Cook
2 siblings, 1 reply; 17+ messages in thread
From: Yinghai Lu @ 2013-07-16 20:21 UTC (permalink / raw)
To: Kees Cook
Cc: Linux Kernel Mailing List, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, the arch/x86 maintainers, Seiji Aguchi,
Fenghua Yu, Steven Rostedt, Frederic Weisbecker,
Paul E. McKenney, Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
>
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: PaX Team <pageexec@gmail.com>
> ---
> v5:
> - add comments to all IDTs about alignment reasoning, suggested by Linus
Where is thread for that?
> v4:
> - rework using __page_aligned_bss, suggested by Yinghai LU
> - move all the other IDT variables as well, suggested by HPA
> v3:
> - merge 32-bit and 64-bit idt_table definition
> v2:
> - 32-bit was already aligned
> ---
> arch/x86/kernel/head_64.S | 15 ---------------
> arch/x86/kernel/tracepoint.c | 6 ++----
> arch/x86/kernel/traps.c | 12 ++++++------
> 3 files changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..e1aabdb 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>
> #include "../../x86/xen/xen-head.S"
>
> - .section .bss, "aw", @nobits
> - .align L1_CACHE_BYTES
> -ENTRY(idt_table)
> - .skip IDT_ENTRIES * 16
> -
> - .align L1_CACHE_BYTES
> -ENTRY(debug_idt_table)
> - .skip IDT_ENTRIES * 16
> -
> -#ifdef CONFIG_TRACING
> - .align L1_CACHE_BYTES
> -ENTRY(trace_idt_table)
> - .skip IDT_ENTRIES * 16
> -#endif
> -
> __PAGE_ALIGNED_BSS
> NEXT_PAGE(empty_zero_page)
> .skip PAGE_SIZE
> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
> index 4e584a8..1c113db 100644
> --- a/arch/x86/kernel/tracepoint.c
> +++ b/arch/x86/kernel/tracepoint.c
> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
> struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
> (unsigned long) trace_idt_table };
>
> -#ifndef CONFIG_X86_64
> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
> - = { { { { 0, 0 } } }, };
> -#endif
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
in that case why not add __cacheline_aligned_bss?
Yinghai
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 20:21 ` Yinghai Lu
@ 2013-07-16 20:28 ` Kees Cook
2013-07-16 20:33 ` Steven Rostedt
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2013-07-16 20:28 UTC (permalink / raw)
To: Yinghai Lu
Cc: Linux Kernel Mailing List, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, the arch/x86 maintainers, Seiji Aguchi,
Fenghua Yu, Steven Rostedt, Frederic Weisbecker,
Paul E. McKenney, Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>> Merge with 32-bit one, since it was already aligned to deal with F00F
>> bug. Since bss is cleared before IDT setup, it can live there. This also
>> moves the other *_idt_table variables into common locations.
>>
>> This avoids the risk of the IDT ever being moved in the bss and having
>> the mapping be offset, resulting in calling incorrect handlers. In the
>> current upstream kernel this is not a manifested bug, but heavily patched
>> kernels (such as those using the PaX patch series) did encounter this bug.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Reported-by: PaX Team <pageexec@gmail.com>
>> ---
>> v5:
>> - add comments to all IDTs about alignment reasoning, suggested by Linus
>
> Where is thread for that?
That was off list, part of trying to coordinate this cleanup vs
minimal changes for the stable tree.
>> v4:
>> - rework using __page_aligned_bss, suggested by Yinghai LU
>> - move all the other IDT variables as well, suggested by HPA
>> v3:
>> - merge 32-bit and 64-bit idt_table definition
>> v2:
>> - 32-bit was already aligned
>> ---
>> arch/x86/kernel/head_64.S | 15 ---------------
>> arch/x86/kernel/tracepoint.c | 6 ++----
>> arch/x86/kernel/traps.c | 12 ++++++------
>> 3 files changed, 8 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index 5e4d8a8..e1aabdb 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>>
>> #include "../../x86/xen/xen-head.S"
>>
>> - .section .bss, "aw", @nobits
>> - .align L1_CACHE_BYTES
>> -ENTRY(idt_table)
>> - .skip IDT_ENTRIES * 16
>> -
>> - .align L1_CACHE_BYTES
>> -ENTRY(debug_idt_table)
>> - .skip IDT_ENTRIES * 16
>> -
>> -#ifdef CONFIG_TRACING
>> - .align L1_CACHE_BYTES
>> -ENTRY(trace_idt_table)
>> - .skip IDT_ENTRIES * 16
>> -#endif
>> -
>> __PAGE_ALIGNED_BSS
>> NEXT_PAGE(empty_zero_page)
>> .skip PAGE_SIZE
>> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
>> index 4e584a8..1c113db 100644
>> --- a/arch/x86/kernel/tracepoint.c
>> +++ b/arch/x86/kernel/tracepoint.c
>> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
>> struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
>> (unsigned long) trace_idt_table };
>>
>> -#ifndef CONFIG_X86_64
>> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
>> - = { { { { 0, 0 } } }, };
>> -#endif
>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> in that case why not add __cacheline_aligned_bss?
It seemed more correct to me to define all the IDTs the same, but
there was no technical reason for that, just one of regularity. I only
care about keeping the real IDT page aligned. :) I'm fine to do
whatever is deemed "correct". :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 20:28 ` Kees Cook
@ 2013-07-16 20:33 ` Steven Rostedt
2013-07-16 20:43 ` H. Peter Anvin
2013-07-16 20:47 ` Kees Cook
0 siblings, 2 replies; 17+ messages in thread
From: Steven Rostedt @ 2013-07-16 20:33 UTC (permalink / raw)
To: Kees Cook
Cc: Yinghai Lu, Linux Kernel Mailing List, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
> >> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> >> Merge with 32-bit one, since it was already aligned to deal with F00F
> >> bug. Since bss is cleared before IDT setup, it can live there. This also
> >> moves the other *_idt_table variables into common locations.
> >>
> It seemed more correct to me to define all the IDTs the same, but
> there was no technical reason for that, just one of regularity. I only
> care about keeping the real IDT page aligned. :) I'm fine to do
> whatever is deemed "correct". :)
I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
what it is). What happens if the F00F bug exists and we switch to an IDT
that's not paged aligned? Is that an issue?
-- Steve
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 20:33 ` Steven Rostedt
@ 2013-07-16 20:43 ` H. Peter Anvin
2013-07-16 20:47 ` Kees Cook
1 sibling, 0 replies; 17+ messages in thread
From: H. Peter Anvin @ 2013-07-16 20:43 UTC (permalink / raw)
To: Steven Rostedt, Kees Cook
Cc: Yinghai Lu, Linux Kernel Mailing List, Thomas Gleixner,
Ingo Molnar, the arch/x86 maintainers, Seiji Aguchi, Fenghua Yu,
Frederic Weisbecker, Paul E. McKenney, Suresh Siddha, PaX Team
The F00F big just means the IDT must be readonly.
Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org>
>wrote:
>> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org>
>wrote:
>> >> Since the IDT is referenced from a fixmap, make sure it is page
>aligned.
>> >> Merge with 32-bit one, since it was already aligned to deal with
>F00F
>> >> bug. Since bss is cleared before IDT setup, it can live there.
>This also
>> >> moves the other *_idt_table variables into common locations.
>> >>
>
>> It seemed more correct to me to define all the IDTs the same, but
>> there was no technical reason for that, just one of regularity. I
>only
>> care about keeping the real IDT page aligned. :) I'm fine to do
>> whatever is deemed "correct". :)
>
>I'm actually unfamiliar with the F00F bug (heard of it, but have no
>idea
>what it is). What happens if the F00F bug exists and we switch to an
>IDT
>that's not paged aligned? Is that an issue?
>
>-- Steve
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 20:33 ` Steven Rostedt
2013-07-16 20:43 ` H. Peter Anvin
@ 2013-07-16 20:47 ` Kees Cook
2013-07-16 22:03 ` H. Peter Anvin
1 sibling, 1 reply; 17+ messages in thread
From: Kees Cook @ 2013-07-16 20:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: Yinghai Lu, Linux Kernel Mailing List, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 1:33 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>> >> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>> >> Merge with 32-bit one, since it was already aligned to deal with F00F
>> >> bug. Since bss is cleared before IDT setup, it can live there. This also
>> >> moves the other *_idt_table variables into common locations.
>> >>
>
>> It seemed more correct to me to define all the IDTs the same, but
>> there was no technical reason for that, just one of regularity. I only
>> care about keeping the real IDT page aligned. :) I'm fine to do
>> whatever is deemed "correct". :)
>
> I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
> what it is). What happens if the F00F bug exists and we switch to an IDT
> that's not paged aligned? Is that an issue?
Regardless of F00F, the IDT is now unconditionally being set up in a
fixmap entry (so that the unprivileged "sidt" instruction won't leak a
"real" kernel address, and so that this exposed address is read-only).
If the real IDT is not page aligned, the fixmap IDT will appear offset
and everything starts calling the wrong handlers.
The other IDTs don't need to be page aligned, but I marked them that
way in the clean up because it seemed sensible to define these tables
similarly. I can change the others to be __cacheline_aligned_bss if
that's desired.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 20:47 ` Kees Cook
@ 2013-07-16 22:03 ` H. Peter Anvin
2013-07-16 22:13 ` Yinghai Lu
0 siblings, 1 reply; 17+ messages in thread
From: H. Peter Anvin @ 2013-07-16 22:03 UTC (permalink / raw)
To: Kees Cook
Cc: Steven Rostedt, Yinghai Lu, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On 07/16/2013 01:47 PM, Kees Cook wrote:
> On Tue, Jul 16, 2013 at 1:33 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>>>>> Merge with 32-bit one, since it was already aligned to deal with F00F
>>>>> bug. Since bss is cleared before IDT setup, it can live there. This also
>>>>> moves the other *_idt_table variables into common locations.
>>>>>
>>
>>> It seemed more correct to me to define all the IDTs the same, but
>>> there was no technical reason for that, just one of regularity. I only
>>> care about keeping the real IDT page aligned. :) I'm fine to do
>>> whatever is deemed "correct". :)
>>
>> I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
>> what it is). What happens if the F00F bug exists and we switch to an IDT
>> that's not paged aligned? Is that an issue?
>
> Regardless of F00F, the IDT is now unconditionally being set up in a
> fixmap entry (so that the unprivileged "sidt" instruction won't leak a
> "real" kernel address, and so that this exposed address is read-only).
> If the real IDT is not page aligned, the fixmap IDT will appear offset
> and everything starts calling the wrong handlers.
>
> The other IDTs don't need to be page aligned, but I marked them that
> way in the clean up because it seemed sensible to define these tables
> similarly. I can change the others to be __cacheline_aligned_bss if
> that's desired.
>
I'm fine keeping them as page aligned. They are page-sized on x86-64
anyway (half page on i386).
-hpa
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 22:03 ` H. Peter Anvin
@ 2013-07-16 22:13 ` Yinghai Lu
2013-07-16 22:16 ` H. Peter Anvin
0 siblings, 1 reply; 17+ messages in thread
From: Yinghai Lu @ 2013-07-16 22:13 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Kees Cook, Steven Rostedt, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 3:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 01:47 PM, Kees Cook wrote:
>> The other IDTs don't need to be page aligned, but I marked them that
>> way in the clean up because it seemed sensible to define these tables
>> similarly. I can change the others to be __cacheline_aligned_bss if
>> that's desired.
>>
>
> I'm fine keeping them as page aligned. They are page-sized on x86-64
> anyway (half page on i386).
ok, then should change
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
==>
> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 22:13 ` Yinghai Lu
@ 2013-07-16 22:16 ` H. Peter Anvin
2013-07-16 23:39 ` Yinghai Lu
0 siblings, 1 reply; 17+ messages in thread
From: H. Peter Anvin @ 2013-07-16 22:16 UTC (permalink / raw)
To: Yinghai Lu
Cc: Kees Cook, Steven Rostedt, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On 07/16/2013 03:13 PM, Yinghai Lu wrote:
>
> ok, then should change
>
>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> ==>
>
>> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
It doesn't need to be cacheline aligned, either, to the best of my
knowledge. The former comment is more correct.
-hpa
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 22:16 ` H. Peter Anvin
@ 2013-07-16 23:39 ` Yinghai Lu
2013-07-16 23:43 ` H. Peter Anvin
0 siblings, 1 reply; 17+ messages in thread
From: Yinghai Lu @ 2013-07-16 23:39 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Kees Cook, Steven Rostedt, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 3:16 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 03:13 PM, Yinghai Lu wrote:
>>
>> ok, then should change
>>
>>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>>
>> ==>
>>
>>> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
>>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> It doesn't need to be cacheline aligned, either, to the best of my
> knowledge. The former comment is more correct.
ok. so the old code is just for optimization to keep it cacheline aligned?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 23:39 ` Yinghai Lu
@ 2013-07-16 23:43 ` H. Peter Anvin
2013-07-16 23:59 ` Yinghai Lu
0 siblings, 1 reply; 17+ messages in thread
From: H. Peter Anvin @ 2013-07-16 23:43 UTC (permalink / raw)
To: Yinghai Lu
Cc: Kees Cook, Steven Rostedt, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On 07/16/2013 04:39 PM, Yinghai Lu wrote:
>
> ok. so the old code is just for optimization to keep it cacheline aligned?
>
To the best of my knowledge. I guess I should look through the git log
to make sure it isn't some old erratum fix.
-hpa
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] x86: make sure IDT is page aligned
2013-07-16 23:43 ` H. Peter Anvin
@ 2013-07-16 23:59 ` Yinghai Lu
0 siblings, 0 replies; 17+ messages in thread
From: Yinghai Lu @ 2013-07-16 23:59 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Kees Cook, Steven Rostedt, Linux Kernel Mailing List,
Thomas Gleixner, Ingo Molnar, the arch/x86 maintainers,
Seiji Aguchi, Fenghua Yu, Frederic Weisbecker, Paul E. McKenney,
Suresh Siddha, PaX Team
On Tue, Jul 16, 2013 at 4:43 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 04:39 PM, Yinghai Lu wrote:
>>
>> ok. so the old code is just for optimization to keep it cacheline aligned?
>>
>
> To the best of my knowledge. I guess I should look through the git log
> to make sure it isn't some old erratum fix.
>
looks like before
commit e57113bc1ff591005ec0b0fb4885d97c01de73d8
Author: Jan Beulich <jbeulich@novell.com>
Date: Sat Mar 25 16:30:01 2006 +0100
[PATCH] x86_64: miscellaneous cleanup
- adjust limits of GDT/IDT pseudo-descriptors (some were off by one)
- move empty_zero_page into .bss.page_aligned
- move cpu_gdt_table into .data.page_aligned
- move idt_table into .bss
- align inital_code and init_rsp
- eliminate pointless (re-)declaration of idt_table in traps.c
it is PAGE aligned as it is after gdt page, after the commit, it is
cacheline aligned.
@@ -378,9 +378,12 @@ gdt_end:
/* zero the remaining page */
.fill PAGE_SIZE / 8 - GDT_ENTRIES,8,0
-ENTRY(idt_table)
- .rept 256
- .quad 0
- .quad 0
- .endr
+ .section .bss, "aw", @nobits
+ .align L1_CACHE_BYTES
+ENTRY(idt_table)
+ .skip 256 * 16
+ .section .bss.page_aligned, "aw", @nobits
+ .align PAGE_SIZE
+ENTRY(empty_zero_page)
+ .skip PAGE_SIZE
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tip:x86/urgent] x86: Make sure IDT is page aligned
2013-07-16 18:34 [PATCH v5] x86: make sure IDT is page aligned Kees Cook
2013-07-16 18:58 ` Steven Rostedt
2013-07-16 20:21 ` Yinghai Lu
@ 2013-07-16 22:33 ` tip-bot for Kees Cook
2013-07-17 18:57 ` Yinghai Lu
2 siblings, 1 reply; 17+ messages in thread
From: tip-bot for Kees Cook @ 2013-07-16 22:33 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, keescook, pageexec, tglx, hpa
Commit-ID: 4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
Gitweb: http://git.kernel.org/tip/4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
Author: Kees Cook <keescook@chromium.org>
AuthorDate: Tue, 16 Jul 2013 11:34:41 -0700
Committer: H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 16 Jul 2013 15:14:48 -0700
x86: Make sure IDT is page aligned
Since the IDT is referenced from a fixmap, make sure it is page aligned.
Merge with 32-bit one, since it was already aligned to deal with F00F
bug. Since bss is cleared before IDT setup, it can live there. This also
moves the other *_idt_table variables into common locations.
This avoids the risk of the IDT ever being moved in the bss and having
the mapping be offset, resulting in calling incorrect handlers. In the
current upstream kernel this is not a manifested bug, but heavily patched
kernels (such as those using the PaX patch series) did encounter this bug.
The tables other than idt_table technically do not need to be page
aligned, at least not at the current time, but using a common
declaration avoids mistakes. On 64 bits the table is exactly one page
long, anyway.
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: http://lkml.kernel.org/r/20130716183441.GA14232@www.outflux.net
Reported-by: PaX Team <pageexec@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
arch/x86/kernel/head_64.S | 15 ---------------
arch/x86/kernel/tracepoint.c | 6 ++----
arch/x86/kernel/traps.c | 12 ++++++------
3 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5e4d8a8..e1aabdb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -512,21 +512,6 @@ ENTRY(phys_base)
#include "../../x86/xen/xen-head.S"
- .section .bss, "aw", @nobits
- .align L1_CACHE_BYTES
-ENTRY(idt_table)
- .skip IDT_ENTRIES * 16
-
- .align L1_CACHE_BYTES
-ENTRY(debug_idt_table)
- .skip IDT_ENTRIES * 16
-
-#ifdef CONFIG_TRACING
- .align L1_CACHE_BYTES
-ENTRY(trace_idt_table)
- .skip IDT_ENTRIES * 16
-#endif
-
__PAGE_ALIGNED_BSS
NEXT_PAGE(empty_zero_page)
.skip PAGE_SIZE
diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
index 4e584a8..1c113db 100644
--- a/arch/x86/kernel/tracepoint.c
+++ b/arch/x86/kernel/tracepoint.c
@@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
(unsigned long) trace_idt_table };
-#ifndef CONFIG_X86_64
-gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
- = { { { { 0, 0 } } }, };
-#endif
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
static int trace_irq_vector_refcount;
static DEFINE_MUTEX(irq_vector_mutex);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0865e8..1b23a1c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -63,19 +63,19 @@
#include <asm/x86_init.h>
#include <asm/pgalloc.h>
#include <asm/proto.h>
+
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss;
#else
#include <asm/processor-flags.h>
#include <asm/setup.h>
asmlinkage int system_call(void);
-
-/*
- * The IDT has to be page-aligned to simplify the Pentium
- * F0 0F bug workaround.
- */
-gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
#endif
+/* Must be page-aligned because the real IDT is used in a fixmap. */
+gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
+
DECLARE_BITMAP(used_vectors, NR_VECTORS);
EXPORT_SYMBOL_GPL(used_vectors);
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [tip:x86/urgent] x86: Make sure IDT is page aligned
2013-07-16 22:33 ` [tip:x86/urgent] x86: Make " tip-bot for Kees Cook
@ 2013-07-17 18:57 ` Yinghai Lu
2013-07-17 19:57 ` H. Peter Anvin
0 siblings, 1 reply; 17+ messages in thread
From: Yinghai Lu @ 2013-07-17 18:57 UTC (permalink / raw)
To: Ingo Molnar, H. Peter Anvin, Linux Kernel Mailing List,
Kees Cook, PaX Team, Thomas Gleixner, H. Peter Anvin
Cc: linux-tip-commits
On Tue, Jul 16, 2013 at 3:33 PM, tip-bot for Kees Cook <tipbot@zytor.com> wrote:
> Commit-ID: 4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
> Gitweb: http://git.kernel.org/tip/4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
> Author: Kees Cook <keescook@chromium.org>
> AuthorDate: Tue, 16 Jul 2013 11:34:41 -0700
> Committer: H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Tue, 16 Jul 2013 15:14:48 -0700
>
> x86: Make sure IDT is page aligned
>
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
>
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
>
> The tables other than idt_table technically do not need to be page
> aligned, at least not at the current time, but using a common
> declaration avoids mistakes. On 64 bits the table is exactly one page
> long, anyway.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: http://lkml.kernel.org/r/20130716183441.GA14232@www.outflux.net
> Reported-by: PaX Team <pageexec@gmail.com>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
Hi, Peter,
Any reason for why following changelog get dropped?
---
v5:
- add comments to all IDTs about alignment reasoning, suggested by Linus
v4:
- rework using __page_aligned_bss, suggested by Yinghai LU
- move all the other IDT variables as well, suggested by HPA
v3:
- merge 32-bit and 64-bit idt_table definition
v2:
- 32-bit was already aligned
---
That at least would help us to check if you apply the right version.
Yinghai
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tip:x86/urgent] x86: Make sure IDT is page aligned
2013-07-17 18:57 ` Yinghai Lu
@ 2013-07-17 19:57 ` H. Peter Anvin
2013-07-18 7:05 ` Ingo Molnar
0 siblings, 1 reply; 17+ messages in thread
From: H. Peter Anvin @ 2013-07-17 19:57 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, Linux Kernel Mailing List, Kees Cook, PaX Team,
Thomas Gleixner, H. Peter Anvin, linux-tip-commits
On 07/17/2013 11:57 AM, Yinghai Lu wrote:
>
> Hi, Peter,
>
> Any reason for why following changelog get dropped?
>
> ---
> v5:
> - add comments to all IDTs about alignment reasoning, suggested by Linus
> v4:
> - rework using __page_aligned_bss, suggested by Yinghai LU
> - move all the other IDT variables as well, suggested by HPA
> v3:
> - merge 32-bit and 64-bit idt_table definition
> v2:
> - 32-bit was already aligned
> ---
>
> That at least would help us to check if you apply the right version.
>
Procedurally, the changelogs don't belong in commit messages. I tend to
leave them in if they are particularly illustrative or if the commit
message only makes sense with the additional context, but neither of
those is really ideal.
Technically, because of the --- line which indicates the end of the
commit message.
-hpa
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [tip:x86/urgent] x86: Make sure IDT is page aligned
2013-07-17 19:57 ` H. Peter Anvin
@ 2013-07-18 7:05 ` Ingo Molnar
0 siblings, 0 replies; 17+ messages in thread
From: Ingo Molnar @ 2013-07-18 7:05 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Yinghai Lu, Linux Kernel Mailing List, Kees Cook, PaX Team,
Thomas Gleixner, H. Peter Anvin, linux-tip-commits
* H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/17/2013 11:57 AM, Yinghai Lu wrote:
> >
> > Hi, Peter,
> >
> > Any reason for why following changelog get dropped?
> >
> > ---
> > v5:
> > - add comments to all IDTs about alignment reasoning, suggested by Linus
> > v4:
> > - rework using __page_aligned_bss, suggested by Yinghai LU
> > - move all the other IDT variables as well, suggested by HPA
> > v3:
> > - merge 32-bit and 64-bit idt_table definition
> > v2:
> > - 32-bit was already aligned
> > ---
> >
> > That at least would help us to check if you apply the right version.
> >
>
> Procedurally, the changelogs don't belong in commit messages. I tend to
> leave them in if they are particularly illustrative or if the commit
> message only makes sense with the additional context, but neither of
> those is really ideal.
>
> Technically, because of the --- line which indicates the end of the
> commit message.
Furthermote, the -tip notification email will generally email-thread on
lkml to the patch submission that was applied.
That is a more robust indication of which submission was applied than any
changelog detail.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 17+ messages in thread