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 <[email protected]>
---
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
On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <[email protected]> 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 <[email protected]>
> ---
> 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
Paul Moore <[email protected]> writes:
> On Thu, Sep 22, 2022 at 3:38 PM Nathan Lynch <[email protected]> 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 <[email protected]>
>> ---
>> 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
Paul Moore <[email protected]> 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.