linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
@ 2020-03-07  9:39 Phong Tran
  2020-03-09 10:32 ` Steven Price
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Phong Tran @ 2020-03-07  9:39 UTC (permalink / raw)
  To: catalin.marinas, will, alexios.zavras, tglx, akpm, steven.price,
	steve.capper, mark.rutland, broonie, keescook
  Cc: linux-arm-kernel, linux-kernel, kernel-hardening, Phong Tran

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.
 
 	  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 */
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;
+}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-07  9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
@ 2020-03-09 10:32 ` Steven Price
  2020-03-09 12:17 ` Mark Rutland
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Steven Price @ 2020-03-09 10:32 UTC (permalink / raw)
  To: Phong Tran, Catalin Marinas, will, alexios.zavras, tglx, akpm,
	Steve Capper, Mark Rutland, broonie, keescook
  Cc: linux-arm-kernel, linux-kernel, kernel-hardening

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;
> +}
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-07  9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
  2020-03-09 10:32 ` Steven Price
@ 2020-03-09 12:17 ` Mark Rutland
  2020-03-09 12:21   ` Greg KH
  2020-03-09 16:15   ` Kees Cook
  2020-04-20 20:42 ` Will Deacon
  2020-04-21 17:35 ` [PATCH v2] " Phong Tran
  3 siblings, 2 replies; 13+ messages in thread
From: Mark Rutland @ 2020-03-09 12:17 UTC (permalink / raw)
  To: Phong Tran
  Cc: catalin.marinas, will, alexios.zavras, tglx, akpm, steven.price,
	steve.capper, broonie, keescook, linux-arm-kernel, linux-kernel,
	kernel-hardening

On Sat, Mar 07, 2020 at 04:39:26PM +0700, Phong Tran wrote:
> follow the suggestion from
> https://github.com/KSPP/linux/issues/35

That says:

| This should be implemented for all architectures

... so surely this should be in generic code, rahter than being
arm64-specific?

Thanks,
Mark.

> 
> 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.
>  
>  	  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 */
> 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;
> +}
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-09 12:17 ` Mark Rutland
@ 2020-03-09 12:21   ` Greg KH
  2020-03-09 16:15   ` Kees Cook
  1 sibling, 0 replies; 13+ messages in thread
From: Greg KH @ 2020-03-09 12:21 UTC (permalink / raw)
  To: Phong Tran
  Cc: Mark Rutland, catalin.marinas, will, alexios.zavras, tglx, akpm,
	steven.price, steve.capper, broonie, keescook, linux-arm-kernel,
	linux-kernel, kernel-hardening

On Mon, Mar 09, 2020 at 12:17:14PM +0000, Mark Rutland wrote:
> On Sat, Mar 07, 2020 at 04:39:26PM +0700, Phong Tran wrote:
> > follow the suggestion from
> > https://github.com/KSPP/linux/issues/35
> 
> That says:
> 
> | This should be implemented for all architectures
> 
> ... so surely this should be in generic code, rahter than being
> arm64-specific?
> 
> Thanks,
> Mark.
> 
> > 
> > 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.
> >  
> >  	  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 */
> > 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;

Do not check the return value of this function, especially as it is
returning a pointer, not an int!

Just call the function and move on, you should not do anything different
if it returns an error or not.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  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
  1 sibling, 1 reply; 13+ messages in thread
From: Kees Cook @ 2020-03-09 16:15 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Phong Tran, catalin.marinas, will, alexios.zavras, tglx, akpm,
	steven.price, steve.capper, broonie, linux-arm-kernel,
	linux-kernel, kernel-hardening

On Mon, Mar 09, 2020 at 12:17:14PM +0000, Mark Rutland wrote:
> On Sat, Mar 07, 2020 at 04:39:26PM +0700, Phong Tran wrote:
> > follow the suggestion from
> > https://github.com/KSPP/linux/issues/35
> 
> That says:
> 
> | This should be implemented for all architectures
> 
> ... so surely this should be in generic code, rahter than being
> arm64-specific?

Not all architectures have implemented CONFIG_DEBUG_WX...

-Kees

> 
> Thanks,
> Mark.
> 
> > 
> > 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.
> >  
> >  	  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 */
> > 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;
> > +}
> > -- 
> > 2.20.1
> > 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-09 16:15   ` Kees Cook
@ 2020-03-09 16:51     ` Mark Rutland
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2020-03-09 16:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: Phong Tran, catalin.marinas, will, alexios.zavras, tglx, akpm,
	steven.price, steve.capper, broonie, linux-arm-kernel,
	linux-kernel, kernel-hardening

