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

[Christophe Leroy <christophe.leroy@c-s.fr>]

Le 03/04/2019 à 05:38, Christopher M Riedl a écrit :
>> On March 29, 2019 at 3:41 AM Christophe Leroy <christophe.leroy@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@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@informatik.wtf>

instead of

Signed-off-by: cmr <cmr@informatik.wtf>

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