[PATCH v3] x86: make sure IDT is page aligned

[PATCH v3] x86: make sure IDT is page aligned

[Kees Cook]

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.
This avoids the risk of it ever being moved in the bss and having the
mapping be offset, resulting in calling incorrect handlers.

Signed-off-by: Kees Cook <keescook@chromium.org>
Reported-by: PaX Team <pageexec@gmail.com>
Cc: stable@vger.kernel.org
---
v3:
 - merge 32-bit and 64-bit idt_table definition
v2:
 - 32-bit was already aligned
---
 arch/x86/kernel/head_64.S |    4 ----
 arch/x86/kernel/traps.c   |    7 ++-----
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5e4d8a8..317b8cc 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -514,10 +514,6 @@ ENTRY(phys_base)
 	
 	.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
 
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0865e8..0952614 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -68,13 +68,10 @@
 #include <asm/setup.h>
 
 asmlinkage int system_call(void);
+#endif
 
-/*
- * The IDT has to be page-aligned to simplify the Pentium
- * F0 0F bug workaround.
- */
+/* The IDT has to be page-aligned to keep it aligned with its fixmap. */
 gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
-#endif
 
 DECLARE_BITMAP(used_vectors, NR_VECTORS);
 EXPORT_SYMBOL_GPL(used_vectors);
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

[tip:x86/urgent] x86: Make sure IDT is page aligned

[tip-bot for Kees Cook]

Commit-ID:  c0b3450f101523a49823fa93d155f1d258e5ac6f
Gitweb:     http://git.kernel.org/tip/c0b3450f101523a49823fa93d155f1d258e5ac6f
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Fri, 12 Jul 2013 15:50:17 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Fri, 12 Jul 2013 16:14:08 -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.
This avoids the risk of it ever being moved in the bss and having the
mapping be offset, resulting in calling incorrect handlers.

[ hpa: It isn't clear that this is a manifest bug in any way, but
  tagging for -stable because it shouldn't hurt and might avoid some
  very hard-to-debug breakages due to unrelated changes. ]

Signed-off-by: Kees Cook <keescook@chromium.org>
Link: http://lkml.kernel.org/r/20130712225017.GA5366@www.outflux.net
Reported-by: PaX Team <pageexec@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/head_64.S | 4 ----
 arch/x86/kernel/traps.c   | 7 ++-----
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5e4d8a8..317b8cc 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -514,10 +514,6 @@ ENTRY(phys_base)
 	
 	.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
 
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0865e8..0952614 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -68,13 +68,10 @@
 #include <asm/setup.h>
 
 asmlinkage int system_call(void);
+#endif
 
-/*
- * The IDT has to be page-aligned to simplify the Pentium
- * F0 0F bug workaround.
- */
+/* The IDT has to be page-aligned to keep it aligned with its fixmap. */
 gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
-#endif
 
 DECLARE_BITMAP(used_vectors, NR_VECTORS);
 EXPORT_SYMBOL_GPL(used_vectors);

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Yinghai Lu]

On Fri, Jul 12, 2013 at 8:21 PM, tip-bot for Kees Cook <tipbot@zytor.com> wrote:
> Commit-ID:  c0b3450f101523a49823fa93d155f1d258e5ac6f
> Gitweb:     http://git.kernel.org/tip/c0b3450f101523a49823fa93d155f1d258e5ac6f
> Author:     Kees Cook <keescook@chromium.org>
> AuthorDate: Fri, 12 Jul 2013 15:50:17 -0700
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Fri, 12 Jul 2013 16:14:08 -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.
> This avoids the risk of it ever being moved in the bss and having the
> mapping be offset, resulting in calling incorrect handlers.
>
> [ hpa: It isn't clear that this is a manifest bug in any way, but
>   tagging for -stable because it shouldn't hurt and might avoid some
>   very hard-to-debug breakages due to unrelated changes. ]
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: http://lkml.kernel.org/r/20130712225017.GA5366@www.outflux.net
> Reported-by: PaX Team <pageexec@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> ---
>  arch/x86/kernel/head_64.S | 4 ----
>  arch/x86/kernel/traps.c   | 7 ++-----
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..317b8cc 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -514,10 +514,6 @@ ENTRY(phys_base)
>
>         .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
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index b0865e8..0952614 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -68,13 +68,10 @@
>  #include <asm/setup.h>
>
>  asmlinkage int system_call(void);
> +#endif
>
> -/*
> - * The IDT has to be page-aligned to simplify the Pentium
> - * F0 0F bug workaround.
> - */
> +/* The IDT has to be page-aligned to keep it aligned with its fixmap. */
>  gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
> -#endif
>
>  DECLARE_BITMAP(used_vectors, NR_VECTORS);
>  EXPORT_SYMBOL_GPL(used_vectors);

