[PATCH 0/2] powerpc/pseries: restrict error injection and DT changes when locked down

[PATCH 0/2] powerpc/pseries: restrict error injection and DT changes when locked down

[Nathan Lynch]

Add two new lockdown reasons for use in powerpc's pseries platform
code.

The pseries platform allows hardware-level error injection via certain
calls to the RTAS (Run Time Abstraction Services) firmware. ACPI-based
error injection is already restricted in lockdown; this facility
should be restricted for the same reasons.

pseries also allows nearly arbitrary device tree changes via
/proc/powerpc/ofdt. Just as overriding ACPI tables is not allowed
while locked down, so should this facility be restricted.

Nathan Lynch (2):
  powerpc/pseries: block untrusted device tree changes when locked down
  powerpc/rtas: block error injection when locked down

 arch/powerpc/kernel/rtas.c                | 25 ++++++++++++++++++++++-
 arch/powerpc/platforms/pseries/reconfig.c |  5 +++++
 include/linux/security.h                  |  2 ++
 security/security.c                       |  2 ++
 4 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.37.3

[PATCH 0/2] powerpc/pseries: restrict error injection and DT changes when locked down

[Nathan Lynch]

Add two new lockdown reasons for use in powerpc's pseries platform
code.

The pseries platform allows hardware-level error injection via certain
calls to the RTAS (Run Time Abstraction Services) firmware. ACPI-based
error injection is already restricted in lockdown; this facility
should be restricted for the same reasons.

pseries also allows nearly arbitrary device tree changes via
/proc/powerpc/ofdt. Just as overriding ACPI tables is not allowed
while locked down, so should this facility be restricted.

Nathan Lynch (2):
  powerpc/pseries: block untrusted device tree changes when locked down
  powerpc/rtas: block error injection when locked down

 arch/powerpc/kernel/rtas.c                | 25 ++++++++++++++++++++++-
 arch/powerpc/platforms/pseries/reconfig.c |  5 +++++
 include/linux/security.h                  |  2 ++
 security/security.c                       |  2 ++
 4 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.37.3

[PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Nathan Lynch]

The /proc/powerpc/ofdt interface allows the root user to freely alter
the in-kernel device tree, enabling arbitrary physical address writes
via drivers that could bind to malicious device nodes, thus making it
possible to disable lockdown.

Historically this interface has been used on the pseries platform to
facilitate the runtime addition and removal of processor, memory, and
device resources (aka Dynamic Logical Partitioning or DLPAR). Years
ago, the processor and memory use cases were migrated to designs that
happen to be lockdown-friendly: device tree updates are communicated
directly to the kernel from firmware without passing through untrusted
user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
remains the sole legitimate user of /proc/powerpc/ofdt, but it is
already broken in lockdown since it uses /dev/mem to allocate argument
buffers for the rtas syscall. So only illegitimate uses of the
interface should see a behavior change when running on a locked down
kernel.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
 include/linux/security.h                  | 1 +
 security/security.c                       | 1 +
 3 files changed, 7 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..599bd2c78514 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <linux/proc_fs.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 
