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

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE2737d

8 messages, 4 authors, 2019-04-12 · open the first message on its own page

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

From: Christopher M. Riedl <hidden>
Date: 2019-04-08 03:07:43

Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
	disable memmove
	disable memset
	disable memzcan
memex:
	no-op'd mwrite
super_regs:
	no-op'd write_spr
bpt_cmds:
	disable
proc_call:
	disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
	Use bool type for xmon_is_ro flag
	Replace XMON_RO with XMON_RW config option
	Make XMON_RW dependent on STRICT_KERNEL_RWX
	Use XMON_RW to control PAGE_KERNEL_TEXT
	Add printf in xmon read-only mode when dropping/skipping writes
	Disable memzcan (zero-fill memop) in xmon read-only mode

 arch/powerpc/Kconfig.debug                   | 10 +++++
 arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
 arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
 arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
 arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)
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.
+
 config DEBUGGER
 	bool
 	depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);
 
 static unsigned long adrs;
 static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
 #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
 #endif
 
+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
+
 static char *help_string = "\
 Commands:\n\
   b	show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
 				memlocate();
 				break;
 			case 'z':
+				if (xmon_is_ro) {
+					printf(xmon_is_ro_warn);
+					break;
+				}
 				memzcan();
 				break;
 			case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
 			set_lpp_cmd();
 			break;
 		case 'b':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
 			bpt_cmds();
 			break;
 		case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
 			bootcmds();
 			break;
 		case 'p':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
 			proccall();
 			break;
 		case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
 static void
 write_spr(int n, unsigned long val)
 {
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return;
+	}
+
 	if (setjmp(bus_error_jmp) == 0) {
 		catch_spr_faults = 1;
 		sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
 	char *p, *q;
 
 	n = 0;
+
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return n;
+	}
+
 	if (setjmp(bus_error_jmp) == 0) {
 		catch_memory_errors = 1;
 		sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
 	scanhex((void *)&mcount);
 	switch( cmd ){
 	case 'm':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
 		memmove((void *)mdest, (void *)msrc, mcount);
 		break;
 	case 's':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
 		memset((void *)mdest, mval, mcount);
 		break;
 	case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+	} else if (strncmp(p, "ro", 2) == 0) {
+		xmon_init(1);
+		xmon_on = 1;
+		xmon_is_ro = true;
 	} else if (strncmp(p, "off", 3) == 0)
 		xmon_on = 0;
 	else
-- 
2.21.0

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

From: Oliver <oohall@gmail.com>
Date: 2019-04-08 05:36:23

On Mon, Apr 8, 2019 at 1:06 PM Christopher M. Riedl [off-list ref] wrote:
Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
        disable memmove
        disable memset
        disable memzcan
memex:
        no-op'd mwrite
super_regs:
        no-op'd write_spr
bpt_cmds:
        disable
proc_call:
        disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
        Use bool type for xmon_is_ro flag
        Replace XMON_RO with XMON_RW config option
        Make XMON_RW dependent on STRICT_KERNEL_RWX
Do you mean make it dependent on XMON?
quoted hunk
        Use XMON_RW to control PAGE_KERNEL_TEXT
        Add printf in xmon read-only mode when dropping/skipping writes
        Disable memzcan (zero-fill memop) in xmon read-only mode

 arch/powerpc/Kconfig.debug                   | 10 +++++
 arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
 arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
 arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
 arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)
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 hunk
+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.
quoted hunk
 config DEBUGGER
        bool
        depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);

 static unsigned long adrs;
 static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
 #define GETWORD(v)     (((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
 #endif

+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
+
 static char *help_string = "\
 Commands:\n\
   b    show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
                                memlocate();
                                break;
                        case 'z':
