[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 9 Mar 2020 10:32:27 +0000
From: Steven Price <steven.price@....com>
To: Phong Tran <tranmanphong@...il.com>,
Catalin Marinas <Catalin.Marinas@....com>, "will@...nel.org"
<will@...nel.org>, "alexios.zavras@...el.com" <alexios.zavras@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
Steve Capper <Steve.Capper@....com>, Mark Rutland <Mark.Rutland@....com>,
"broonie@...nel.org" <broonie@...nel.org>,
"keescook@...omium.org" <keescook@...omium.org>
Cc: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
On 07/03/2020 09:39, Phong Tran wrote:
> follow the suggestion from
> https://github.com/KSPP/linux/issues/35
>
> Signed-off-by: Phong Tran <tranmanphong@...il.com>
> ---
> arch/arm64/Kconfig.debug | 3 ++-
> arch/arm64/include/asm/ptdump.h | 2 ++
> arch/arm64/mm/dump.c | 1 +
> arch/arm64/mm/ptdump_debugfs.c | 18 ++++++++++++++++++
> 4 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 1c906d932d6b..be552fa351e2 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -48,7 +48,8 @@ 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 and
> + can be checked by echo "1" to "check_wx_pages" debugfs in runtime.
I would suggest it may be better spelling this out a bit more, because
at the moment it's a little confusing when the config option is "Warn on
W+X mappings at boot", but your change makes it sound like it only
happens when you do the echo. Perhaps something like:
There is no runtime or memory usage effect of this option
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".
>
> If in doubt, say "Y".
>
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 38187f74e089..b80d6b4fc508 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);
> +int ptdump_check_wx_init(void);
> #else
> static inline void ptdump_debugfs_register(struct ptdump_info *info,
> const char *name) { }
> +static inline int ptdump_check_wx_init(void) { return 0; }
> #endif
> void ptdump_check_wx(void);
> #endif /* CONFIG_PTDUMP_CORE */
This is a confusing! Why have you made it dependent on
CONFIG_PTDUMP_DEBUGFS?
Well actually I can see why - it's because you've put the new functions
in ptdump_debugfs.c which is (currently) only built when
CONFIG_PTDUMP_DBEUGFS is enabled.
So you need to either:
a) Ensure the new code is built when CONFIG_PTDUMP_DEBUGFS isn't enabled.
b) Update the Kconfig help text to say that the debugfs file for
triggering a runtime W+X check is only available if
CONFIG_PTDUMP_DEBUGFS is also enabled.
Other than the confusion over config symbols this looks good.
Thanks,
Steve
> 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 1f2eae3e988b..73cddc12c3c2 100644
> --- a/arch/arm64/mm/ptdump_debugfs.c
> +++ b/arch/arm64/mm/ptdump_debugfs.c
> @@ -16,3 +16,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");
> +
> +int ptdump_check_wx_init(void)
> +{
> + return debugfs_create_file("check_wx_pages", 0200, NULL,
> + NULL, &check_wx_fops) ? 0 : -ENOMEM;
> +}
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
