[PATCH] powerpc/xmon: add read-only mode

[Christopher M Riedl]

> On April 3, 2019 at 12:15 AM Christophe Leroy <christophe.leroy at c-s.fr> wrote:
> 
> 
> 
> 
> Le 03/04/2019 à 05:38, Christopher M Riedl a écrit :
> >> On March 29, 2019 at 3:41 AM Christophe Leroy <christophe.leroy at c-s.fr> wrote:
> >>
> >>
> >>
> >>
> >> Le 29/03/2019 à 05:21, cmr a écrit :
> >>> Operations which write to memory should be restricted on secure systems
> >>> and optionally to avoid self-destructive behaviors.
> >>>
> >>> Add a config option, XMON_RO, to control default xmon behavior along
> >>> with kernel cmdline options xmon=ro and xmon=rw for explicit control.
> >>> The default is to enable read-only mode.
> >>>
> >>> The following xmon operations are affected:
> >>> memops:
> >>> 	disable memmove
> >>> 	disable memset
> >>> memex:
> >>> 	no-op'd mwrite
> >>> super_regs:
> >>> 	no-op'd write_spr
> >>> bpt_cmds:
> >>> 	disable
> >>> proc_call:
> >>> 	disable
> >>>
> >>> Signed-off-by: cmr <cmr at informatik.wtf>
> >>
> >> A Fully qualified name should be used.
> > 
> > What do you mean by fully-qualified here? PPC_XMON_RO? (PPC_)XMON_READONLY?
> 
> I mean it should be
> 
> Signed-off-by: Christopher M Riedl <cmr at informatik.wtf>
> 
> instead of
> 
> Signed-off-by: cmr <cmr at informatik.wtf>
> 

Hehe, thanks :)

> > 
> >>
> >>> ---
> >>>    arch/powerpc/Kconfig.debug |  7 +++++++
> >>>    arch/powerpc/xmon/xmon.c   | 24 ++++++++++++++++++++++++
> >>>    2 files changed, 31 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> >>> index 4e00cb0a5464..33cc01adf4cb 100644
> >>> --- a/arch/powerpc/Kconfig.debug
> >>> +++ b/arch/powerpc/Kconfig.debug
> >>> @@ -117,6 +117,13 @@ config XMON_DISASSEMBLY
> >>>    	  to say Y here, unless you're building for a memory-constrained
> >>>    	  system.
> >>>    
> >>> +config XMON_RO
> >>> +	bool "Set xmon read-only mode"
> >>> +	depends on XMON
> >>> +	default y
> >>
> >> Should it really be always default y ?
> >> I would set default 'y' only when some security options are also set.
> >>
> > 
> > This is a good point, I based this on an internal Slack suggestion but giving this more thought, disabling read-only mode by default makes more sense. I'm not sure what security options could be set though?
> > 
> 
> Maybe starting with CONFIG_STRICT_KERNEL_RWX
> 
> Another point that may also be addressed by your patch is the definition 
> of PAGE_KERNEL_TEXT:
> 
> #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || 
> defined(CONFIG_BDI_SWITCH) ||\
> 	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
> #define PAGE_KERNEL_TEXT	PAGE_KERNEL_X
> #else
> #define PAGE_KERNEL_TEXT	PAGE_KERNEL_ROX
> #endif
> 
> The above let me think that it would be better if you add a config 
> XMON_RW instead of XMON_RO, with default !STRICT_KERNEL_RWX
> 
> Christophe

Thanks! I like that a lot better, this, along with your other suggestions
in the initial review, will be in the next version.