+                               if (xmon_is_ro) {
+                                       printf(xmon_is_ro_warn);
+                                       break;
+                               }
                                memzcan();
                                break;
                        case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
                        set_lpp_cmd();
                        break;
                case 'b':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        bpt_cmds();
                        break;
                case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
                        bootcmds();
                        break;
                case 'p':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        proccall();
                        break;
                case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
 static void
 write_spr(int n, unsigned long val)
 {
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_spr_faults = 1;
                sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
        char *p, *q;

        n = 0;
+
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return n;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_memory_errors = 1;
                sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
        scanhex((void *)&mcount);
        switch( cmd ){
        case 'm':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memmove((void *)mdest, (void *)msrc, mcount);
                break;
        case 's':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memset((void *)mdest, mval, mcount);
                break;
        case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+       } else if (strncmp(p, "ro", 2) == 0) {
+               xmon_init(1);
+               xmon_on = 1;
+               xmon_is_ro = true;
        } else if (strncmp(p, "off", 3) == 0)
                xmon_on = 0;
        else
--
2.21.0

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

From: Andrew Donnellan <hidden>
Date: 2019-04-08 06:40:33

On 8/4/19 1:08 pm, Christopher M. Riedl wrote:
quoted hunk
Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
	disable memmove
	disable memset
	disable memzcan
memex:
	no-op'd mwrite
super_regs:
	no-op'd write_spr
bpt_cmds:
	disable
proc_call:
	disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
	Use bool type for xmon_is_ro flag
	Replace XMON_RO with XMON_RW config option
	Make XMON_RW dependent on STRICT_KERNEL_RWX
	Use XMON_RW to control PAGE_KERNEL_TEXT
	Add printf in xmon read-only mode when dropping/skipping writes
	Disable memzcan (zero-fill memop) in xmon read-only mode

  arch/powerpc/Kconfig.debug                   | 10 +++++
  arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
  arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
  arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
  arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
  5 files changed, 61 insertions(+), 6 deletions(-)
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"
"Allow xmon write operations" would be clearer. This option has no impact on read operations.
quoted hunk
+	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.
This is an improvement but still doesn't clearly explain the relationship between selecting this option and using the cmdline options.

quoted hunk
+
  config DEBUGGER
  	bool
  	depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	defined(CONFIG_BDI_SWITCH) || defined(CONFIG_KPROBES) || \
+	defined(CONFIG_DYNAMIC_FTRACE)
We might need to rethink how the cmdline options work.

What happens if CONFIG_XMON_RW=n, but "xmon=rw" on the command line?

I think I agree with Oliver that we're not being clear whether this config option is selecting code to be compiled in, or whether it's just setting a default.
quoted hunk
  #define PAGE_KERNEL_TEXT	PAGE_KERNEL_X
  #else
  #define PAGE_KERNEL_TEXT	PAGE_KERNEL_ROX
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);
  
  static unsigned long adrs;
  static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
  #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
  #endif
  
+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
Yay, error messages! Though something like: "Operation disabled: xmon is in read-only mode" would be clearer.
quoted hunk
+
  static char *help_string = "\
  Commands:\n\
    b	show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
  				memlocate();
  				break;
  			case 'z':
+				if (xmon_is_ro) {
+					printf(xmon_is_ro_warn);
+					break;
+				}
  				memzcan();
  				break;
  			case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
  			set_lpp_cmd();
  			break;
  		case 'b':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
  			bpt_cmds();
  			break;
  		case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
  			bootcmds();
  			break;
  		case 'p':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
  			proccall();
  			break;
  		case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
  static void
  write_spr(int n, unsigned long val)
  {
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return;
+	}
+
  	if (setjmp(bus_error_jmp) == 0) {
  		catch_spr_faults = 1;
  		sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
  	char *p, *q;
  
  	n = 0;
+
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return n;
+	}
+
  	if (setjmp(bus_error_jmp) == 0) {
  		catch_memory_errors = 1;
  		sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
  	scanhex((void *)&mcount);
  	switch( cmd ){
  	case 'm':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
  		memmove((void *)mdest, (void *)msrc, mcount);
  		break;
  	case 's':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
  		memset((void *)mdest, mval, mcount);
  		break;
  	case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+	} else if (strncmp(p, "ro", 2) == 0) {
+		xmon_init(1);
+		xmon_on = 1;
+		xmon_is_ro = true;
  	} else if (strncmp(p, "off", 3) == 0)
  		xmon_on = 0;
  	else
-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

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

From: Christopher M Riedl <hidden>
Date: 2019-04-10 02:37:04

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
Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
        disable memmove
        disable memset
        disable memzcan
memex:
        no-op'd mwrite
super_regs:
        no-op'd write_spr
bpt_cmds:
        disable
proc_call:
        disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
        Use bool type for xmon_is_ro flag
        Replace XMON_RO with XMON_RW config option
        Make XMON_RW dependent on STRICT_KERNEL_RWX
Do you mean make it dependent on XMON?
Yeah that's really not clear at all -- XMON_RW is set based on the value of
STRICT_KERNEL_RWX.
quoted
        Use XMON_RW to control PAGE_KERNEL_TEXT
        Add printf in xmon read-only mode when dropping/skipping writes
        Disable memzcan (zero-fill memop) in xmon read-only mode

 arch/powerpc/Kconfig.debug                   | 10 +++++
 arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
 arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
 arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
 arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)
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.

