> On April 3, 2019 at 12:15 AM Christophe Leroy <christophe.le...@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.le...@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 <c...@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 <c...@informatik.wtf> > > instead of > > Signed-off-by: cmr <c...@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.
