From: Joerg Roedel <[email protected]>
The user page-table gets the updated kernel mappings in
pti_finalize(), which runs after the RO+X permissions got
applied to the kernel page-table in mark_readonly().
But with CONFIG_DEBUG_WX enabled, the user page-table is
already checked in mark_readonly() for insecure mappings.
This causes false-positive warnings, because the user
page-table did not get the updated mappings yet.
Move the W+X check for the user page-table into
pti_finalize() after it updated all required mappings.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/include/asm/pgtable.h | 7 +++++--
arch/x86/mm/dump_pagetables.c | 3 +--
arch/x86/mm/pti.c | 2 ++
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index e39088cb..a1cb333 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
void ptdump_walk_pgd_level_checkwx(void);
+void ptdump_walk_user_pgd_level_checkwx(void);
#ifdef CONFIG_DEBUG_WX
-#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
#else
-#define debug_checkwx() do { } while (0)
+#define debug_checkwx() do { } while (0)
+#define debug_checkwx_user() do { } while (0)
#endif
/*
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index ccd92c4..b8ab901 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
}
EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
-static void ptdump_walk_user_pgd_level_checkwx(void)
+void ptdump_walk_user_pgd_level_checkwx(void)
{
#ifdef CONFIG_PAGE_TABLE_ISOLATION
pgd_t *pgd = INIT_PGD;
@@ -586,7 +586,6 @@ static void ptdump_walk_user_pgd_level_checkwx(void)
void ptdump_walk_pgd_level_checkwx(void)
{
ptdump_walk_pgd_level_core(NULL, NULL, true, false);
- ptdump_walk_user_pgd_level_checkwx();
}
static int __init pt_dump_init(void)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 69a9d60..026a89a 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -628,4 +628,6 @@ void pti_finalize(void)
*/
pti_clone_entry_text();
pti_clone_kernel_text();
+
+ debug_checkwx_user();
}
--
2.7.4
On 08/08/2018 04:16 AM, Joerg Roedel wrote:
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.
One bit of information missing from the changelog: Could you clarify how
there are any entries in the user page tables for the code to complain?
Before pti_init(), I would have expected the user page tables to be empty.
That causes a different problem, but it would not have resulted in
warnings, so I think I'm missing something.
On Wed, Aug 8, 2018 at 4:16 AM, Joerg Roedel <[email protected]> wrote:
> From: Joerg Roedel <[email protected]>
>
> The user page-table gets the updated kernel mappings in
> pti_finalize(), which runs after the RO+X permissions got
> applied to the kernel page-table in mark_readonly().
>
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.
>
> Move the W+X check for the user page-table into
> pti_finalize() after it updated all required mappings.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/include/asm/pgtable.h | 7 +++++--
> arch/x86/mm/dump_pagetables.c | 3 +--
> arch/x86/mm/pti.c | 2 ++
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index e39088cb..a1cb333 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
> void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
> void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
> void ptdump_walk_pgd_level_checkwx(void);
> +void ptdump_walk_user_pgd_level_checkwx(void);
>
> #ifdef CONFIG_DEBUG_WX
> -#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
> #else
> -#define debug_checkwx() do { } while (0)
> +#define debug_checkwx() do { } while (0)
> +#define debug_checkwx_user() do { } while (0)
> #endif
>
> /*
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index ccd92c4..b8ab901 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
> }
> EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
>
> -static void ptdump_walk_user_pgd_level_checkwx(void)
> +void ptdump_walk_user_pgd_level_checkwx(void)
> {
> #ifdef CONFIG_PAGE_TABLE_ISOLATION
> pgd_t *pgd = INIT_PGD;
> @@ -586,7 +586,6 @@ static void ptdump_walk_user_pgd_level_checkwx(void)
> void ptdump_walk_pgd_level_checkwx(void)
> {
> ptdump_walk_pgd_level_core(NULL, NULL, true, false);
> - ptdump_walk_user_pgd_level_checkwx();
> }
>
> static int __init pt_dump_init(void)
> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
> index 69a9d60..026a89a 100644
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -628,4 +628,6 @@ void pti_finalize(void)
> */
> pti_clone_entry_text();
> pti_clone_kernel_text();
> +
> + debug_checkwx_user();
> }
I'm slightly nervous about complicating this and splitting up the
check. I have a mild preference that all the checks get moved later,
so that all architectures have the checks happening at the same time
during boot. Splitting this up could give us some weird differences
between architectures, etc.
-Kees
--
Kees Cook
Pixel Security
Hi Dave,
On Wed, Aug 08, 2018 at 08:54:37AM -0700, Dave Hansen wrote:
> One bit of information missing from the changelog: Could you clarify how
> there are any entries in the user page tables for the code to complain?
> Before pti_init(), I would have expected the user page tables to be empty.
The W+X check runs at the end of mark_readonly() in x86, which is after
pti_init() already put kernel mappings into the user page-table. Problem
is that the cloned entries are still W+X mapped, which is fixed in
pti_finalize() running _after_ mark_readonly().
Regards,
Joerg
Hi Kees,
On Wed, Aug 08, 2018 at 01:33:01PM -0700, Kees Cook wrote:
> I'm slightly nervous about complicating this and splitting up the
> check. I have a mild preference that all the checks get moved later,
> so that all architectures have the checks happening at the same time
> during boot. Splitting this up could give us some weird differences
> between architectures, etc.
As fas as I can see the checks are implemented on x86, arm, and arm64. I
agree that it would be better to run the checks at a unified place
across architectures and can send a patch-set for set once the dust
around the 32-bit PTI implementation for x86 has settled.
But currently the call-places are architecture specific and with that in
mind the split-up on x86 is the right thing to do. I'll change that back
when I implement your idea above.
Regards,
Joerg
Commit-ID: a13c600e15de44ccf03df28d3311ef3cb754ed9b
Gitweb: https://git.kernel.org/tip/a13c600e15de44ccf03df28d3311ef3cb754ed9b
Author: Joerg Roedel <[email protected]>
AuthorDate: Wed, 8 Aug 2018 13:16:40 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 9 Aug 2018 20:42:07 +0200
x86/mm/pti: Move user W+X check into pti_finalize()
The user page-table gets the updated kernel mappings in pti_finalize(),
which runs after the RO+X permissions got applied to the kernel page-table
in mark_readonly().
But with CONFIG_DEBUG_WX enabled, the user page-table is already checked in
mark_readonly() for insecure mappings. This causes false-positive
warnings, because the user page-table did not get the updated mappings yet.
Move the W+X check for the user page-table into pti_finalize() after it
updated all required mappings.
Signed-off-by: Joerg Roedel <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: "H . Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: Linus Torvalds <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: David Laight <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Andrea Arcangeli <[email protected]>
Cc: Waiman Long <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: "David H . Gutteridge" <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/pgtable.h | 7 +++++--
arch/x86/mm/dump_pagetables.c | 3 +--
arch/x86/mm/pti.c | 2 ++
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index e39088cb59ab..a1cb3339da8d 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
void ptdump_walk_pgd_level_checkwx(void);
+void ptdump_walk_user_pgd_level_checkwx(void);
#ifdef CONFIG_DEBUG_WX
-#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
#else
-#define debug_checkwx() do { } while (0)
+#define debug_checkwx() do { } while (0)
+#define debug_checkwx_user() do { } while (0)
#endif
/*
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index ccd92c4da57c..b8ab9012ffd6 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
}
EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
-static void ptdump_walk_user_pgd_level_checkwx(void)
+void ptdump_walk_user_pgd_level_checkwx(void)
{
#ifdef CONFIG_PAGE_TABLE_ISOLATION
pgd_t *pgd = INIT_PGD;
@@ -586,7 +586,6 @@ static void ptdump_walk_user_pgd_level_checkwx(void)
void ptdump_walk_pgd_level_checkwx(void)
{
ptdump_walk_pgd_level_core(NULL, NULL, true, false);
- ptdump_walk_user_pgd_level_checkwx();
}
static int __init pt_dump_init(void)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 1dc5c683e7a5..d58b4aba9510 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -646,4 +646,6 @@ void pti_finalize(void)
*/
pti_clone_entry_text();
pti_clone_kernel_text();
+
+ debug_checkwx_user();
}
Commit-ID: d878efce73fe86db34ddb2013260adf571a701a7
Gitweb: https://git.kernel.org/tip/d878efce73fe86db34ddb2013260adf571a701a7
Author: Joerg Roedel <[email protected]>
AuthorDate: Wed, 8 Aug 2018 13:16:40 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 10 Aug 2018 21:12:45 +0200
x86/mm/pti: Move user W+X check into pti_finalize()
The user page-table gets the updated kernel mappings in pti_finalize(),
which runs after the RO+X permissions got applied to the kernel page-table
in mark_readonly().
But with CONFIG_DEBUG_WX enabled, the user page-table is already checked in
mark_readonly() for insecure mappings. This causes false-positive
warnings, because the user page-table did not get the updated mappings yet.
Move the W+X check for the user page-table into pti_finalize() after it
updated all required mappings.
[ tglx: Folded !NX supported fix ]
Signed-off-by: Joerg Roedel <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: "H . Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: Linus Torvalds <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: David Laight <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Andrea Arcangeli <[email protected]>
Cc: Waiman Long <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: "David H . Gutteridge" <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/pgtable.h | 7 +++++--
arch/x86/mm/dump_pagetables.c | 6 +++---
arch/x86/mm/pti.c | 2 ++
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index e39088cb59ab..a1cb3339da8d 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
void ptdump_walk_pgd_level_checkwx(void);
+void ptdump_walk_user_pgd_level_checkwx(void);
#ifdef CONFIG_DEBUG_WX
-#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
#else
-#define debug_checkwx() do { } while (0)
+#define debug_checkwx() do { } while (0)
+#define debug_checkwx_user() do { } while (0)
#endif
/*
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index ccd92c4da57c..a12afff146d1 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -569,12 +569,13 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
}
EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
-static void ptdump_walk_user_pgd_level_checkwx(void)
+void ptdump_walk_user_pgd_level_checkwx(void)
{
#ifdef CONFIG_PAGE_TABLE_ISOLATION
pgd_t *pgd = INIT_PGD;
- if (!static_cpu_has(X86_FEATURE_PTI))
+ if (!(__supported_pte_mask & _PAGE_NX) ||
+ !static_cpu_has(X86_FEATURE_PTI))
return;
pr_info("x86/mm: Checking user space page tables\n");
@@ -586,7 +587,6 @@ static void ptdump_walk_user_pgd_level_checkwx(void)
void ptdump_walk_pgd_level_checkwx(void)
{
ptdump_walk_pgd_level_core(NULL, NULL, true, false);
- ptdump_walk_user_pgd_level_checkwx();
}
static int __init pt_dump_init(void)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 1dc5c683e7a5..d58b4aba9510 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -646,4 +646,6 @@ void pti_finalize(void)
*/
pti_clone_entry_text();
pti_clone_kernel_text();
+
+ debug_checkwx_user();
}
On Wed, 2018-08-08 at 13:16 +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> The user page-table gets the updated kernel mappings in
> pti_finalize(), which runs after the RO+X permissions got
> applied to the kernel page-table in mark_readonly().
>
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.
>
> Move the W+X check for the user page-table into
> pti_finalize() after it updated all required mappings.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/include/asm/pgtable.h | 7 +++++--
> arch/x86/mm/dump_pagetables.c | 3 +--
> arch/x86/mm/pti.c | 2 ++
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable.h
> b/arch/x86/include/asm/pgtable.h
> index e39088cb..a1cb333 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long
> address, pmdval_t pmd);
> void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
> void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd,
> bool user);
> void ptdump_walk_pgd_level_checkwx(void);
> +void ptdump_walk_user_pgd_level_checkwx(void);
>
> #ifdef CONFIG_DEBUG_WX
> -#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
> #else
> -#define debug_checkwx() do { } while (0)
> +#define debug_checkwx() do { } while (0)
> +#define debug_checkwx_user() do { } while (0)
> #endif
>
> /*
> diff --git a/arch/x86/mm/dump_pagetables.c
> b/arch/x86/mm/dump_pagetables.c
> index ccd92c4..b8ab901 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file
> *m, pgd_t *pgd, bool user)
> }
> EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
>
> -static void ptdump_walk_user_pgd_level_checkwx(void)
> +void ptdump_walk_user_pgd_level_checkwx(void)
> {
> #ifdef CONFIG_PAGE_TABLE_ISOLATION
> pgd_t *pgd = INIT_PGD;
> @@ -586,7 +586,6 @@ static void
> ptdump_walk_user_pgd_level_checkwx(void)
> void ptdump_walk_pgd_level_checkwx(void)
> {
> ptdump_walk_pgd_level_core(NULL, NULL, true, false);
> - ptdump_walk_user_pgd_level_checkwx();
> }
>
> static int __init pt_dump_init(void)
> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
> index 69a9d60..026a89a 100644
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -628,4 +628,6 @@ void pti_finalize(void)
> */
> pti_clone_entry_text();
> pti_clone_kernel_text();
> +
> + debug_checkwx_user();
> }
I've tested this in a VM and on an Atom laptop, as usual. No
regressions noted.
(The version I tested was the latter pulled into tip:
[ tglx: Folded !NX supported fix ])
Tested-by: David H. Gutteridge <[email protected]>
Regards,
Dave