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

On April 11, 2019 at 8:37 AM Michael Ellerman [off-list ref] wrote:


Christopher M Riedl [off-list ref] writes:

quoted

quoted

On April 8, 2019 at 1:34 AM Oliver [off-list ref] wrote:
On Mon, Apr 8, 2019 at 1:06 PM Christopher M. Riedl [off-list ref] wrote:

...

quoted

quoted

quoted

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.

quoted

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

quoted

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.

quoted

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.

quoted

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.

quoted

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.