@@ -361,6 +362,10 @@ static ssize_t ofdt_write(struct file *file, const char __user *buf, size_t coun
 	char *kbuf;
 	char *tmp;
 
+	rv = security_locked_down(LOCKDOWN_DEVICE_TREE);
+	if (rv)
+		return rv;
+
 	kbuf = memdup_user_nul(buf, count);
 	if (IS_ERR(kbuf))
 		return PTR_ERR(kbuf);
diff --git a/include/linux/security.h b/include/linux/security.h
index 7bd0c490703d..1ca8dbacd3cc 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -122,6 +122,7 @@ enum lockdown_reason {
 	LOCKDOWN_XMON_WR,
 	LOCKDOWN_BPF_WRITE_USER,
 	LOCKDOWN_DBG_WRITE_KERNEL,
+	LOCKDOWN_DEVICE_TREE,
 	LOCKDOWN_INTEGRITY_MAX,
 	LOCKDOWN_KCORE,
 	LOCKDOWN_KPROBES,
diff --git a/security/security.c b/security/security.c
index 4b95de24bc8d..2863fc31eec6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
 	[LOCKDOWN_XMON_WR] = "xmon write access",
 	[LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
 	[LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
+	[LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
 	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
 	[LOCKDOWN_KCORE] = "/proc/kcore access",
 	[LOCKDOWN_KPROBES] = "use of kprobes",
-- 
2.37.3

[PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Nathan Lynch]

The /proc/powerpc/ofdt interface allows the root user to freely alter
the in-kernel device tree, enabling arbitrary physical address writes
via drivers that could bind to malicious device nodes, thus making it
possible to disable lockdown.

Historically this interface has been used on the pseries platform to
facilitate the runtime addition and removal of processor, memory, and
device resources (aka Dynamic Logical Partitioning or DLPAR). Years
ago, the processor and memory use cases were migrated to designs that
happen to be lockdown-friendly: device tree updates are communicated
directly to the kernel from firmware without passing through untrusted
user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
remains the sole legitimate user of /proc/powerpc/ofdt, but it is
already broken in lockdown since it uses /dev/mem to allocate argument
buffers for the rtas syscall. So only illegitimate uses of the
interface should see a behavior change when running on a locked down
kernel.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
 include/linux/security.h                  | 1 +
 security/security.c                       | 1 +
 3 files changed, 7 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..599bd2c78514 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <linux/proc_fs.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 
@@ -361,6 +362,10 @@ static ssize_t ofdt_write(struct file *file, const char __user *buf, size_t coun
 	char *kbuf;
 	char *tmp;
 
+	rv = security_locked_down(LOCKDOWN_DEVICE_TREE);
+	if (rv)
+		return rv;
+
 	kbuf = memdup_user_nul(buf, count);
 	if (IS_ERR(kbuf))
 		return PTR_ERR(kbuf);
diff --git a/include/linux/security.h b/include/linux/security.h
index 7bd0c490703d..1ca8dbacd3cc 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -122,6 +122,7 @@ enum lockdown_reason {
 	LOCKDOWN_XMON_WR,
 	LOCKDOWN_BPF_WRITE_USER,
 	LOCKDOWN_DBG_WRITE_KERNEL,
+	LOCKDOWN_DEVICE_TREE,
 	LOCKDOWN_INTEGRITY_MAX,
 	LOCKDOWN_KCORE,
 	LOCKDOWN_KPROBES,
diff --git a/security/security.c b/security/security.c
index 4b95de24bc8d..2863fc31eec6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
 	[LOCKDOWN_XMON_WR] = "xmon write access",
 	[LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
 	[LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
+	[LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
 	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
 	[LOCKDOWN_KCORE] = "/proc/kcore access",
 	[LOCKDOWN_KPROBES] = "use of kprobes",
-- 
2.37.3

[PATCH 2/2] powerpc/rtas: block error injection when locked down

[Nathan Lynch]

The error injection facility on pseries VMs allows corruption of
arbitrary guest memory, potentially enabling a sufficiently privileged
user to disable lockdown or perform other modifications of the running
kernel via the rtas syscall.

Block the PAPR error injection facility from being opened or called
when locked down.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
 include/linux/security.h   |  1 +
 security/security.c        |  1 +
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 693133972294..c2540d393f1c 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -23,6 +23,7 @@
 #include <linux/memblock.h>
 #include <linux/slab.h>
 #include <linux/reboot.h>
+#include <linux/security.h>
 #include <linux/syscalls.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
@@ -464,6 +465,9 @@ void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
 	va_end(list);
 }
 
+static int ibm_open_errinjct_token;
+static int ibm_errinjct_token;
+
 int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 {
 	va_list list;
@@ -476,6 +480,16 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
 		return -1;
 
+	if (token == ibm_open_errinjct_token || token == ibm_errinjct_token) {
+		/*
+		 * It would be nicer to not discard the error value
+		 * from security_locked_down(), but callers expect an
+		 * RTAS status, not an errno.
+		 */
+		if (security_locked_down(LOCKDOWN_RTAS_ERROR_INJECTION))
+			return -1;
+	}
+
 	if ((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)) {
 		WARN_ON_ONCE(1);
 		return -1;
@@ -1227,6 +1241,14 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	if (block_rtas_call(token, nargs, &args))
 		return -EINVAL;
 
+	if (token == ibm_open_errinjct_token || token == ibm_errinjct_token) {
+		int err;
+
+		err = security_locked_down(LOCKDOWN_RTAS_ERROR_INJECTION);
+		if (err)
+			return err;
+	}
+
 	/* Need to handle ibm,suspend_me call specially */
 	if (token == rtas_token("ibm,suspend-me")) {
 
@@ -1325,7 +1347,8 @@ void __init rtas_initialize(void)
 #ifdef CONFIG_RTAS_ERROR_LOGGING
 	rtas_last_error_token = rtas_token("rtas-last-error");
 #endif
-
+	ibm_open_errinjct_token = rtas_token("ibm,open-errinjct");
+	ibm_errinjct_token = rtas_token("ibm,errinjct");
 	rtas_syscall_filter_init();
 }
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 1ca8dbacd3cc..b5d5138ae66a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -123,6 +123,7 @@ enum lockdown_reason {
 	LOCKDOWN_BPF_WRITE_USER,
 	LOCKDOWN_DBG_WRITE_KERNEL,
 	LOCKDOWN_DEVICE_TREE,
+	LOCKDOWN_RTAS_ERROR_INJECTION,
 	LOCKDOWN_INTEGRITY_MAX,
 	LOCKDOWN_KCORE,
 	LOCKDOWN_KPROBES,
diff --git a/security/security.c b/security/security.c
index 2863fc31eec6..6518b239ada2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -61,6 +61,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
 	[LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
 	[LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
 	[LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
+	[LOCKDOWN_RTAS_ERROR_INJECTION] = "RTAS error injection",
 	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
 	[LOCKDOWN_KCORE] = "/proc/kcore access",
 	[LOCKDOWN_KPROBES] = "use of kprobes",
-- 
2.37.3

[PATCH 2/2] powerpc/rtas: block error injection when locked down

[Nathan Lynch]

The error injection facility on pseries VMs allows corruption of
arbitrary guest memory, potentially enabling a sufficiently privileged
user to disable lockdown or perform other modifications of the running
kernel via the rtas syscall.

Block the PAPR error injection facility from being opened or called
when locked down.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
 include/linux/security.h   |  1 +
 security/security.c        |  1 +
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 693133972294..c2540d393f1c 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -23,6 +23,7 @@
 #include <linux/memblock.h>
 #include <linux/slab.h>
 #include <linux/reboot.h>
+#include <linux/security.h>
 #include <linux/syscalls.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
@@ -464,6 +465,9 @@ void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret,
 	va_end(list);
 }
 
+static int ibm_open_errinjct_token;
+static int ibm_errinjct_token;
+
 int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 {
 	va_list list;
@@ -476,6 +480,16 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...)
 	if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
 		return -1;
 
+	if (token == ibm_open_errinjct_token || token == ibm_errinjct_token) {
+		/*
+		 * It would be nicer to not discard the error value
+		 * from security_locked_down(), but callers expect an
+		 * RTAS status, not an errno.
+		 */
+		if (security_locked_down(LOCKDOWN_RTAS_ERROR_INJECTION))
+			return -1;
+	}
+
 	if ((mfmsr() & (MSR_IR|MSR_DR)) != (MSR_IR|MSR_DR)) {
 		WARN_ON_ONCE(1);
 		return -1;
@@ -1227,6 +1241,14 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
 	if (block_rtas_call(token, nargs, &args))
 		return -EINVAL;
 
+	if (token == ibm_open_errinjct_token || token == ibm_errinjct_token) {
+		int err;
+
+		err = security_locked_down(LOCKDOWN_RTAS_ERROR_INJECTION);
+		if (err)
+			return err;
+	}
+
 	/* Need to handle ibm,suspend_me call specially */
 	if (token == rtas_token("ibm,suspend-me")) {
 
@@ -1325,7 +1347,8 @@ void __init rtas_initialize(void)
 #ifdef CONFIG_RTAS_ERROR_LOGGING
 	rtas_last_error_token = rtas_token("rtas-last-error");
 #endif
-
+	ibm_open_errinjct_token = rtas_token("ibm,open-errinjct");
+	ibm_errinjct_token = rtas_token("ibm,errinjct");
 	rtas_syscall_filter_init();
 }
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 1ca8dbacd3cc..b5d5138ae66a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -123,6 +123,7 @@ enum lockdown_reason {
 	LOCKDOWN_BPF_WRITE_USER,
 	LOCKDOWN_DBG_WRITE_KERNEL,
 	LOCKDOWN_DEVICE_TREE,
+	LOCKDOWN_RTAS_ERROR_INJECTION,
 	LOCKDOWN_INTEGRITY_MAX,
 	LOCKDOWN_KCORE,
 	LOCKDOWN_KPROBES,
diff --git a/security/security.c b/security/security.c
index 2863fc31eec6..6518b239ada2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -61,6 +61,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
 	[LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
 	[LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
 	[LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
+	[LOCKDOWN_RTAS_ERROR_INJECTION] = "RTAS error injection",
 	[LOCKDOWN_INTEGRITY_MAX] = "integrity",
 	[LOCKDOWN_KCORE] = "/proc/kcore access",
 	[LOCKDOWN_KPROBES] = "use of kprobes",
-- 
2.37.3

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Paul Moore]

On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>
> The /proc/powerpc/ofdt interface allows the root user to freely alter
> the in-kernel device tree, enabling arbitrary physical address writes
> via drivers that could bind to malicious device nodes, thus making it
> possible to disable lockdown.
>
> Historically this interface has been used on the pseries platform to
> facilitate the runtime addition and removal of processor, memory, and
> device resources (aka Dynamic Logical Partitioning or DLPAR). Years
> ago, the processor and memory use cases were migrated to designs that
> happen to be lockdown-friendly: device tree updates are communicated
> directly to the kernel from firmware without passing through untrusted
> user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
> remains the sole legitimate user of /proc/powerpc/ofdt, but it is
> already broken in lockdown since it uses /dev/mem to allocate argument
> buffers for the rtas syscall. So only illegitimate uses of the
> interface should see a behavior change when running on a locked down
> kernel.
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
>  include/linux/security.h                  | 1 +
>  security/security.c                       | 1 +
>  3 files changed, 7 insertions(+)

A couple of small nits below, but in general this seems reasonable.
However, as we are currently at -rc6 I would like us to wait to merge
this until after the upcoming merge window closes (I don't like
merging new functionality into -next at -rc6).

https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/tree/README.md

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..1ca8dbacd3cc 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -122,6 +122,7 @@ enum lockdown_reason {
>         LOCKDOWN_XMON_WR,
>         LOCKDOWN_BPF_WRITE_USER,
>         LOCKDOWN_DBG_WRITE_KERNEL,
> +       LOCKDOWN_DEVICE_TREE,

I would suggest moving LOCKDOWN_DEVICE_TREE to be next to
LOCKDOWN_ACPI_TABLES.  It's not a hard requirement, but it seems like
a nice idea to group similar things when we can.

>         LOCKDOWN_INTEGRITY_MAX,
>         LOCKDOWN_KCORE,
>         LOCKDOWN_KPROBES,
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..2863fc31eec6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>         [LOCKDOWN_XMON_WR] = "xmon write access",
>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
> +       [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",

Might as well move this one too.

>         [LOCKDOWN_INTEGRITY_MAX] = "integrity",
>         [LOCKDOWN_KCORE] = "/proc/kcore access",
>         [LOCKDOWN_KPROBES] = "use of kprobes",
> --
> 2.37.3

-- 
paul-moore.com

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Paul Moore]

On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>
> The /proc/powerpc/ofdt interface allows the root user to freely alter
> the in-kernel device tree, enabling arbitrary physical address writes
> via drivers that could bind to malicious device nodes, thus making it
> possible to disable lockdown.
>
> Historically this interface has been used on the pseries platform to
> facilitate the runtime addition and removal of processor, memory, and
> device resources (aka Dynamic Logical Partitioning or DLPAR). Years
> ago, the processor and memory use cases were migrated to designs that
> happen to be lockdown-friendly: device tree updates are communicated
> directly to the kernel from firmware without passing through untrusted
> user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
> remains the sole legitimate user of /proc/powerpc/ofdt, but it is
> already broken in lockdown since it uses /dev/mem to allocate argument
> buffers for the rtas syscall. So only illegitimate uses of the
> interface should see a behavior change when running on a locked down
> kernel.
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
>  include/linux/security.h                  | 1 +
>  security/security.c                       | 1 +
>  3 files changed, 7 insertions(+)

A couple of small nits below, but in general this seems reasonable.
However, as we are currently at -rc6 I would like us to wait to merge
this until after the upcoming merge window closes (I don't like
merging new functionality into -next at -rc6).

https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/tree/README.md

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 7bd0c490703d..1ca8dbacd3cc 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -122,6 +122,7 @@ enum lockdown_reason {
>         LOCKDOWN_XMON_WR,
>         LOCKDOWN_BPF_WRITE_USER,
>         LOCKDOWN_DBG_WRITE_KERNEL,
> +       LOCKDOWN_DEVICE_TREE,

I would suggest moving LOCKDOWN_DEVICE_TREE to be next to
LOCKDOWN_ACPI_TABLES.  It's not a hard requirement, but it seems like
a nice idea to group similar things when we can.

>         LOCKDOWN_INTEGRITY_MAX,
>         LOCKDOWN_KCORE,
>         LOCKDOWN_KPROBES,
> diff --git a/security/security.c b/security/security.c
> index 4b95de24bc8d..2863fc31eec6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>         [LOCKDOWN_XMON_WR] = "xmon write access",
>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
> +       [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",

Might as well move this one too.

>         [LOCKDOWN_INTEGRITY_MAX] = "integrity",
>         [LOCKDOWN_KCORE] = "/proc/kcore access",
>         [LOCKDOWN_KPROBES] = "use of kprobes",
> --
> 2.37.3

-- 
paul-moore.com

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Paul Moore]

On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>
> The error injection facility on pseries VMs allows corruption of
> arbitrary guest memory, potentially enabling a sufficiently privileged
> user to disable lockdown or perform other modifications of the running
> kernel via the rtas syscall.
>
> Block the PAPR error injection facility from being opened or called
> when locked down.
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>  include/linux/security.h   |  1 +
>  security/security.c        |  1 +
>  3 files changed, 26 insertions(+), 1 deletion(-)

...

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1ca8dbacd3cc..b5d5138ae66a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -123,6 +123,7 @@ enum lockdown_reason {
>         LOCKDOWN_BPF_WRITE_USER,
>         LOCKDOWN_DBG_WRITE_KERNEL,
>         LOCKDOWN_DEVICE_TREE,
> +       LOCKDOWN_RTAS_ERROR_INJECTION,

With the understanding that I've never heard of RTAS until now, are
there any other RTAS events that would require a lockdown reason?  As
a follow up, is it important to distinguish between different RTAS
lockdown reasons?

I'm trying to determine if we can just call it LOCKDOWN_RTAS.

>         LOCKDOWN_INTEGRITY_MAX,
>         LOCKDOWN_KCORE,
>         LOCKDOWN_KPROBES,
> diff --git a/security/security.c b/security/security.c
> index 2863fc31eec6..6518b239ada2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -61,6 +61,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
>         [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
> +       [LOCKDOWN_RTAS_ERROR_INJECTION] = "RTAS error injection",

See above.

>         [LOCKDOWN_INTEGRITY_MAX] = "integrity",
>         [LOCKDOWN_KCORE] = "/proc/kcore access",
>         [LOCKDOWN_KPROBES] = "use of kprobes",
> --
> 2.37.3

-- 
paul-moore.com

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Paul Moore]

On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>
> The error injection facility on pseries VMs allows corruption of
> arbitrary guest memory, potentially enabling a sufficiently privileged
> user to disable lockdown or perform other modifications of the running
> kernel via the rtas syscall.
>
> Block the PAPR error injection facility from being opened or called
> when locked down.
>
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> ---
>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>  include/linux/security.h   |  1 +
>  security/security.c        |  1 +
>  3 files changed, 26 insertions(+), 1 deletion(-)

...

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1ca8dbacd3cc..b5d5138ae66a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -123,6 +123,7 @@ enum lockdown_reason {
>         LOCKDOWN_BPF_WRITE_USER,
>         LOCKDOWN_DBG_WRITE_KERNEL,
>         LOCKDOWN_DEVICE_TREE,
> +       LOCKDOWN_RTAS_ERROR_INJECTION,

With the understanding that I've never heard of RTAS until now, are
there any other RTAS events that would require a lockdown reason?  As
a follow up, is it important to distinguish between different RTAS
lockdown reasons?

I'm trying to determine if we can just call it LOCKDOWN_RTAS.

>         LOCKDOWN_INTEGRITY_MAX,
>         LOCKDOWN_KCORE,
>         LOCKDOWN_KPROBES,
> diff --git a/security/security.c b/security/security.c
> index 2863fc31eec6..6518b239ada2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -61,6 +61,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
>         [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
> +       [LOCKDOWN_RTAS_ERROR_INJECTION] = "RTAS error injection",

See above.

>         [LOCKDOWN_INTEGRITY_MAX] = "integrity",
>         [LOCKDOWN_KCORE] = "/proc/kcore access",
>         [LOCKDOWN_KPROBES] = "use of kprobes",
> --
> 2.37.3

-- 
paul-moore.com

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Michael Ellerman]

Paul Moore <paul@paul-moore.com> writes:
> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>
>> The /proc/powerpc/ofdt interface allows the root user to freely alter
>> the in-kernel device tree, enabling arbitrary physical address writes
>> via drivers that could bind to malicious device nodes, thus making it
>> possible to disable lockdown.
>>
>> Historically this interface has been used on the pseries platform to
>> facilitate the runtime addition and removal of processor, memory, and
>> device resources (aka Dynamic Logical Partitioning or DLPAR). Years
>> ago, the processor and memory use cases were migrated to designs that
>> happen to be lockdown-friendly: device tree updates are communicated
>> directly to the kernel from firmware without passing through untrusted
>> user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
>> remains the sole legitimate user of /proc/powerpc/ofdt, but it is
>> already broken in lockdown since it uses /dev/mem to allocate argument
>> buffers for the rtas syscall. So only illegitimate uses of the
>> interface should see a behavior change when running on a locked down
>> kernel.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
>>  include/linux/security.h                  | 1 +
>>  security/security.c                       | 1 +
>>  3 files changed, 7 insertions(+)
>
> A couple of small nits below, but in general this seems reasonable.
> However, as we are currently at -rc6 I would like us to wait to merge
> this until after the upcoming merge window closes (I don't like
> merging new functionality into -next at -rc6).

It's a bug fix, not a new feature IMHO.

I'd like to take it via the powerpc tree.

cheers

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Michael Ellerman]

Paul Moore <paul@paul-moore.com> writes:
> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>
>> The /proc/powerpc/ofdt interface allows the root user to freely alter
>> the in-kernel device tree, enabling arbitrary physical address writes
>> via drivers that could bind to malicious device nodes, thus making it
>> possible to disable lockdown.
>>
>> Historically this interface has been used on the pseries platform to
>> facilitate the runtime addition and removal of processor, memory, and
>> device resources (aka Dynamic Logical Partitioning or DLPAR). Years
>> ago, the processor and memory use cases were migrated to designs that
>> happen to be lockdown-friendly: device tree updates are communicated
>> directly to the kernel from firmware without passing through untrusted
>> user space. I/O device DLPAR via the "drmgr" command in powerpc-utils
>> remains the sole legitimate user of /proc/powerpc/ofdt, but it is
>> already broken in lockdown since it uses /dev/mem to allocate argument
>> buffers for the rtas syscall. So only illegitimate uses of the
>> interface should see a behavior change when running on a locked down
>> kernel.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/reconfig.c | 5 +++++
>>  include/linux/security.h                  | 1 +
>>  security/security.c                       | 1 +
>>  3 files changed, 7 insertions(+)
>
> A couple of small nits below, but in general this seems reasonable.
> However, as we are currently at -rc6 I would like us to wait to merge
> this until after the upcoming merge window closes (I don't like
> merging new functionality into -next at -rc6).

It's a bug fix, not a new feature IMHO.

I'd like to take it via the powerpc tree.

cheers

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Michael Ellerman]

Paul Moore <paul@paul-moore.com> writes:
> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>
>> The error injection facility on pseries VMs allows corruption of
>> arbitrary guest memory, potentially enabling a sufficiently privileged
>> user to disable lockdown or perform other modifications of the running
>> kernel via the rtas syscall.
>>
>> Block the PAPR error injection facility from being opened or called
>> when locked down.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>>  include/linux/security.h   |  1 +
>>  security/security.c        |  1 +
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> ...
>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 1ca8dbacd3cc..b5d5138ae66a 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -123,6 +123,7 @@ enum lockdown_reason {
>>         LOCKDOWN_BPF_WRITE_USER,
>>         LOCKDOWN_DBG_WRITE_KERNEL,
>>         LOCKDOWN_DEVICE_TREE,
>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
>
> With the understanding that I've never heard of RTAS until now, are
> there any other RTAS events that would require a lockdown reason?  As
> a follow up, is it important to distinguish between different RTAS
> lockdown reasons?
>
> I'm trying to determine if we can just call it LOCKDOWN_RTAS.

Yes I think we should.

Currently it only locks down the error injection calls, not all of RTAS.

But firmware can/will add new RTAS calls in future, so it's always
possible something will need to be added to the list of things that need
to be blocked during lockdown.

So I think calling it LOCKDOWN_RTAS would be more general and future
proof, and can be read to mean "lockdown the parts of RTAS that need
to be locked down".

Similarly we have LOCKDOWN_ACPI_TABLES which locks down modification to
ACPI data, but doesn't disable ACPI use entirely AIUI.

cheers

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Michael Ellerman]

Paul Moore <paul@paul-moore.com> writes:
> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>
>> The error injection facility on pseries VMs allows corruption of
>> arbitrary guest memory, potentially enabling a sufficiently privileged
>> user to disable lockdown or perform other modifications of the running
>> kernel via the rtas syscall.
>>
>> Block the PAPR error injection facility from being opened or called
>> when locked down.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>>  include/linux/security.h   |  1 +
>>  security/security.c        |  1 +
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> ...
>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 1ca8dbacd3cc..b5d5138ae66a 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -123,6 +123,7 @@ enum lockdown_reason {
>>         LOCKDOWN_BPF_WRITE_USER,
>>         LOCKDOWN_DBG_WRITE_KERNEL,
>>         LOCKDOWN_DEVICE_TREE,
>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
>
> With the understanding that I've never heard of RTAS until now, are
> there any other RTAS events that would require a lockdown reason?  As
> a follow up, is it important to distinguish between different RTAS
> lockdown reasons?
>
> I'm trying to determine if we can just call it LOCKDOWN_RTAS.

Yes I think we should.

Currently it only locks down the error injection calls, not all of RTAS.

But firmware can/will add new RTAS calls in future, so it's always
possible something will need to be added to the list of things that need
to be blocked during lockdown.

So I think calling it LOCKDOWN_RTAS would be more general and future
proof, and can be read to mean "lockdown the parts of RTAS that need
to be locked down".

Similarly we have LOCKDOWN_ACPI_TABLES which locks down modification to
ACPI data, but doesn't disable ACPI use entirely AIUI.

cheers

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Nathan Lynch]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Paul Moore <paul@paul-moore.com> writes:
>> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>>
>>> The error injection facility on pseries VMs allows corruption of
>>> arbitrary guest memory, potentially enabling a sufficiently privileged
>>> user to disable lockdown or perform other modifications of the running
>>> kernel via the rtas syscall.
>>>
>>> Block the PAPR error injection facility from being opened or called
>>> when locked down.
>>>
>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>> ---
>>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>>>  include/linux/security.h   |  1 +
>>>  security/security.c        |  1 +
>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> ...
>>
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 1ca8dbacd3cc..b5d5138ae66a 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -123,6 +123,7 @@ enum lockdown_reason {
>>>         LOCKDOWN_BPF_WRITE_USER,
>>>         LOCKDOWN_DBG_WRITE_KERNEL,
>>>         LOCKDOWN_DEVICE_TREE,
>>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
>>
>> With the understanding that I've never heard of RTAS until now, are
>> there any other RTAS events that would require a lockdown reason?  As
>> a follow up, is it important to distinguish between different RTAS
>> lockdown reasons?

1. Not to my current knowledge.
2. Yes, I think so, see below.

>>
>> I'm trying to determine if we can just call it LOCKDOWN_RTAS.
>
> Yes I think we should.
>
> Currently it only locks down the error injection calls, not all of RTAS.
>
> But firmware can/will add new RTAS calls in future, so it's always
> possible something will need to be added to the list of things that need
> to be blocked during lockdown.
>
> So I think calling it LOCKDOWN_RTAS would be more general and future
> proof, and can be read to mean "lockdown the parts of RTAS that need
> to be locked down".

RTAS provides callable interfaces for a broad variety of functions,
including device configuration, halt/reboot/suspend, CPU online/offline,
NVRAM access, firmware upgrade, platform diagnostic data retrieval, and
others.

Currently I don't know of other RTAS-provided functions that should be
restricted. But if we were to add more, then -- in answer to Paul -- yes
I think it would be important to have distinct reasons and
messages. Taking the point of view of someone diagnosing lockdown denial
messages and relating them to kernel code and user space activity, I
would rather we err toward specificity.

Also: LOCKDOWN_RTAS_ERROR_INJECTION is proposed for lockdown's integrity
mode. But consider that future RTAS-related additions could be proposed
for confidentiality mode, which is more restrictive. (A plausible
example could be platform dump retrieval.) We would need at least two
RTAS reasons and one wouldn't suffice.

So a single RTAS catch-all lockdown reason doesn't appeal to me, but
lockdown reasons and messages aren't ABI (right?) and we could
eventually change whatever decision we reach here. So if you both still
prefer a single LOCKDOWN_RTAS reason, I can do that for v2.

That said, I could see a case for instead adding
LOCKDOWN_HW_ERROR_INJECTION (without the RTAS), to indicate restriction
of hardware- or platform-level error injection. I was a little surprised
to find that an error injection reason doesn't already exist for the
ACPI-based facility (drivers/acpi/apei/einj.c), but since the user
interface is debugfs-based I suppose it's already restricted in practice
by LOCKDOWN_DEBUGFS.

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Nathan Lynch]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Paul Moore <paul@paul-moore.com> writes:
>> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>>>
>>> The error injection facility on pseries VMs allows corruption of
>>> arbitrary guest memory, potentially enabling a sufficiently privileged
>>> user to disable lockdown or perform other modifications of the running
>>> kernel via the rtas syscall.
>>>
>>> Block the PAPR error injection facility from being opened or called
>>> when locked down.
>>>
>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>> ---
>>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
>>>  include/linux/security.h   |  1 +
>>>  security/security.c        |  1 +
>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> ...
>>
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 1ca8dbacd3cc..b5d5138ae66a 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -123,6 +123,7 @@ enum lockdown_reason {
>>>         LOCKDOWN_BPF_WRITE_USER,
>>>         LOCKDOWN_DBG_WRITE_KERNEL,
>>>         LOCKDOWN_DEVICE_TREE,
>>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
>>
>> With the understanding that I've never heard of RTAS until now, are
>> there any other RTAS events that would require a lockdown reason?  As
>> a follow up, is it important to distinguish between different RTAS
>> lockdown reasons?

1. Not to my current knowledge.
2. Yes, I think so, see below.

>>
>> I'm trying to determine if we can just call it LOCKDOWN_RTAS.
>
> Yes I think we should.
>
> Currently it only locks down the error injection calls, not all of RTAS.
>
> But firmware can/will add new RTAS calls in future, so it's always
> possible something will need to be added to the list of things that need
> to be blocked during lockdown.
>
> So I think calling it LOCKDOWN_RTAS would be more general and future
> proof, and can be read to mean "lockdown the parts of RTAS that need
> to be locked down".

RTAS provides callable interfaces for a broad variety of functions,
including device configuration, halt/reboot/suspend, CPU online/offline,
NVRAM access, firmware upgrade, platform diagnostic data retrieval, and
others.

Currently I don't know of other RTAS-provided functions that should be
restricted. But if we were to add more, then -- in answer to Paul -- yes
I think it would be important to have distinct reasons and
messages. Taking the point of view of someone diagnosing lockdown denial
messages and relating them to kernel code and user space activity, I
would rather we err toward specificity.

Also: LOCKDOWN_RTAS_ERROR_INJECTION is proposed for lockdown's integrity
mode. But consider that future RTAS-related additions could be proposed
for confidentiality mode, which is more restrictive. (A plausible
example could be platform dump retrieval.) We would need at least two
RTAS reasons and one wouldn't suffice.

So a single RTAS catch-all lockdown reason doesn't appeal to me, but
lockdown reasons and messages aren't ABI (right?) and we could
eventually change whatever decision we reach here. So if you both still
prefer a single LOCKDOWN_RTAS reason, I can do that for v2.

That said, I could see a case for instead adding
LOCKDOWN_HW_ERROR_INJECTION (without the RTAS), to indicate restriction
of hardware- or platform-level error injection. I was a little surprised
to find that an error injection reason doesn't already exist for the
ACPI-based facility (drivers/acpi/apei/einj.c), but since the user
interface is debugfs-based I suppose it's already restricted in practice
by LOCKDOWN_DEBUGFS.

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Nathan Lynch]

Paul Moore <paul@paul-moore.com> writes:
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 7bd0c490703d..1ca8dbacd3cc 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -122,6 +122,7 @@ enum lockdown_reason {
>>         LOCKDOWN_XMON_WR,
>>         LOCKDOWN_BPF_WRITE_USER,
>>         LOCKDOWN_DBG_WRITE_KERNEL,
>> +       LOCKDOWN_DEVICE_TREE,
>
> I would suggest moving LOCKDOWN_DEVICE_TREE to be next to
> LOCKDOWN_ACPI_TABLES.  It's not a hard requirement, but it seems like
> a nice idea to group similar things when we can.
>
>>         LOCKDOWN_INTEGRITY_MAX,
>>         LOCKDOWN_KCORE,
>>         LOCKDOWN_KPROBES,
>> diff --git a/security/security.c b/security/security.c
>> index 4b95de24bc8d..2863fc31eec6 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>>         [LOCKDOWN_XMON_WR] = "xmon write access",
>>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
>> +       [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
>
> Might as well move this one too.

Yes, I can do that for v2. Thanks.

Re: [PATCH 1/2] powerpc/pseries: block untrusted device tree changes when locked down

[Nathan Lynch]

Paul Moore <paul@paul-moore.com> writes:
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 7bd0c490703d..1ca8dbacd3cc 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -122,6 +122,7 @@ enum lockdown_reason {
>>         LOCKDOWN_XMON_WR,
>>         LOCKDOWN_BPF_WRITE_USER,
>>         LOCKDOWN_DBG_WRITE_KERNEL,
>> +       LOCKDOWN_DEVICE_TREE,
>
> I would suggest moving LOCKDOWN_DEVICE_TREE to be next to
> LOCKDOWN_ACPI_TABLES.  It's not a hard requirement, but it seems like
> a nice idea to group similar things when we can.
>
>>         LOCKDOWN_INTEGRITY_MAX,
>>         LOCKDOWN_KCORE,
>>         LOCKDOWN_KPROBES,
>> diff --git a/security/security.c b/security/security.c
>> index 4b95de24bc8d..2863fc31eec6 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -60,6 +60,7 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>>         [LOCKDOWN_XMON_WR] = "xmon write access",
>>         [LOCKDOWN_BPF_WRITE_USER] = "use of bpf to write user RAM",
>>         [LOCKDOWN_DBG_WRITE_KERNEL] = "use of kgdb/kdb to write kernel RAM",
>> +       [LOCKDOWN_DEVICE_TREE] = "modifying device tree contents",
>
> Might as well move this one too.

Yes, I can do that for v2. Thanks.

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Paul Moore]

On Fri, Sep 23, 2022 at 11:40 AM Nathan Lynch <nathanl@linux.ibm.com> wrote:
> Michael Ellerman <mpe@ellerman.id.au> writes:
> > Paul Moore <paul@paul-moore.com> writes:
> >> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
> >>>
> >>> The error injection facility on pseries VMs allows corruption of
> >>> arbitrary guest memory, potentially enabling a sufficiently privileged
> >>> user to disable lockdown or perform other modifications of the running
> >>> kernel via the rtas syscall.
> >>>
> >>> Block the PAPR error injection facility from being opened or called
> >>> when locked down.
> >>>
> >>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> >>> ---
> >>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
> >>>  include/linux/security.h   |  1 +
> >>>  security/security.c        |  1 +
> >>>  3 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> ...
> >>
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 1ca8dbacd3cc..b5d5138ae66a 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -123,6 +123,7 @@ enum lockdown_reason {
> >>>         LOCKDOWN_BPF_WRITE_USER,
> >>>         LOCKDOWN_DBG_WRITE_KERNEL,
> >>>         LOCKDOWN_DEVICE_TREE,
> >>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
> >>
> >> With the understanding that I've never heard of RTAS until now, are
> >> there any other RTAS events that would require a lockdown reason?  As
> >> a follow up, is it important to distinguish between different RTAS
> >> lockdown reasons?
>
> 1. Not to my current knowledge.
> 2. Yes, I think so, see below.
>
> >>
> >> I'm trying to determine if we can just call it LOCKDOWN_RTAS.
> >
> > Yes I think we should.
> >
> > Currently it only locks down the error injection calls, not all of RTAS.
> >
> > But firmware can/will add new RTAS calls in future, so it's always
> > possible something will need to be added to the list of things that need
> > to be blocked during lockdown.
> >
> > So I think calling it LOCKDOWN_RTAS would be more general and future
> > proof, and can be read to mean "lockdown the parts of RTAS that need
> > to be locked down".
>
> RTAS provides callable interfaces for a broad variety of functions,
> including device configuration, halt/reboot/suspend, CPU online/offline,
> NVRAM access, firmware upgrade, platform diagnostic data retrieval, and
> others.
>
> Currently I don't know of other RTAS-provided functions that should be
> restricted. But if we were to add more, then -- in answer to Paul -- yes
> I think it would be important to have distinct reasons and
> messages. Taking the point of view of someone diagnosing lockdown denial
> messages and relating them to kernel code and user space activity, I
> would rather we err toward specificity.

As I said before, RTAS is a great mystery to me, if it can be extended
in the future then having a targeted lockdown name makes perfect
sense.

> So a single RTAS catch-all lockdown reason doesn't appeal to me, but
> lockdown reasons and messages aren't ABI (right?) ...

Correct.  Or at least that is my understanding, but there have been
some odd rulings on lockdown in the past so my advice would be to make
*very* sure you get this right the first time.  From what you and
Michael have said, it seems like a function specific name is the way
to go here, and based on your explanations of the situation it seems
like putting this in the integrity bin is the right way to go.

-- 
paul-moore.com

Re: [PATCH 2/2] powerpc/rtas: block error injection when locked down

[Paul Moore]

On Fri, Sep 23, 2022 at 11:40 AM Nathan Lynch <nathanl@linux.ibm.com> wrote:
> Michael Ellerman <mpe@ellerman.id.au> writes:
> > Paul Moore <paul@paul-moore.com> writes:
> >> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
> >>>
> >>> The error injection facility on pseries VMs allows corruption of
> >>> arbitrary guest memory, potentially enabling a sufficiently privileged
> >>> user to disable lockdown or perform other modifications of the running
> >>> kernel via the rtas syscall.
> >>>
> >>> Block the PAPR error injection facility from being opened or called
> >>> when locked down.
> >>>
> >>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> >>> ---
> >>>  arch/powerpc/kernel/rtas.c | 25 ++++++++++++++++++++++++-
> >>>  include/linux/security.h   |  1 +
> >>>  security/security.c        |  1 +
> >>>  3 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> ...
> >>
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 1ca8dbacd3cc..b5d5138ae66a 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -123,6 +123,7 @@ enum lockdown_reason {
> >>>         LOCKDOWN_BPF_WRITE_USER,
> >>>         LOCKDOWN_DBG_WRITE_KERNEL,
> >>>         LOCKDOWN_DEVICE_TREE,
> >>> +       LOCKDOWN_RTAS_ERROR_INJECTION,
> >>
> >> With the understanding that I've never heard of RTAS until now, are
> >> there any other RTAS events that would require a lockdown reason?  As
> >> a follow up, is it important to distinguish between different RTAS
> >> lockdown reasons?
>
> 1. Not to my current knowledge.
> 2. Yes, I think so, see below.
>
> >>
> >> I'm trying to determine if we can just call it LOCKDOWN_RTAS.
> >
> > Yes I think we should.
> >
> > Currently it only locks down the error injection calls, not all of RTAS.
> >
> > But firmware can/will add new RTAS calls in future, so it's always
> > possible something will need to be added to the list of things that need
> > to be blocked during lockdown.
> >
> > So I think calling it LOCKDOWN_RTAS would be more general and future
> > proof, and can be read to mean "lockdown the parts of RTAS that need
> > to be locked down".
>
> RTAS provides callable interfaces for a broad variety of functions,
> including device configuration, halt/reboot/suspend, CPU online/offline,
> NVRAM access, firmware upgrade, platform diagnostic data retrieval, and
> others.
>
> Currently I don't know of other RTAS-provided functions that should be
> restricted. But if we were to add more, then -- in answer to Paul -- yes
> I think it would be important to have distinct reasons and
> messages. Taking the point of view of someone diagnosing lockdown denial
> messages and relating them to kernel code and user space activity, I
> would rather we err toward specificity.

As I said before, RTAS is a great mystery to me, if it can be extended
in the future then having a targeted lockdown name makes perfect
sense.

> So a single RTAS catch-all lockdown reason doesn't appeal to me, but
> lockdown reasons and messages aren't ABI (right?) ...

Correct.  Or at least that is my understanding, but there have been
some odd rulings on lockdown in the past so my advice would be to make
*very* sure you get this right the first time.  From what you and
Michael have said, it seems like a function specific name is the way
to go here, and based on your explanations of the situation it seems
like putting this in the integrity bin is the right way to go.

-- 
paul-moore.com