From: Steven Price <steven.price@arm.com>
To: Phong Tran <tranmanphong@gmail.com>,
Catalin Marinas <Catalin.Marinas@arm.com>,
"will@kernel.org" <will@kernel.org>,
"alexios.zavras@intel.com" <alexios.zavras@intel.com>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
Steve Capper <Steve.Capper@arm.com>,
Mark Rutland <Mark.Rutland@arm.com>,
"broonie@kernel.org" <broonie@kernel.org>,
"keescook@chromium.org" <keescook@chromium.org>
Cc: "linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel-hardening@lists.openwall.com"
<kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
Date: Mon, 9 Mar 2020 10:32:27 +0000 [thread overview]
Message-ID: <34739c99-3436-e88f-769b-43c48caa8817@arm.com> (raw)
In-Reply-To: <20200307093926.27145-1-tranmanphong@gmail.com>
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@gmail.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;
> +}
>
next prev parent reply other threads:[~2020-03-09 10:32 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-07 9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
2020-03-09 10:32 ` Steven Price [this message]
2020-03-09 12:17 ` Mark Rutland
2020-03-09 12:21 ` Greg KH
2020-03-09 16:15 ` Kees Cook
2020-03-09 16:51 ` Mark Rutland
2020-04-20 20:42 ` Will Deacon
2020-04-21 17:35 ` [PATCH v2] " Phong Tran
2020-04-22 13:32 ` Steven Price
2020-04-22 14:35 ` Mark Rutland
2020-04-22 15:26 ` Will Deacon
2020-04-22 17:11 ` Mark Rutland
2020-04-24 10:52 ` Phong Tran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34739c99-3436-e88f-769b-43c48caa8817@arm.com \
--to=steven.price@arm.com \
--cc=Catalin.Marinas@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Steve.Capper@arm.com \
--cc=akpm@linux-foundation.org \
--cc=alexios.zavras@intel.com \
--cc=broonie@kernel.org \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=tranmanphong@gmail.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).