There are two things I am trying to address with XMON_RW:
1) provide a default access mode for xmon based on system "security"
2) replace XMON in the decision to write-protect kernel text at compile-time

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:
If this is set, kernel text and rodata memory will be made read-only,
and non-text memory will be made non-executable. This provides
protection against certain security exploits (e.g. executing the heap
or modifying text)

These features are considered standard security practice these days.
You should say Y here in almost all cases.
Considering this, does XMON_DEFAULT_RO_MODE really make things more clear?

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.
'xmon=ro' remains for those who would optionally like to prevent shooting themselves
in the foot/feet while using xmon.

Hopefully this makes a bit more sense now?
quoted
 config DEBUGGER
        bool
        depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);

 static unsigned long adrs;
 static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
 #define GETWORD(v)     (((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
 #endif

+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
+
 static char *help_string = "\
 Commands:\n\
   b    show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
                                memlocate();
                                break;
                        case 'z':
+                               if (xmon_is_ro) {
+                                       printf(xmon_is_ro_warn);
+                                       break;
+                               }
                                memzcan();
                                break;
                        case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
                        set_lpp_cmd();
                        break;
                case 'b':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        bpt_cmds();
                        break;
                case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
                        bootcmds();
                        break;
                case 'p':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        proccall();
                        break;
                case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
 static void
 write_spr(int n, unsigned long val)
 {
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_spr_faults = 1;
                sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
        char *p, *q;

        n = 0;
+
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return n;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_memory_errors = 1;
                sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
        scanhex((void *)&mcount);
        switch( cmd ){
        case 'm':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memmove((void *)mdest, (void *)msrc, mcount);
                break;
        case 's':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memset((void *)mdest, mval, mcount);
                break;
        case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+       } else if (strncmp(p, "ro", 2) == 0) {
+               xmon_init(1);
+               xmon_on = 1;
+               xmon_is_ro = true;
        } else if (strncmp(p, "off", 3) == 0)
                xmon_on = 0;
        else
--
2.21.0

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

From: Christopher M Riedl <hidden>
Date: 2019-04-10 02:42:51

On April 8, 2019 at 2:37 AM Andrew Donnellan [off-list ref] wrote:


On 8/4/19 1:08 pm, Christopher M. Riedl wrote:
quoted
Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
	disable memmove
	disable memset
	disable memzcan
memex:
	no-op'd mwrite
super_regs:
	no-op'd write_spr
bpt_cmds:
	disable
proc_call:
	disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
	Use bool type for xmon_is_ro flag
	Replace XMON_RO with XMON_RW config option
	Make XMON_RW dependent on STRICT_KERNEL_RWX
	Use XMON_RW to control PAGE_KERNEL_TEXT
	Add printf in xmon read-only mode when dropping/skipping writes
	Disable memzcan (zero-fill memop) in xmon read-only mode

  arch/powerpc/Kconfig.debug                   | 10 +++++
  arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
  arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
  arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
  arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
  5 files changed, 61 insertions(+), 6 deletions(-)
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"
"Allow xmon write operations" would be clearer. This option has no 
impact on read operations.
Agreed, if the option isn't renamed again I will fix this in the next version :)
quoted
+	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.
This is an improvement but still doesn't clearly explain the 
relationship between selecting this option and using the cmdline options.
I will reword this in the next version.
quoted
+
  config DEBUGGER
  	bool
  	depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	defined(CONFIG_BDI_SWITCH) || defined(CONFIG_KPROBES) || \
+	defined(CONFIG_DYNAMIC_FTRACE)
We might need to rethink how the cmdline options work.

What happens if CONFIG_XMON_RW=n, but "xmon=rw" on the command line?

I think I agree with Oliver that we're not being clear whether this 
config option is selecting code to be compiled in, or whether it's just 
setting a default.
Please refer to my reply to Oliver's comments -- 'xmon=rw' is broken and will be removed
in the next version. I think 'xmon=ro' still makes sense to optionally enable xmon's read-
only mode even when writes are allowed on a non-secure system.
quoted
  #define PAGE_KERNEL_TEXT	PAGE_KERNEL_X
  #else
  #define PAGE_KERNEL_TEXT	PAGE_KERNEL_ROX
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
   * set breakpoints anywhere, so don't write protect the kernel text
   * on platforms where such control is possible.
   */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-	defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+	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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);
  
  static unsigned long adrs;
  static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
  #define GETWORD(v)	(((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
  #endif
  
