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

[Michael Ellerman]

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

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

> 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