* [PATCH] powerpc/xmon: add read-only mode
@ 2019-03-29 4:21 cmr
2019-03-29 4:49 ` Andrew Donnellan
2019-03-29 7:41 ` Christophe Leroy
0 siblings, 2 replies; 8+ messages in thread
From: cmr @ 2019-03-29 4:21 UTC (permalink / raw)
To: linuxppc-dev; +Cc: cmr, andonnel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-03-29 4:21 [PATCH] powerpc/xmon: add read-only mode cmr
@ 2019-03-29 4:49 ` Andrew Donnellan
2019-04-03 13:02 ` Christopher M Riedl
2019-03-29 7:41 ` Christophe Leroy
1 sibling, 1 reply; 8+ messages in thread
From: Andrew Donnellan @ 2019-03-29 4:49 UTC (permalink / raw)
To: cmr, linuxppc-dev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-03-29 4:49 ` Andrew Donnellan
@ 2019-04-03 13:02 ` Christopher M Riedl
2019-04-03 23:59 ` Andrew Donnellan
0 siblings, 1 reply; 8+ messages in thread
From: Christopher M Riedl @ 2019-04-03 13:02 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev
> 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".
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-04-03 13:02 ` Christopher M Riedl
@ 2019-04-03 23:59 ` Andrew Donnellan
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Donnellan @ 2019-04-03 23:59 UTC (permalink / raw)
To: Christopher M Riedl, linuxppc-dev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-03-29 4:21 [PATCH] powerpc/xmon: add read-only mode cmr
2019-03-29 4:49 ` Andrew Donnellan
@ 2019-03-29 7:41 ` Christophe Leroy
2019-04-03 3:38 ` Christopher M Riedl
1 sibling, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2019-03-29 7:41 UTC (permalink / raw)
To: cmr, linuxppc-dev; +Cc: andonnel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-03-29 7:41 ` Christophe Leroy
@ 2019-04-03 3:38 ` Christopher M Riedl
2019-04-03 4:15 ` Christophe Leroy
0 siblings, 1 reply; 8+ messages in thread
From: Christopher M Riedl @ 2019-04-03 3:38 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: andonnel
> 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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-04-03 3:38 ` Christopher M Riedl
@ 2019-04-03 4:15 ` Christophe Leroy
2019-04-03 13:15 ` Christopher M Riedl
0 siblings, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2019-04-03 4:15 UTC (permalink / raw)
To: Christopher M Riedl, linuxppc-dev; +Cc: andonnel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/xmon: add read-only mode
2019-04-03 4:15 ` Christophe Leroy
@ 2019-04-03 13:15 ` Christopher M Riedl
0 siblings, 0 replies; 8+ messages in thread
From: Christopher M Riedl @ 2019-04-03 13:15 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: andonnel
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-04 0:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 4:21 [PATCH] powerpc/xmon: add read-only mode cmr
2019-03-29 4:49 ` Andrew Donnellan
2019-04-03 13:02 ` Christopher M Riedl
2019-04-03 23:59 ` Andrew Donnellan
2019-03-29 7:41 ` Christophe Leroy
2019-04-03 3:38 ` Christopher M Riedl
2019-04-03 4:15 ` Christophe Leroy
2019-04-03 13:15 ` Christopher M Riedl
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.