| Message ID | 20200421173557.10817-1-tranmanphong@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: add check_wx_pages debugfs for CHECK_WX | expand |
On 21/04/2020 18:35, Phong Tran wrote: > follow the suggestion from > https://github.com/KSPP/linux/issues/35 > > Signed-off-by: Phong Tran <tranmanphong@gmail.com> I'm fine with this as is, so you can have my Reviewed-by: Steven Price <steven.price@arm.com> However, if you have time to look at it then it would be good to look at moving the ptdump_check_wx()/debug_checkwx() calls into common code as this should be supported on arm/arm64/powerpc/riscv/x86 as far as I can see. And it's always best to get these things in common code early on rather than letting the architectures diverge. Also in future it would be good if you include some text in the commit message that explains the purpose/intention of the change rather than just a link. Having a self-contained commit message helps a lot when searching the git history to find out why the code was written the way it is. Steve > --- > Change since v1: > - Update the Kconfig help text > - Don't check the return value of debugfs_create_file() > - Tested on QEMU aarch64 > root@qemuarm64:~# zcat /proc/config.gz | grep PTDUMP > CONFIG_GENERIC_PTDUMP=y > CONFIG_PTDUMP_CORE=y > CONFIG_PTDUMP_DEBUGFS=y > root@qemuarm64:~# uname -a > Linux qemuarm64 5.7.0-rc2-00001-g20ddb383c313 #3 SMP PREEMPT Tue Apr 21 23:18:56 +07 2020 aarch64 GNU/Linux > root@qemuarm64:~# echo 1 > /sys/kernel/debug/check_wx_pages > [ 63.261868] Checked W+X mappings: passed, no W+X pages found > --- > arch/arm64/Kconfig.debug | 5 ++++- > arch/arm64/include/asm/ptdump.h | 2 ++ > arch/arm64/mm/dump.c | 1 + > arch/arm64/mm/ptdump_debugfs.c | 18 ++++++++++++++++++ > 4 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug > index a1efa246c9ed..cd82c9d3664a 100644 > --- a/arch/arm64/Kconfig.debug > +++ b/arch/arm64/Kconfig.debug > @@ -48,7 +48,10 @@ config DEBUG_WX > of other unfixed kernel bugs easier. > > There is no runtime or memory usage effect of this option > - once the kernel has booted up - it's a one time check. > + once the kernel has booted up - it's a one time check at > + boot, and can also be triggered at runtime by echo "1" to > + "check_wx_pages". The "check_wx_pages" is available only with > + CONFIG_PTDUMP_DEBUGFS is enabled. > > If in doubt, say "Y". > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > index 38187f74e089..c90a6ec6f59b 100644 > --- a/arch/arm64/include/asm/ptdump.h > +++ b/arch/arm64/include/asm/ptdump.h > @@ -24,9 +24,11 @@ struct ptdump_info { > void ptdump_walk(struct seq_file *s, struct ptdump_info *info); > #ifdef CONFIG_PTDUMP_DEBUGFS > void ptdump_debugfs_register(struct ptdump_info *info, const char *name); > +void ptdump_check_wx_init(void); > #else > static inline void ptdump_debugfs_register(struct ptdump_info *info, > const char *name) { } > +static inline void ptdump_check_wx_init(void) { } > #endif > void ptdump_check_wx(void); > #endif /* CONFIG_PTDUMP_CORE */ > diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c > index 860c00ec8bd3..60c99a047763 100644 > --- a/arch/arm64/mm/dump.c > +++ b/arch/arm64/mm/dump.c > @@ -378,6 +378,7 @@ static int ptdump_init(void) > #endif > ptdump_initialize(); > ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables"); > + ptdump_check_wx_init(); > return 0; > } > device_initcall(ptdump_init); > diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c > index d29d722ec3ec..6b0aa16cb17b 100644 > --- a/arch/arm64/mm/ptdump_debugfs.c > +++ b/arch/arm64/mm/ptdump_debugfs.c > @@ -20,3 +20,21 @@ void ptdump_debugfs_register(struct ptdump_info *info, const char *name) > { > debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); > } > + > +static int check_wx_debugfs_set(void *data, u64 val) > +{ > + if (val != 1ULL) > + return -EINVAL; > + > + ptdump_check_wx(); > + > + return 0; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n"); > + > +void ptdump_check_wx_init(void) > +{ > + debugfs_create_file("check_wx_pages", 0200, NULL, > + NULL, &check_wx_fops) ? 0 : -ENOMEM; > +} >
On Wed, Apr 22, 2020 at 12:35:58AM +0700, Phong Tran wrote: > follow the suggestion from > https://github.com/KSPP/linux/issues/35 > > Signed-off-by: Phong Tran <tranmanphong@gmail.com> > --- > Change since v1: > - Update the Kconfig help text > - Don't check the return value of debugfs_create_file() > - Tested on QEMU aarch64 As on v1, I think that this should be generic across all architectures with CONFIG_DEBUG_WX. Adding this only on arm64 under CONFIG_PTDUMP_DEBUGFS (which does not generally imply a WX check) doesn't seem right. Maybe we need a new ARCH_HAS_CHECK_WX config to make that simpler, but that seems simple to me. Thanks, Marm. > root@qemuarm64:~# zcat /proc/config.gz | grep PTDUMP > CONFIG_GENERIC_PTDUMP=y > CONFIG_PTDUMP_CORE=y > CONFIG_PTDUMP_DEBUGFS=y > root@qemuarm64:~# uname -a > Linux qemuarm64 5.7.0-rc2-00001-g20ddb383c313 #3 SMP PREEMPT Tue Apr 21 23:18:56 +07 2020 aarch64 GNU/Linux > root@qemuarm64:~# echo 1 > /sys/kernel/debug/check_wx_pages > [ 63.261868] Checked W+X mappings: passed, no W+X pages found > --- > arch/arm64/Kconfig.debug | 5 ++++- > arch/arm64/include/asm/ptdump.h | 2 ++ > arch/arm64/mm/dump.c | 1 + > arch/arm64/mm/ptdump_debugfs.c | 18 ++++++++++++++++++ > 4 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug > index a1efa246c9ed..cd82c9d3664a 100644 > --- a/arch/arm64/Kconfig.debug > +++ b/arch/arm64/Kconfig.debug > @@ -48,7 +48,10 @@ config DEBUG_WX > of other unfixed kernel bugs easier. > > There is no runtime or memory usage effect of this option > - once the kernel has booted up - it's a one time check. > + once the kernel has booted up - it's a one time check at > + boot, and can also be triggered at runtime by echo "1" to > + "check_wx_pages". The "check_wx_pages" is available only with > + CONFIG_PTDUMP_DEBUGFS is enabled. > > If in doubt, say "Y". > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > index 38187f74e089..c90a6ec6f59b 100644 > --- a/arch/arm64/include/asm/ptdump.h > +++ b/arch/arm64/include/asm/ptdump.h > @@ -24,9 +24,11 @@ struct ptdump_info { > void ptdump_walk(struct seq_file *s, struct ptdump_info *info); > #ifdef CONFIG_PTDUMP_DEBUGFS > void ptdump_debugfs_register(struct ptdump_info *info, const char *name); > +void ptdump_check_wx_init(void); > #else > static inline void ptdump_debugfs_register(struct ptdump_info *info, > const char *name) { } > +static inline void ptdump_check_wx_init(void) { } > #endif > void ptdump_check_wx(void); > #endif /* CONFIG_PTDUMP_CORE */ > diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c > index 860c00ec8bd3..60c99a047763 100644 > --- a/arch/arm64/mm/dump.c > +++ b/arch/arm64/mm/dump.c > @@ -378,6 +378,7 @@ static int ptdump_init(void) > #endif > ptdump_initialize(); > ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables"); > + ptdump_check_wx_init(); > return 0; > } > device_initcall(ptdump_init); > diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c > index d29d722ec3ec..6b0aa16cb17b 100644 > --- a/arch/arm64/mm/ptdump_debugfs.c > +++ b/arch/arm64/mm/ptdump_debugfs.c > @@ -20,3 +20,21 @@ void ptdump_debugfs_register(struct ptdump_info *info, const char *name) > { > debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); > } > + > +static int check_wx_debugfs_set(void *data, u64 val) > +{ > + if (val != 1ULL) > + return -EINVAL; > + > + ptdump_check_wx(); > + > + return 0; > +} > + > +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n"); > + > +void ptdump_check_wx_init(void) > +{ > + debugfs_create_file("check_wx_pages", 0200, NULL, > + NULL, &check_wx_fops) ? 0 : -ENOMEM; > +} > -- > 2.20.1 >
On Wed, Apr 22, 2020 at 03:35:27PM +0100, Mark Rutland wrote: > On Wed, Apr 22, 2020 at 12:35:58AM +0700, Phong Tran wrote: > > follow the suggestion from > > https://github.com/KSPP/linux/issues/35 > > > > Signed-off-by: Phong Tran <tranmanphong@gmail.com> > > --- > > Change since v1: > > - Update the Kconfig help text > > - Don't check the return value of debugfs_create_file() > > - Tested on QEMU aarch64 > > As on v1, I think that this should be generic across all architectures > with CONFIG_DEBUG_WX. Adding this only on arm64 under > CONFIG_PTDUMP_DEBUGFS (which does not generally imply a WX check) > doesn't seem right. > > Maybe we need a new ARCH_HAS_CHECK_WX config to make that simpler, but > that seems simple to me. Agreed. When I asked about respinning, I assumed this would be done in generic code as you requested on the first version. Phong -- do you think you can take a look at that, please? > Thanks, > Marm. Wow, employee of the month! Will
On Wed, Apr 22, 2020 at 04:26:56PM +0100, Will Deacon wrote: > On Wed, Apr 22, 2020 at 03:35:27PM +0100, Mark Rutland wrote: > > Thanks, > > Marm. > > Wow, employee of the month! Muscle-memory has finally defeated me... Marm.
On 4/22/20 10:26 PM, Will Deacon wrote: > On Wed, Apr 22, 2020 at 03:35:27PM +0100, Mark Rutland wrote: >> On Wed, Apr 22, 2020 at 12:35:58AM +0700, Phong Tran wrote: >>> follow the suggestion from >>> https://github.com/KSPP/linux/issues/35 >>> >>> Signed-off-by: Phong Tran <tranmanphong@gmail.com> >>> --- >>> Change since v1: >>> - Update the Kconfig help text >>> - Don't check the return value of debugfs_create_file() >>> - Tested on QEMU aarch64 >> >> As on v1, I think that this should be generic across all architectures >> with CONFIG_DEBUG_WX. Adding this only on arm64 under >> CONFIG_PTDUMP_DEBUGFS (which does not generally imply a WX check) >> doesn't seem right. >> >> Maybe we need a new ARCH_HAS_CHECK_WX config to make that simpler, but >> that seems simple to me. > > Agreed. When I asked about respinning, I assumed this would be done in > generic code as you requested on the first version. Phong -- do you think > you can take a look at that, please? > sure, plan to make change in mm/ptdump.c with KConfig "ARCH_HAS_CHECK_WX" as suggestion. Then need align with this patch in PowerPC arch also https://lore.kernel.org/kernel-hardening/20200402084053.188537-3-ruscur@russell.cc/ diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug index a1efa246c9ed..50f18e7ff2ae 100644 --- a/arch/arm64/Kconfig.debug +++ b/arch/arm64/Kconfig.debug @@ -25,6 +25,7 @@ config ARM64_RANDOMIZE_TEXT_OFFSET config DEBUG_WX bool "Warn on W+X mappings at boot" + select ARCH_HAS_CHECK_WX select PTDUMP_CORE ---help--- Generate a warning if any W+X mappings are found at boot. diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug index 0271b22e063f..40c9ac5a4db2 100644 --- a/mm/Kconfig.debug +++ b/mm/Kconfig.debug @@ -138,3 +138,6 @@ config PTDUMP_DEBUGFS kernel. If in doubt, say N. + +config ARCH_HAS_CHECK_WX + bool diff --git a/mm/ptdump.c b/mm/ptdump.c index 26208d0d03b7..8f54db007aaa 100644 --- a/mm/ptdump.c +++ b/mm/ptdump.c @@ -137,3 +137,29 @@ void ptdump_walk_pgd(struct ptdump_state *st, struct mm_struct *mm, pgd_t *pgd) /* Flush out the last page */ st->note_page(st, 0, -1, 0); } + +#ifdef CONFIG_ARCH_HAS_CHECK_WX +#include <linux/debugfs.h> +#include <asm/ptdump.h> + +static int check_wx_debugfs_set(void *data, u64 val) +{ + if (val != 1ULL) + return -EINVAL; + + ptdump_check_wx(); + + return 0; +} + +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n"); + +static int ptdump_debugfs_check_wx_init(void) +{ + debugfs_create_file("check_wx_pages", 0200, NULL, + NULL, &check_wx_fops) ? 0 : -ENOMEM; + return 0; +} + +device_initcall(ptdump_debugfs_check_wx_init); +#endif Regards, Phong.
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug index a1efa246c9ed..cd82c9d3664a 100644 --- a/arch/arm64/Kconfig.debug +++ b/arch/arm64/Kconfig.debug @@ -48,7 +48,10 @@ config DEBUG_WX of other unfixed kernel bugs easier. There is no runtime or memory usage effect of this option - once the kernel has booted up - it's a one time check. + once the kernel has booted up - it's a one time check at + boot, and can also be triggered at runtime by echo "1" to + "check_wx_pages". The "check_wx_pages" is available only with + CONFIG_PTDUMP_DEBUGFS is enabled. If in doubt, say "Y". diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h index 38187f74e089..c90a6ec6f59b 100644 --- a/arch/arm64/include/asm/ptdump.h +++ b/arch/arm64/include/asm/ptdump.h @@ -24,9 +24,11 @@ struct ptdump_info { void ptdump_walk(struct seq_file *s, struct ptdump_info *info); #ifdef CONFIG_PTDUMP_DEBUGFS void ptdump_debugfs_register(struct ptdump_info *info, const char *name); +void ptdump_check_wx_init(void); #else static inline void ptdump_debugfs_register(struct ptdump_info *info, const char *name) { } +static inline void ptdump_check_wx_init(void) { } #endif void ptdump_check_wx(void); #endif /* CONFIG_PTDUMP_CORE */ diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c index 860c00ec8bd3..60c99a047763 100644 --- a/arch/arm64/mm/dump.c +++ b/arch/arm64/mm/dump.c @@ -378,6 +378,7 @@ static int ptdump_init(void) #endif ptdump_initialize(); ptdump_debugfs_register(&kernel_ptdump_info, "kernel_page_tables"); + ptdump_check_wx_init(); return 0; } device_initcall(ptdump_init); diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c index d29d722ec3ec..6b0aa16cb17b 100644 --- a/arch/arm64/mm/ptdump_debugfs.c +++ b/arch/arm64/mm/ptdump_debugfs.c @@ -20,3 +20,21 @@ void ptdump_debugfs_register(struct ptdump_info *info, const char *name) { debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); } + +static int check_wx_debugfs_set(void *data, u64 val) +{ + if (val != 1ULL) + return -EINVAL; + + ptdump_check_wx(); + + return 0; +} + +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n"); + +void ptdump_check_wx_init(void) +{ + debugfs_create_file("check_wx_pages", 0200, NULL, + NULL, &check_wx_fops) ? 0 : -ENOMEM; +}
follow the suggestion from https://github.com/KSPP/linux/issues/35 Signed-off-by: Phong Tran <tranmanphong@gmail.com> --- Change since v1: - Update the Kconfig help text - Don't check the return value of debugfs_create_file() - Tested on QEMU aarch64 root@qemuarm64:~# zcat /proc/config.gz | grep PTDUMP CONFIG_GENERIC_PTDUMP=y CONFIG_PTDUMP_CORE=y CONFIG_PTDUMP_DEBUGFS=y root@qemuarm64:~# uname -a Linux qemuarm64 5.7.0-rc2-00001-g20ddb383c313 #3 SMP PREEMPT Tue Apr 21 23:18:56 +07 2020 aarch64 GNU/Linux root@qemuarm64:~# echo 1 > /sys/kernel/debug/check_wx_pages [ 63.261868] Checked W+X mappings: passed, no W+X pages found --- arch/arm64/Kconfig.debug | 5 ++++- arch/arm64/include/asm/ptdump.h | 2 ++ arch/arm64/mm/dump.c | 1 + arch/arm64/mm/ptdump_debugfs.c | 18 ++++++++++++++++++ 4 files changed, 25 insertions(+), 1 deletion(-)