+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
Yay, error messages! Though something like: "Operation disabled: xmon is 
in read-only mode" would be clearer.
Agreed, changing this in the next version.
quoted
+
  static char *help_string = "\
  Commands:\n\
    b	show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
  				memlocate();
  				break;
  			case 'z':
+				if (xmon_is_ro) {
+					printf(xmon_is_ro_warn);
+					break;
+				}
  				memzcan();
  				break;
  			case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
  			set_lpp_cmd();
  			break;
  		case 'b':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
  			bpt_cmds();
  			break;
  		case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
  			bootcmds();
  			break;
  		case 'p':
+			if (xmon_is_ro) {
+				printf(xmon_is_ro_warn);
+				break;
+			}
  			proccall();
  			break;
  		case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
  static void
  write_spr(int n, unsigned long val)
  {
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return;
+	}
+
  	if (setjmp(bus_error_jmp) == 0) {
  		catch_spr_faults = 1;
  		sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
  	char *p, *q;
  
  	n = 0;
+
+	if (xmon_is_ro) {
+		printf(xmon_is_ro_warn);
+		return n;
+	}
+
  	if (setjmp(bus_error_jmp) == 0) {
  		catch_memory_errors = 1;
  		sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
  	scanhex((void *)&mcount);
  	switch( cmd ){
  	case 'm':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
  		memmove((void *)mdest, (void *)msrc, mcount);
  		break;
  	case 's':
+		if (xmon_is_ro) {
+			printf(xmon_is_ro_warn);
+			break;
+		}
  		memset((void *)mdest, mval, mcount);
  		break;
  	case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+	} else if (strncmp(p, "ro", 2) == 0) {
+		xmon_init(1);
+		xmon_on = 1;
+		xmon_is_ro = true;
  	} else if (strncmp(p, "off", 3) == 0)
  		xmon_on = 0;
  	else
-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

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

From: Oliver <oohall@gmail.com>
Date: 2019-04-10 06:55:13

On Wed, Apr 10, 2019 at 12:35 PM Christopher M Riedl [off-list ref] wrote:
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
Operations which write to memory and special purpose registers should be
restricted on systems with integrity guarantees (such as Secure Boot)
and, optionally, to avoid self-destructive behaviors.

Add a config option, XMON_RW, to control default xmon behavior along
with kernel cmdline options xmon=ro and xmon=rw for explicit control.
Use XMON_RW instead of XMON in the condition to set PAGE_KERNEL_TEXT to
allow xmon in read-only mode alongside write-protected kernel text.
XMON_RW defaults to !STRICT_KERNEL_RWX.

The following xmon operations are affected:
memops:
        disable memmove
        disable memset
        disable memzcan
memex:
        no-op'd mwrite
super_regs:
        no-op'd write_spr
bpt_cmds:
        disable
proc_call:
        disable

Signed-off-by: Christopher M. Riedl <redacted>
---
v1->v2:
        Use bool type for xmon_is_ro flag
        Replace XMON_RO with XMON_RW config option
        Make XMON_RW dependent on STRICT_KERNEL_RWX
Do you mean make it dependent on XMON?
Yeah that's really not clear at all -- XMON_RW is set based on the value of
STRICT_KERNEL_RWX.
quoted
quoted
        Use XMON_RW to control PAGE_KERNEL_TEXT
        Add printf in xmon read-only mode when dropping/skipping writes
        Disable memzcan (zero-fill memop) in xmon read-only mode

 arch/powerpc/Kconfig.debug                   | 10 +++++
 arch/powerpc/include/asm/book3s/32/pgtable.h |  5 ++-
 arch/powerpc/include/asm/book3s/64/pgtable.h |  5 ++-
 arch/powerpc/include/asm/nohash/pgtable.h    |  5 ++-
 arch/powerpc/xmon/xmon.c                     | 42 ++++++++++++++++++++
 5 files changed, 61 insertions(+), 6 deletions(-)
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.

There are two things I am trying to address with XMON_RW:
1) provide a default access mode for xmon based on system "security"
2) replace XMON in the decision to write-protect kernel text at compile-time

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:
quoted
If this is set, kernel text and rodata memory will be made read-only,
and non-text memory will be made non-executable. This provides
protection against certain security exploits (e.g. executing the heap
or modifying text)

