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

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

[cmr]

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>
---
 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
+	help
+	  Disable state- and memory-altering write operations in xmon.
+
 config DEBUGGER
 	bool
 	depends on KGDB || XMON
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..c13ee73cdfd4 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -80,6 +80,7 @@ static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
 #endif
 static unsigned long in_xmon __read_mostly = 0;
 static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
+static int xmon_ro = IS_ENABLED(CONFIG_XMON_RO);
 
 static unsigned long adrs;
 static int size = 1;
@@ -1042,6 +1043,8 @@ cmds(struct pt_regs *excp)
 			set_lpp_cmd();
 			break;
 		case 'b':
+			if (xmon_ro == 1)
+				break;
 			bpt_cmds();
 			break;
 		case 'C':
@@ -1055,6 +1058,8 @@ cmds(struct pt_regs *excp)
 			bootcmds();
 			break;
 		case 'p':
+			if (xmon_ro == 1)
+				break;
 			proccall();
 			break;
 		case 'P':
@@ -1777,6 +1782,9 @@ read_spr(int n, unsigned long *vp)
 static void
 write_spr(int n, unsigned long val)
 {
+	if (xmon_ro == 1)
+		return;
+
 	if (setjmp(bus_error_jmp) == 0) {
 		catch_spr_faults = 1;
 		sync();
@@ -2016,6 +2024,10 @@ mwrite(unsigned long adrs, void *buf, int size)
 	char *p, *q;
 
 	n = 0;
+
+	if (xmon_ro == 1)
+		return n;
+
 	if (setjmp(bus_error_jmp) == 0) {
 		catch_memory_errors = 1;
 		sync();
@@ -2884,9 +2896,13 @@ memops(int cmd)
 	scanhex((void *)&mcount);
 	switch( cmd ){
 	case 'm':
+		if (xmon_ro == 1)
+			break;
 		memmove((void *)mdest, (void *)msrc, mcount);
 		break;
 	case 's':
+		if (xmon_ro == 1)
+			break;
 		memset((void *)mdest, mval, mcount);
 		break;
 	case 'd':
@@ -3796,6 +3812,14 @@ static int __init early_parse_xmon(char *p)
 	} else if (strncmp(p, "on", 2) == 0) {
 		xmon_init(1);
 		xmon_on = 1;
+	} else if (strncmp(p, "rw", 2) == 0) {
+		xmon_init(1);
+		xmon_on = 1;
+		xmon_ro = 0;
+	} else if (strncmp(p, "ro", 2) == 0) {
+		xmon_init(1);
+		xmon_on = 1;
+		xmon_ro = 1;
 	} else if (strncmp(p, "off", 3) == 0)
 		xmon_on = 0;
 	else
-- 
2.21.0

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

[Andrew Donnellan]

On 29/3/19 3:21 pm, cmr wrote:
> Operations which write to memory should be restricted on secure systems
> and optionally to avoid self-destructive behaviors.

For reference:
  - https://github.com/linuxppc/issues/issues/219
  - https://github.com/linuxppc/issues/issues/232

Perhaps clarify what is meant here by "secure systems".

Otherwise commit message looks good.

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

You have a git config to fix :)

> ---
>   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
> +	help
> +	  Disable state- and memory-altering write operations in xmon.

The meaning of this option is a bit unclear.

 From the code - it looks like what this option actually does is enable 
RO mode *by default*. In which case it should probably be called 
XMON_RO_DEFAULT and the description should note that RW mode can still 
be enabled via a cmdline option.

> +
>   config DEBUGGER
>   	bool
>   	depends on KGDB || XMON
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index a0f44f992360..c13ee73cdfd4 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -80,6 +80,7 @@ static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
>   #endif
>   static unsigned long in_xmon __read_mostly = 0;
>   static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
> +static int xmon_ro = IS_ENABLED(CONFIG_XMON_RO);
>   
>   static unsigned long adrs;
>   static int size = 1;
> @@ -1042,6 +1043,8 @@ cmds(struct pt_regs *excp)
>   			set_lpp_cmd();
>   			break;
>   		case 'b':
> +			if (xmon_ro == 1)
> +				break;

For all these cases - it would be much better to print an error message 
somewhere when we abort due to read-only mode.

>   			bpt_cmds();
>   			break;
>   		case 'C':
> @@ -1055,6 +1058,8 @@ cmds(struct pt_regs *excp)
>   			bootcmds();
>   			break;
>   		case 'p':
> +			if (xmon_ro == 1)
> +				break;
>   			proccall();
>   			break;
>   		case 'P':
> @@ -1777,6 +1782,9 @@ read_spr(int n, unsigned long *vp)
>   static void
>   write_spr(int n, unsigned long val)
>   {
> +	if (xmon_ro == 1)
> +		return;
> +
>   	if (setjmp(bus_error_jmp) == 0) {
>   		catch_spr_faults = 1;
>   		sync();
> @@ -2016,6 +2024,10 @@ mwrite(unsigned long adrs, void *buf, int size)
>   	char *p, *q;
>   
>   	n = 0;
> +
> +	if (xmon_ro == 1)
> +		return n;
> +
>   	if (setjmp(bus_error_jmp) == 0) {
>   		catch_memory_errors = 1;
>   		sync();
> @@ -2884,9 +2896,13 @@ memops(int cmd)
>   	scanhex((void *)&mcount);
>   	switch( cmd ){
>   	case 'm':
> +		if (xmon_ro == 1)
> +			break;
>   		memmove((void *)mdest, (void *)msrc, mcount);
>   		break;
>   	case 's':
> +		if (xmon_ro == 1)
> +			break;
>   		memset((void *)mdest, mval, mcount);
>   		break;
>   	case 'd':
> @@ -3796,6 +3812,14 @@ static int __init early_parse_xmon(char *p)
>   	} else if (strncmp(p, "on", 2) == 0) {
>   		xmon_init(1);
>   		xmon_on = 1;
> +	} else if (strncmp(p, "rw", 2) == 0) {
> +		xmon_init(1);
> +		xmon_on = 1;
> +		xmon_ro = 0;
> +	} else if (strncmp(p, "ro", 2) == 0) {
> +		xmon_init(1);
> +		xmon_on = 1;
> +		xmon_ro = 1;
>   	} else if (strncmp(p, "off", 3) == 0)
>   		xmon_on = 0;
>   	else
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

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

[Christophe Leroy]

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.

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

> +	help
> +	  Disable state- and memory-altering write operations in xmon.
> +
>   config DEBUGGER
>   	bool
>   	depends on KGDB || XMON
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index a0f44f992360..c13ee73cdfd4 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -80,6 +80,7 @@ static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
>   #endif
>   static unsigned long in_xmon __read_mostly = 0;
>   static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
> +static int xmon_ro = IS_ENABLED(CONFIG_XMON_RO);

Would be better to use a 'bool' type and call it something like 
'xmon_is_ro' (ref Kernel Codying Style, §17)

>   
>   static unsigned long adrs;
>   static int size = 1;
> @@ -1042,6 +1043,8 @@ cmds(struct pt_regs *excp)
>   			set_lpp_cmd();
>   			break;
>   		case 'b':
> +			if (xmon_ro == 1)

if (xmon_is_ro)

[Same everywhere below]


Christophe

> +				break;
>   			bpt_cmds();
>   			break;
>   		case 'C':
> @@ -1055,6 +1058,8 @@ cmds(struct pt_regs *excp)
>   			bootcmds();
>   			break;
>   		case 'p':
> +			if (xmon_ro == 1)
> +				break;
>   			proccall();
>   			break;
>   		case 'P':
> @@ -1777,6 +1782,9 @@ read_spr(int n, unsigned long *vp)
>   static void
>   write_spr(int n, unsigned long val)
>   {
> +	if (xmon_ro == 1)
> +		return;
> +
>   	if (setjmp(bus_error_jmp) == 0) {
>   		catch_spr_faults = 1;
>   		sync();
> @@ -2016,6 +2024,10 @@ mwrite(unsigned long adrs, void *buf, int size)
>   	char *p, *q;
>   
>   	n = 0;
> +
> +	if (xmon_ro == 1)
> +		return n;
> +
>   	if (setjmp(bus_error_jmp) == 0) {
>   		catch_memory_errors = 1;
>   		sync();
> @@ -2884,9 +2896,13 @@ memops(int cmd)
>   	scanhex((void *)&mcount);
>   	switch( cmd ){
>   	case 'm':
> +		if (xmon_ro == 1)
> +			break;
>   		memmove((void *)mdest, (void *)msrc, mcount);
>   		break;
>   	case 's':
> +		if (xmon_ro == 1)
> +			break;
>   		memset((void *)mdest, mval, mcount);
>   		break;
>   	case 'd':
> @@ -3796,6 +3812,14 @@ static int __init early_parse_xmon(char *p)
>   	} else if (strncmp(p, "on", 2) == 0) {
>   		xmon_init(1);
>   		xmon_on = 1;
> +	} else if (strncmp(p, "rw", 2) == 0) {
> +		xmon_init(1);
> +		xmon_on = 1;
> +		xmon_ro = 0;
> +	} else if (strncmp(p, "ro", 2) == 0) {
> +		xmon_init(1);
> +		xmon_on = 1;
> +		xmon_ro = 1;
>   	} else if (strncmp(p, "off", 3) == 0)
>   		xmon_on = 0;
>   	else
> 

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

[Christopher M Riedl]

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

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

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

[Christophe Leroy]

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

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

[Christopher M Riedl]

> On March 29, 2019 at 12:49 AM Andrew Donnellan <andrew.donnellan@au1.ibm.com> wrote:
> 
> 
> On 29/3/19 3:21 pm, cmr wrote:
> > Operations which write to memory should be restricted on secure systems
> > and optionally to avoid self-destructive behaviors.
> 
> For reference:
>   - https://github.com/linuxppc/issues/issues/219
>   - https://github.com/linuxppc/issues/issues/232
> 
> Perhaps clarify what is meant here by "secure systems".
> 
> Otherwise commit message looks good.
> 

I will reword this for the next patch to reflect the verbiage in the referenced
github issue -- ie. Secure Boot and not violating secure boot integrity by using xmon.

> 
> > ---
> >   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
> > +	help
> > +	  Disable state- and memory-altering write operations in xmon.
> 
> The meaning of this option is a bit unclear.
> 
>  From the code - it looks like what this option actually does is enable 
> RO mode *by default*. In which case it should probably be called 
> XMON_RO_DEFAULT and the description should note that RW mode can still 
> be enabled via a cmdline option.
>

Based on Christophe's feedback the default will change for this option in the
next patch. I will also add the cmdline options to the description for clarity.

>
> > +
> >   config DEBUGGER
> >   	bool
> >   	depends on KGDB || XMON
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index a0f44f992360..c13ee73cdfd4 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -80,6 +80,7 @@ static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
> >   #endif
> >   static unsigned long in_xmon __read_mostly = 0;
> >   static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
> > +static int xmon_ro = IS_ENABLED(CONFIG_XMON_RO);
> >   
> >   static unsigned long adrs;
> >   static int size = 1;
> > @@ -1042,6 +1043,8 @@ cmds(struct pt_regs *excp)
> >   			set_lpp_cmd();
> >   			break;
> >   		case 'b':
> > +			if (xmon_ro == 1)
> > +				break;
> 
> For all these cases - it would be much better to print an error message 
> somewhere when we abort due to read-only mode.
> 

I included print messages initially but then thought about how xmon is intended
for "power" users. I can add print statements to avoid confusion and frustration
since the operations are just "silently" dropped -- *if* that aligns with xmon's
"philosophy".

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

[Christopher M Riedl]

> On April 3, 2019 at 12:15 AM Christophe Leroy <christophe.leroy@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@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>
> 

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.

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

[Andrew Donnellan]

On 4/4/19 12:02 am, Christopher M Riedl wrote:
> 
>> On March 29, 2019 at 12:49 AM Andrew Donnellan <andrew.donnellan@au1.ibm.com> wrote:
>>
>>
>> On 29/3/19 3:21 pm, cmr wrote:
>>> Operations which write to memory should be restricted on secure systems
>>> and optionally to avoid self-destructive behaviors.
>>
>> For reference:
>>    - https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_linuxppc_issues_issues_219&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=-pHOU8dm1U-U1crivyxKr_-xvZrIBB8YUqvA3el0Ee0&m=zNkGBUKLoTqdSUy_VUpM8VLTEqy7sJfIXpWU-ujc6Rc&s=9jgy61R_p5jvtwOKCMFfnhmJegzCIIomcf4I1BRvBPg&e=
>>    - https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_linuxppc_issues_issues_232&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=-pHOU8dm1U-U1crivyxKr_-xvZrIBB8YUqvA3el0Ee0&m=zNkGBUKLoTqdSUy_VUpM8VLTEqy7sJfIXpWU-ujc6Rc&s=fFYm1ZTaEp6HbeZMV5JEmlbBtDwdehfiW1H3shFoFMM&e=
>>
>> Perhaps clarify what is meant here by "secure systems".
>>
>> Otherwise commit message looks good.
>>
> 
> I will reword this for the next patch to reflect the verbiage in the referenced
> github issue -- ie. Secure Boot and not violating secure boot integrity by using xmon.

Sounds good.

> 
>>
>>> ---
>>>    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
>>> +	help
>>> +	  Disable state- and memory-altering write operations in xmon.
>>
>> The meaning of this option is a bit unclear.
>>
>>   From the code - it looks like what this option actually does is enable
>> RO mode *by default*. In which case it should probably be called
>> XMON_RO_DEFAULT and the description should note that RW mode can still
>> be enabled via a cmdline option.
>>
> 
> Based on Christophe's feedback the default will change for this option in the
> next patch. I will also add the cmdline options to the description for clarity.
> 

Yep, adding a description of the cmdline options is also a good idea.

>>
>>> +
>>>    config DEBUGGER
>>>    	bool
>>>    	depends on KGDB || XMON
>>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>>> index a0f44f992360..c13ee73cdfd4 100644
>>> --- a/arch/powerpc/xmon/xmon.c
>>> +++ b/arch/powerpc/xmon/xmon.c
>>> @@ -80,6 +80,7 @@ static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
>>>    #endif
>>>    static unsigned long in_xmon __read_mostly = 0;
>>>    static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
>>> +static int xmon_ro = IS_ENABLED(CONFIG_XMON_RO);
>>>    
>>>    static unsigned long adrs;
>>>    static int size = 1;
>>> @@ -1042,6 +1043,8 @@ cmds(struct pt_regs *excp)
>>>    			set_lpp_cmd();
>>>    			break;
>>>    		case 'b':
>>> +			if (xmon_ro == 1)
>>> +				break;
>>
>> For all these cases - it would be much better to print an error message
>> somewhere when we abort due to read-only mode.
>>
> 
> I included print messages initially but then thought about how xmon is intended
> for "power" users. I can add print statements to avoid confusion and frustration
> since the operations are just "silently" dropped -- *if* that aligns with xmon's
> "philosophy".
> 

Power users often want a straightforward self-explanatory UX more than 
anyone :)


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited