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

[Christopher M Riedl]

> On April 11, 2019 at 8:37 AM Michael Ellerman <mpe at ellerman.id.au> wrote:
> 
> 
> Christopher M Riedl <cmr at informatik.wtf> writes:
> >> On April 8, 2019 at 1:34 AM Oliver <oohall at gmail.com> wrote:
> >> On Mon, Apr 8, 2019 at 1:06 PM Christopher M. Riedl <cmr at informatik.wtf> wrote:
> ...
> >> >
> >> > diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> >> > index 4e00cb0a5464..0c7f21476018 100644
> >> > --- a/arch/powerpc/Kconfig.debug
> >> > +++ b/arch/powerpc/Kconfig.debug
> >> > @@ -117,6 +117,16 @@ config XMON_DISASSEMBLY
> >> >           to say Y here, unless you're building for a memory-constrained
> >> >           system.
> >> >
> >> 
> >> > +config XMON_RW
> >> > +       bool "Allow xmon read and write operations"
> >> > +       depends on XMON
> >> > +       default !STRICT_KERNEL_RWX
> >> > +       help
> >> > +         Allow xmon to read and write to memory and special-purpose registers.
> >> > +          Conversely, prevent xmon write access when set to N. Read and write
> >> > +          access can also be explicitly controlled with 'xmon=rw' or 'xmon=ro'
> >> > +          (read-only) cmdline options. Default is !STRICT_KERNEL_RWX.
> >> 
> >> Maybe I am a dumb, but I found this *extremely* confusing.
> >> Conventionally Kconfig options will control what code is and is not
> >> included in the kernel (see XMON_DISASSEMBLY) rather than changing the
> >> default behaviour of code. It's not wrong to do so and I'm going to
> >> assume that you were following the pattern of XMON_DEFAULT, but I
> >> think you need to be a little more clear about what option actually
> >> does. Renaming it to XMON_DEFAULT_RO_MODE and re-wording the
> >> description to indicate it's a only a mode change would help a lot.
> >> 
> >> Sorry if this comes across as pointless bikeshedding since it's the
> >> opposite of what Christophe said in the last patch, but this was a bit
> >> of a head scratcher.
> >
> > If anyone is dumb here it's me for making this confusing :)
> > I chatted with Michael Ellerman about this, so let me try to explain this more clearly.
> 
> Yeah it's my fault :)
>

"Signed-off-by: Christopher M. Riedl" -- I take full responsibility hah.

> 
> > There are two things I am trying to address with XMON_RW:
> > 1) provide a default access mode for xmon based on system "security"
> 
> I think I've gone off this idea. Tying them together is just enforcing a
> linkage that people may not want.
> 
> I think XMON_RW should just be an option that stands on its own. It
> should probably be default n, to give people a safe default.
> 

Next version includes this along with making it clear that this option
provides the default mode for XMON.

>
> > 2) replace XMON in the decision to write-protect kernel text at compile-time
> 
> We should do that as a separate patch. That's actually a bug in the
> current STRICT_KERNEL_RWX support.
> 
> ie. STRICT_KERNEL_RWX should always give you PAGE_KERNEL_ROX, regardless
> of XMON or anything else.
> 
> > I think a single Kconfig for both of those things is sensible as ultimately the
> > point is to allow xmon to operate in read-only mode on "secure" systems -- without
> > violating any integrity/security guarantees (such as write-protected kernel text).
> >
> > Christophe suggested looking at STRICT_KERNEL_RWX and I think that option makes the
> > most sense to base XMON_RW on since the description for STRICT_KERNEL_RWX states:
> 
> Once we fix the bugs in STRICT_KERNEL_RWX people are going to enable
> that by default, so it will essentially be always on in future.
> 
> 
> > With that said, I will remove the 'xmon=rw' cmdline option as it really doesn't work
> > since kernel text is write-protected at compile time.
> 
> I think 'xmon=rw' still makes sense. Only some of the RW functionality
> relies on being able to patch kernel text.
> 
> And once you have proccall() you can just call a function to make it
> read/write anyway, or use memex to manually frob the page tables.
> 
> cheers

Great, adding this back in the next version.