These features are considered standard security practice these days.
You should say Y here in almost all cases.
Considering this, does XMON_DEFAULT_RO_MODE really make things more clear?
Not really.
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 you're overestimating the importance of being able to write to
the kernel text. The only features that require a writeable text are
using the mem commands to modify the kernel text (a bad idea) and
software breakpoints.

Funnily enough, enabling STRICT_RWX make software breakpoints work
again since it enables a smarter implementation of patch_instruction()
that uses a temporary RW mapping to do the patch. Considering that
kprobes and dynamic ftrace are also implemented with
patch_instruction() too we could just default to the newer
implementation and force PAGE_KERNEL_TEXT to be PAGE_KERNEL_ROX all
the time.
'xmon=ro' remains for those who would optionally like to prevent shooting themselves
in the foot/feet while using xmon.

Hopefully this makes a bit more sense now?
quoted
quoted
 config DEBUGGER
        bool
        depends on KGDB || XMON
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index aa8406b8f7ba..615144ad667d 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -86,8 +86,9 @@ static inline bool pte_user(pte_t pte)
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 581f91be9dd4..bc4655122f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -168,8 +168,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 1ca1c1864b32..c052931bd243 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -22,8 +22,9 @@
  * set breakpoints anywhere, so don't write protect the kernel text
  * on platforms where such control is possible.
  */
-#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
-       defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+#if defined(CONFIG_KGDB) || defined(CONFIG_XMON_RW) || \
+       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
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a0f44f992360..224ca0b3506b 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 bool xmon_is_ro = !IS_ENABLED(CONFIG_XMON_RW);

 static unsigned long adrs;
 static int size = 1;
@@ -202,6 +203,8 @@ static void dump_tlb_book3e(void);
 #define GETWORD(v)     (((v)[0] << 24) + ((v)[1] << 16) + ((v)[2] << 8) + (v)[3])
 #endif

+static const char *xmon_is_ro_warn = "xmon read-only mode: skipping write\n";
+
 static char *help_string = "\
 Commands:\n\
   b    show breakpoints\n\
@@ -989,6 +992,10 @@ cmds(struct pt_regs *excp)
                                memlocate();
                                break;
                        case 'z':
+                               if (xmon_is_ro) {
+                                       printf(xmon_is_ro_warn);
+                                       break;
+                               }
                                memzcan();
                                break;
                        case 'i':
@@ -1042,6 +1049,10 @@ cmds(struct pt_regs *excp)
                        set_lpp_cmd();
                        break;
                case 'b':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        bpt_cmds();
                        break;
                case 'C':
@@ -1055,6 +1066,10 @@ cmds(struct pt_regs *excp)
                        bootcmds();
                        break;
                case 'p':
+                       if (xmon_is_ro) {
+                               printf(xmon_is_ro_warn);
+                               break;
+                       }
                        proccall();
                        break;
                case 'P':
@@ -1777,6 +1792,11 @@ read_spr(int n, unsigned long *vp)
 static void
 write_spr(int n, unsigned long val)
 {
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_spr_faults = 1;
                sync();
@@ -2016,6 +2036,12 @@ mwrite(unsigned long adrs, void *buf, int size)
        char *p, *q;

        n = 0;
+
+       if (xmon_is_ro) {
+               printf(xmon_is_ro_warn);
+               return n;
+       }
+
        if (setjmp(bus_error_jmp) == 0) {
                catch_memory_errors = 1;
                sync();
@@ -2884,9 +2910,17 @@ memops(int cmd)
        scanhex((void *)&mcount);
        switch( cmd ){
        case 'm':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memmove((void *)mdest, (void *)msrc, mcount);
                break;
        case 's':
+               if (xmon_is_ro) {
+                       printf(xmon_is_ro_warn);
+                       break;
+               }
                memset((void *)mdest, mval, mcount);
                break;
        case 'd':
@@ -3796,6 +3830,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_is_ro = false;
+       } else if (strncmp(p, "ro", 2) == 0) {
+               xmon_init(1);
+               xmon_on = 1;
+               xmon_is_ro = true;
        } else if (strncmp(p, "off", 3) == 0)
                xmon_on = 0;
        else
--
2.21.0

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

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2019-04-11 13:09:21

Christopher M Riedl [off-list ref] writes:
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
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 :)
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

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

From: Christopher M Riedl <hidden>
Date: 2019-04-12 01:44:50

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help