arch/x86/kernel/traps.c:74:1: warning: braces around scalar
initializer [enabled by default]
arch/x86/kernel/traps.c:74:1: warning: (near initialization for
‘idt_table[0].offset_low’) [enabled by default]
arch/x86/kernel/traps.c:74:1: warning: braces around scalar
initializer [enabled by default]
arch/x86/kernel/traps.c:74:1: warning: (near initialization for
‘idt_table[0].offset_low’) [enabled by default]
arch/x86/kernel/traps.c:74:1: warning: excess elements in scalar
initializer [enabled by default]
arch/x86/kernel/traps.c:74:1: warning: (near initialization for
‘idt_table[0].offset_low’) [enabled by default]

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Yinghai Lu]

On Fri, Jul 12, 2013 at 8:21 PM, tip-bot for Kees Cook <tipbot@zytor.com> wrote:
> Commit-ID:  c0b3450f101523a49823fa93d155f1d258e5ac6f
> Gitweb:     http://git.kernel.org/tip/c0b3450f101523a49823fa93d155f1d258e5ac6f
> Author:     Kees Cook <keescook@chromium.org>
> AuthorDate: Fri, 12 Jul 2013 15:50:17 -0700
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Fri, 12 Jul 2013 16:14:08 -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.
> This avoids the risk of it ever being moved in the bss and having the
> mapping be offset, resulting in calling incorrect handlers.

why can put idt_table not be in .bss?

>
> [ hpa: It isn't clear that this is a manifest bug in any way, but
>   tagging for -stable because it shouldn't hurt and might avoid some
>   very hard-to-debug breakages due to unrelated changes. ]
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: http://lkml.kernel.org/r/20130712225017.GA5366@www.outflux.net
> Reported-by: PaX Team <pageexec@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> ---
>  arch/x86/kernel/head_64.S | 4 ----
>  arch/x86/kernel/traps.c   | 7 ++-----
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..317b8cc 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -514,10 +514,6 @@ ENTRY(phys_base)
>
>         .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
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index b0865e8..0952614 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -68,13 +68,10 @@
>  #include <asm/setup.h>
>
>  asmlinkage int system_call(void);
> +#endif
>
> -/*
> - * The IDT has to be page-aligned to simplify the Pentium
> - * F0 0F bug workaround.
> - */
> +/* The IDT has to be page-aligned to keep it aligned with its fixmap. */
>  gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
> -#endif

can we use put all idt_table related in to __bss.aligned section ...?

like
gate_desc idt_table[NR_VECTORS] __page_aligned_bss;

>
>  DECLARE_BITMAP(used_vectors, NR_VECTORS);
>  EXPORT_SYMBOL_GPL(used_vectors);
> --
> 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/

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[H. Peter Anvin]

On 07/13/2013 02:34 PM, Yinghai Lu wrote:
> 
> can we use put all idt_table related in to __bss.aligned section ...?
> 
> like
> gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
> 

I think this is the best solution all around... it not only solves the
initializer problem but also reduces the size of data.

A long, long time ago in a galaxy far, far away I believe the IDT got
set up before we cleared bss, but that is no longer the case as we now
clear bss very, very early (on 32 bits before paging is even enabled.)

Still, it would seem that the minimal solution, if we need something for
3.11/stable, is to simply fix the alignment statement in head_64.S
(optionally move to .bss..page_aligned) and then do the proper cleanup
for 3.12.  If you could prepare a patch for the former (please include
the descriptions we discussed over IRC) and a patchset for the latter it
would be good.

Thanks,

	-hpa

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Kees Cook]

Hi,

On Mon, Jul 15, 2013 at 11:09 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/13/2013 02:34 PM, Yinghai Lu wrote:
>>
>> can we use put all idt_table related in to __bss.aligned section ...?
>>
>> like
>> gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
>>
>
> I think this is the best solution all around... it not only solves the
> initializer problem but also reduces the size of data.
>
> A long, long time ago in a galaxy far, far away I believe the IDT got
> set up before we cleared bss, but that is no longer the case as we now
> clear bss very, very early (on 32 bits before paging is even enabled.)
>
> Still, it would seem that the minimal solution, if we need something for
> 3.11/stable, is to simply fix the alignment statement in head_64.S
> (optionally move to .bss..page_aligned) and then do the proper cleanup
> for 3.12.  If you could prepare a patch for the former (please include
> the descriptions we discussed over IRC) and a patchset for the latter it
> would be good.

Sorry for the trouble I caused on this. I've reorganized how I do test
builds now, which should hopefully cut down on my stupid mistakes.

I'll send two patches, one for 3.11/stable with the absolute minimal
change to alignment, and another for 3.12 that moves all the IDT
variables into common places using __page_aligned_bss as Yinghai Lu
recommended.

Thanks,

-Kees


--
Kees Cook
Chrome OS Security

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Ingo Molnar]

* 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

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[H. Peter Anvin]

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

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Yinghai Lu]

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

[tip:x86/urgent] x86: Make sure IDT is page aligned

[tip-bot for Kees Cook]

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);