On Mon, Mar 09, 2020 at 09:15:10AM -0700, Kees Cook wrote:
> On Mon, Mar 09, 2020 at 12:17:14PM +0000, Mark Rutland wrote:
> > On Sat, Mar 07, 2020 at 04:39:26PM +0700, Phong Tran wrote:
> > > follow the suggestion from
> > > https://github.com/KSPP/linux/issues/35
> > 
> > That says:
> > 
> > | This should be implemented for all architectures
> > 
> > ... so surely this should be in generic code, rahter than being
> > arm64-specific?
> 
> Not all architectures have implemented CONFIG_DEBUG_WX...

Sure; I assumed the generic code could be gated with:

#ifdef CONFIG_DEBUG_WX
	debug_checkwx()
#endif

... or something to that effect, so that the common code could handle
all the sysfs bits and ensure that part was consistent.

Thanksm
Mark.
> 
> -Kees
> 
> > 
> > Thanks,
> > Mark.
> > 
> > > 
> > > 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.
> > >  
> > >  	  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 */
> > > 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;
> > > +}
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Kees Cook

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-07  9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
  2020-03-09 10:32 ` Steven Price
  2020-03-09 12:17 ` Mark Rutland
@ 2020-04-20 20:42 ` Will Deacon
  2020-04-21 17:35 ` [PATCH v2] " Phong Tran
  3 siblings, 0 replies; 13+ messages in thread
From: Will Deacon @ 2020-04-20 20:42 UTC (permalink / raw)
  To: Phong Tran
  Cc: catalin.marinas, alexios.zavras, tglx, akpm, steven.price,
	steve.capper, mark.rutland, broonie, keescook, linux-arm-kernel,
	linux-kernel, kernel-hardening

On Sat, Mar 07, 2020 at 04:39:26PM +0700, 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(-)

Any plans to spin an updated version of this? The review feedback all seemed
reasonable to me.

Thanks,

Will

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-03-07  9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
                   ` (2 preceding siblings ...)
  2020-04-20 20:42 ` Will Deacon
@ 2020-04-21 17:35 ` Phong Tran
  2020-04-22 13:32   ` Steven Price
  2020-04-22 14:35   ` Mark Rutland
  3 siblings, 2 replies; 13+ messages in thread
From: Phong Tran @ 2020-04-21 17:35 UTC (permalink / raw)
  To: mark.rutland, steve.capper, steven.price, will, keescook, greg
  Cc: akpm, alexios.zavras, broonie, kernel-hardening,
	linux-arm-kernel, linux-kernel, tglx, Phong Tran

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(-)

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-04-21 17:35 ` [PATCH v2] " Phong Tran
@ 2020-04-22 13:32   ` Steven Price
  2020-04-22 14:35   ` Mark Rutland
  1 sibling, 0 replies; 13+ messages in thread
From: Steven Price @ 2020-04-22 13:32 UTC (permalink / raw)
  To: Phong Tran, mark.rutland, steve.capper, will, keescook, greg
  Cc: akpm, alexios.zavras, broonie, kernel-hardening,
	linux-arm-kernel, linux-kernel, tglx

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;
> +}
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  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
  1 sibling, 1 reply; 13+ messages in thread
From: Mark Rutland @ 2020-04-22 14:35 UTC (permalink / raw)
  To: Phong Tran
  Cc: steve.capper, steven.price, will, keescook, greg, akpm,
	alexios.zavras, broonie, kernel-hardening, linux-arm-kernel,
	linux-kernel, tglx

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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  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
  0 siblings, 2 replies; 13+ messages in thread
From: Will Deacon @ 2020-04-22 15:26 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Phong Tran, steve.capper, steven.price, keescook, greg, akpm,
	alexios.zavras, broonie, kernel-hardening, linux-arm-kernel,
	linux-kernel, tglx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-04-22 15:26     ` Will Deacon
@ 2020-04-22 17:11       ` Mark Rutland
  2020-04-24 10:52       ` Phong Tran
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2020-04-22 17:11 UTC (permalink / raw)
  To: Will Deacon
  Cc: Phong Tran, steve.capper, steven.price, keescook, greg, akpm,
	alexios.zavras, broonie, kernel-hardening, linux-arm-kernel,
	linux-kernel, tglx

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: Re: [PATCH v2] arm64: add check_wx_pages debugfs for CHECK_WX
  2020-04-22 15:26     ` Will Deacon
  2020-04-22 17:11       ` Mark Rutland
@ 2020-04-24 10:52       ` Phong Tran
  1 sibling, 0 replies; 13+ messages in thread
From: Phong Tran @ 2020-04-24 10:52 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland, ruscur
  Cc: tranmanphong, steve.capper, steven.price, keescook, greg, akpm,
	alexios.zavras, broonie, kernel-hardening, linux-arm-kernel,
	linux-kernel, tglx

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.

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-04-24 10:53 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-07  9:39 [PATCH] arm64: add check_wx_pages debugfs for CHECK_WX Phong Tran
2020-03-09 10:32 ` Steven Price
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

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).