| Message ID | 1533727000-9172-1-git-send-email-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm/pti: Move user W+X check into pti_finalize() | expand |
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 <joro@8bytes.org> wrote: > From: Joerg Roedel <jroedel@suse.de> > > 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 <jroedel@suse.de> > --- > 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
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
On Wed, 2018-08-08 at 13:16 +0200, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > 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 <jroedel@suse.de> > --- > 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 <dhgutteridge@sympatico.ca> Regards, Dave
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(); }