[v2,1/2] powerpc/rtas: Restrict RTAS requests from userspace

A number of userspace utilities depend on making calls to RTAS to retrieve
information and update various things.

The existing API through which we expose RTAS to userspace exposes more
RTAS functionality than we actually need, through the sys_rtas syscall,
which allows root (or anyone with CAP_SYS_ADMIN) to make any RTAS call they
want with arbitrary arguments.

Many RTAS calls take the address of a buffer as an argument, and it's up to
the caller to specify the physical address of the buffer as an argument. We
allocate a buffer (the "RMO buffer") in the Real Memory Area that RTAS can
access, and then expose the physical address and size of this buffer in
/proc/powerpc/rtas/rmo_buffer. Userspace is expected to read this address,
poke at the buffer using /dev/mem, and pass an address in the RMO buffer to
the RTAS call.

However, there's nothing stopping the caller from specifying whatever
address they want in the RTAS call, and it's easy to construct a series of
RTAS calls that can overwrite arbitrary bytes (even without /dev/mem
access).

Additionally, there are some RTAS calls that do potentially dangerous
things and for which there are no legitimate userspace use cases.

In the past, this would not have been a particularly big deal as it was
assumed that root could modify all system state freely, but with Secure
Boot and lockdown we need to care about this.

We can't fundamentally change the ABI at this point, however we can address
this by implementing a filter that checks RTAS calls against a list
of permitted calls and forces the caller to use addresses within the RMO
buffer.

The list is based off the list of calls that are used by the librtas
userspace library, and has been tested with a number of existing userspace
RTAS utilities. For compatibility with any applications we are not aware of
that require other calls, the filter can be turned off at build time.

Reported-by: Daniel Axtens <dja@axtens.net>
Cc: stable@vger.kernel.org
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>

---
v1->v2:
- address comments from mpe
- shorten the names of some struct members
- make the filter array static/ro_after_init, use const char *
- genericise the fixed buffer size cases
- simplify/get rid of some of the error printing
- get rid of rtas_token_name()
---
 arch/powerpc/Kconfig       |  13 ++++
 arch/powerpc/kernel/rtas.c | 153 +++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.8.2, v5.7.16, v5.4.59, v4.19.140, v4.14.193, v4.9.232, v4.4.232.

v5.8.2: Build OK!
v5.7.16: Build OK!
v5.4.59: Failed to apply! Possible dependencies:
    1a8916ee3ac2 ("powerpc: Detect the secure boot mode of the system")
    4238fad366a6 ("powerpc/ima: Add support to initialize ima policy rules")
    9155e2341aa8 ("powerpc/powernv: Add OPAL API interface to access secure variable")
    bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs")

v4.19.140: Failed to apply! Possible dependencies:
    0261a508c9fc ("powerpc/mm: dump segment registers on book3s/32")
    136bc0397ae2 ("powerpc/pseries: Introduce option to build secure virtual machines")
    1a8916ee3ac2 ("powerpc: Detect the secure boot mode of the system")
    75d9fc7fd94e ("powerpc/powernv: move OPAL call wrapper tracing and interrupt handling to C")
    7c91efce1608 ("powerpc/mm: dump block address translation on book3s/32")
    97026b5a5ac2 ("powerpc/mm: Split dump_pagelinuxtables flag_array table")
    a49dddbdb0cc ("powerpc/kernel: Add ucall_norets() ultravisor call handler")
    b2133bd7a553 ("powerpc/book3s/32: do not include pte-common.h")
    bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs")
    cbcbbf4afd6d ("powerpc/mm: Define platform default caches related flags")
    d81e6f8b7c66 ("powerpc/mm: don't use _PAGE_EXEC in book3s/32")
    d82fd29c5a8c ("powerpc/mm: Distribute platform specific PAGE and PMD flags and definitions")
    e66c3209c7fd ("powerpc: Move page table dump files in a dedicated subdirectory")
    fb0b0a73b223 ("powerpc: Enable kcov")
    ff00552578ba ("powerpc/8xx: change name of a few page flags to avoid confusion")

v4.14.193: Failed to apply! Possible dependencies:
    136bc0397ae2 ("powerpc/pseries: Introduce option to build secure virtual machines")
    1a8916ee3ac2 ("powerpc: Detect the secure boot mode of the system")
    4e56207130ed ("kbuild: Cache a few more calls to the compiler")
    4fa8bc949de1 ("kbuild: rename *-asn1.[ch] to *.asn1.[ch]")
    74ce1896c6c6 ("kbuild: clean up *.dtb and *.dtb.S patterns from top-level Makefile")
    75d9fc7fd94e ("powerpc/powernv: move OPAL call wrapper tracing and interrupt handling to C")
    8438ee76b004 ("Makefile: disable PIE before testing asm goto")
    8f2133cc0e1f ("powerpc/pseries: hcall_exit tracepoint retval should be signed")
    92e3da3cf193 ("powerpc: initial pkey plumbing")
    9a8dfb394c04 ("kbuild: clean up *.lex.c and *.tab.[ch] patterns from top-level Makefile")
    9ce285cfe360 (".gitignore: move *-asn1.[ch] patterns to the top-level .gitignore")
    a49dddbdb0cc ("powerpc/kernel: Add ucall_norets() ultravisor call handler")
    bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs")
    c64ba044ed57 ("kbuild: gcov: enable -fno-tree-loop-im if supported")
    d677a4d60193 ("Makefile: support flag -fsanitizer-coverage=trace-cmp")
    d682026dd3c5 (".gitignore: ignore ASN.1 auto generated files")
    e08d6de4e532 ("kbuild: remove kbuild cache")
    e501ce957a78 ("x86: Force asm-goto")
    e9666d10a567 ("jump_label: move 'asm goto' support test to Kconfig")
    ef46d9b3dc01 ("kbuild: clean up *.i and *.lst patterns by make clean")

v4.9.232: Failed to apply! Possible dependencies:
    1515ab932156 ("powerpc/mm: Dump hash table")
    1a8916ee3ac2 ("powerpc: Detect the secure boot mode of the system")
    6cc89bad60a6 ("powerpc/kprobes: Invoke handlers directly")
    7644d5819cf8 ("powerpc: Create asm/debugfs.h and move powerpc_debugfs_root there")
    7c0f6ba682b9 ("Replace <asm/uaccess.h> with <linux/uaccess.h> globally")
    8eb07b187000 ("powerpc/mm: Dump linux pagetables")
    92e3da3cf193 ("powerpc: initial pkey plumbing")
    bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs")
    da6658859b9c ("powerpc: Change places using CONFIG_KEXEC to use CONFIG_KEXEC_CORE instead.")
    dd5ac03e0955 ("powerpc/mm: Fix page table dump build on non-Book3S")

v4.4.232: Failed to apply! Possible dependencies:
    019132ff3daf ("x86/mm/pkeys: Fill in pkey field in siginfo")
    0e749e54244e ("dax: increase granularity of dax_clear_blocks() operations")
    1a8916ee3ac2 ("powerpc: Detect the secure boot mode of the system")
    33a709b25a76 ("mm/gup, x86/mm/pkeys: Check VMAs and PTEs for protection keys")
    34c0fd540e79 ("mm, dax, pmem: introduce pfn_t")
    3565fce3a659 ("mm, x86: get_user_pages() for dax mappings")
    52db400fcd50 ("pmem, dax: clean up clear_pmem()")
    5c1d90f51027 ("x86/mm/pkeys: Add PTE bits for storing protection key")
    63c17fb8e5a4 ("mm/core, x86/mm/pkeys: Store protection bits in high VMA flags")
    69660fd797c3 ("x86, mm: introduce _PAGE_DEVMAP")
    7b2d0dbac489 ("x86/mm/pkeys: Pass VMA down in to fault signal generation code")
    8f62c883222c ("x86/mm/pkeys: Add arch-specific VMA protection bits")
    92e3da3cf193 ("powerpc: initial pkey plumbing")
    b2e0d1625e19 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
    b95f5f4391fa ("libnvdimm: convert to statically allocated badblocks")
    bd5d9c743d38 ("powerpc: expose secure variables to userspace via sysfs")
    f25748e3c34e ("mm, dax: convert vmf_insert_pfn_pmd() to pfn_t")
    fe683adabfe6 ("dax: guarantee page aligned results from bdev_direct_access()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

On Thu, 20 Aug 2020 14:45:12 +1000, Andrew Donnellan wrote:
> A number of userspace utilities depend on making calls to RTAS to retrieve
> information and update various things.
> 
> The existing API through which we expose RTAS to userspace exposes more
> RTAS functionality than we actually need, through the sys_rtas syscall,
> which allows root (or anyone with CAP_SYS_ADMIN) to make any RTAS call they
> want with arbitrary arguments.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/rtas: Restrict RTAS requests from userspace
      https://git.kernel.org/powerpc/c/bd59380c5ba4147dcbaad3e582b55ccfd120b764
[2/2] selftests/powerpc: Add a rtas_filter selftest
      https://git.kernel.org/powerpc/c/dc9af82ea0614bb138705d1f5230d53b3b1dfb83

cheers

On 26/8/20 11:53 pm, Sasha Levin wrote:
> How should we proceed with this patch?

mpe: I believe we came to the conclusion that we shouldn't put this in 
stable just yet?

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On 26/8/20 11:53 pm, Sasha Levin wrote:
>> How should we proceed with this patch?
>
> mpe: I believe we came to the conclusion that we shouldn't put this in 
> stable just yet?

Yeah.

Let's give it a little time to get some wider testing before we backport
it.

cheers

Michael Ellerman <mpe@ellerman.id.au> writes:
> Andrew Donnellan <ajd@linux.ibm.com> writes:
>> On 26/8/20 11:53 pm, Sasha Levin wrote:
>>> How should we proceed with this patch?
>>
>> mpe: I believe we came to the conclusion that we shouldn't put this in 
>> stable just yet?
>
> Yeah.
>
> Let's give it a little time to get some wider testing before we backport
> it.

So my fault for not dropping the Cc: stable on the commit, sorry.

cheers

On 8/19/20 9:45 PM, Andrew Donnellan wrote:
> A number of userspace utilities depend on making calls to RTAS to retrieve
> information and update various things.
> 
> The existing API through which we expose RTAS to userspace exposes more
> RTAS functionality than we actually need, through the sys_rtas syscall,
> which allows root (or anyone with CAP_SYS_ADMIN) to make any RTAS call they
> want with arbitrary arguments.
> 
> Many RTAS calls take the address of a buffer as an argument, and it's up to
> the caller to specify the physical address of the buffer as an argument. We
> allocate a buffer (the "RMO buffer") in the Real Memory Area that RTAS can
> access, and then expose the physical address and size of this buffer in
> /proc/powerpc/rtas/rmo_buffer. Userspace is expected to read this address,
> poke at the buffer using /dev/mem, and pass an address in the RMO buffer to
> the RTAS call.
> 
> However, there's nothing stopping the caller from specifying whatever
> address they want in the RTAS call, and it's easy to construct a series of
> RTAS calls that can overwrite arbitrary bytes (even without /dev/mem
> access).
> 
> Additionally, there are some RTAS calls that do potentially dangerous
> things and for which there are no legitimate userspace use cases.
> 
> In the past, this would not have been a particularly big deal as it was
> assumed that root could modify all system state freely, but with Secure
> Boot and lockdown we need to care about this.
> 
> We can't fundamentally change the ABI at this point, however we can address
> this by implementing a filter that checks RTAS calls against a list
> of permitted calls and forces the caller to use addresses within the RMO
> buffer.
> 
> The list is based off the list of calls that are used by the librtas
> userspace library, and has been tested with a number of existing userspace
> RTAS utilities. For compatibility with any applications we are not aware of
> that require other calls, the filter can be turned off at build time.
> 
> Reported-by: Daniel Axtens <dja@axtens.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> 
> ---
> v1->v2:
> - address comments from mpe
> - shorten the names of some struct members
> - make the filter array static/ro_after_init, use const char *
> - genericise the fixed buffer size cases
> - simplify/get rid of some of the error printing
> - get rid of rtas_token_name()
> ---
>  arch/powerpc/Kconfig       |  13 ++++
>  arch/powerpc/kernel/rtas.c | 153 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 166 insertions(+)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 1f48bbfb3ce9..8dd42b82379b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -989,6 +989,19 @@ config PPC_SECVAR_SYSFS
>  	  read/write operations on these variables. Say Y if you have
>  	  secure boot enabled and want to expose variables to userspace.
> 
> +config PPC_RTAS_FILTER
> +	bool "Enable filtering of RTAS syscalls"
> +	default y
> +	depends on PPC_RTAS
> +	help
> +	  The RTAS syscall API has security issues that could be used to
> +	  compromise system integrity. This option enforces restrictions on the
> +	  RTAS calls and arguments passed by userspace programs to mitigate
> +	  these issues.
> +
> +	  Say Y unless you know what you are doing and the filter is causing
> +	  problems for you.
> +
>  endmenu
> 
>  config ISA_DMA_API
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 806d554ce357..954f41676f69 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -992,6 +992,147 @@ struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
>  	return NULL;
>  }
> 
> +#ifdef CONFIG_PPC_RTAS_FILTER
> +
> +/*
> + * The sys_rtas syscall, as originally designed, allows root to pass
> + * arbitrary physical addresses to RTAS calls. A number of RTAS calls
> + * can be abused to write to arbitrary memory and do other things that
> + * are potentially harmful to system integrity, and thus should only
> + * be used inside the kernel and not exposed to userspace.
> + *
> + * All known legitimate users of the sys_rtas syscall will only ever
> + * pass addresses that fall within the RMO buffer, and use a known
> + * subset of RTAS calls.
> + *
> + * Accordingly, we filter RTAS requests to check that the call is
> + * permitted, and that provided pointers fall within the RMO buffer.
> + * The rtas_filters list contains an entry for each permitted call,
> + * with the indexes of the parameters which are expected to contain
> + * addresses and sizes of buffers allocated inside the RMO buffer.
> + */
> +struct rtas_filter {
> +	const char *name;
> +	int token;
> +	/* Indexes into the args buffer, -1 if not used */
> +	int buf_idx1;
> +	int size_idx1;
> +	int buf_idx2;
> +	int size_idx2;
> +
> +	int fixed_size;
> +};
> +
> +static struct rtas_filter rtas_filters[] __ro_after_init = {
> +	{ "ibm,activate-firmware", -1, -1, -1, -1, -1 },
> +	{ "ibm,configure-connector", -1, 0, -1, 1, -1, 4096 },	/* Special cased */
> +	{ "display-character", -1, -1, -1, -1, -1 },
> +	{ "ibm,display-message", -1, 0, -1, -1, -1 },
> +	{ "ibm,errinjct", -1, 2, -1, -1, -1, 1024 },
> +	{ "ibm,close-errinjct", -1, -1, -1, -1, -1 },
> +	{ "ibm,open-errinct", -1, -1, -1, -1, -1 },

There is a typo here. Should be ibm,open-errinjct.

kernel: [ 1100.408626] sys_rtas: RTAS call blocked - exploit attempt?
kernel: [ 1100.408631] sys_rtas: token=0x26, nargs=0 (called by errinjct)

Which is producing this when trying to invoke the errinjct tool.

I'll send a fixes patch out shortly.

-Tyrel

> +	{ "ibm,get-config-addr-info2", -1, -1, -1, -1, -1 },
> +	{ "ibm,get-dynamic-sensor-state", -1, 1, -1, -1, -1 },
> +	{ "ibm,get-indices", -1, 2, 3, -1, -1 },
> +	{ "get-power-level", -1, -1, -1, -1, -1 },
> +	{ "get-sensor-state", -1, -1, -1, -1, -1 },
> +	{ "ibm,get-system-parameter", -1, 1, 2, -1, -1 },
> +	{ "get-time-of-day", -1, -1, -1, -1, -1 },
> +	{ "ibm,get-vpd", -1, 0, -1, 1, 2 },
> +	{ "ibm,lpar-perftools", -1, 2, 3, -1, -1 },
> +	{ "ibm,platform-dump", -1, 4, 5, -1, -1 },
> +	{ "ibm,read-slot-reset-state", -1, -1, -1, -1, -1 },
> +	{ "ibm,scan-log-dump", -1, 0, 1, -1, -1 },
> +	{ "ibm,set-dynamic-indicator", -1, 2, -1, -1, -1 },
> +	{ "ibm,set-eeh-option", -1, -1, -1, -1, -1 },
> +	{ "set-indicator", -1, -1, -1, -1, -1 },
> +	{ "set-power-level", -1, -1, -1, -1, -1 },
> +	{ "set-time-for-power-on", -1, -1, -1, -1, -1 },
> +	{ "ibm,set-system-parameter", -1, 1, -1, -1, -1 },
> +	{ "set-time-of-day", -1, -1, -1, -1, -1 },
> +	{ "ibm,suspend-me", -1, -1, -1, -1, -1 },
> +	{ "ibm,update-nodes", -1, 0, -1, -1, -1, 4096 },
> +	{ "ibm,update-properties", -1, 0, -1, -1, -1, 4096 },
> +	{ "ibm,physical-attestation", -1, 0, 1, -1, -1 },
> +};
> +
> +static bool in_rmo_buf(u32 base, u32 end)
> +{
> +	return base >= rtas_rmo_buf &&
> +		base < (rtas_rmo_buf + RTAS_RMOBUF_MAX) &&
> +		base <= end &&
> +		end >= rtas_rmo_buf &&
> +		end < (rtas_rmo_buf + RTAS_RMOBUF_MAX);
> +}
> +
> +static bool block_rtas_call(int token, int nargs,
> +			    struct rtas_args *args)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
> +		struct rtas_filter *f = &rtas_filters[i];
> +		u32 base, size, end;
> +
> +		if (token != f->token)
> +			continue;
> +
> +		if (f->buf_idx1 != -1) {
> +			base = be32_to_cpu(args->args[f->buf_idx1]);
> +			if (f->size_idx1 != -1)
> +				size = be32_to_cpu(args->args[f->size_idx1]);
> +			else if (f->fixed_size)
> +				size = f->fixed_size;
> +			else
> +				size = 1;
> +
> +			end = base + size - 1;
> +			if (!in_rmo_buf(base, end))
> +				goto err;
> +		}
> +
> +		if (f->buf_idx2 != -1) {
> +			base = be32_to_cpu(args->args[f->buf_idx2]);
> +			if (f->size_idx2 != -1)
> +				size = be32_to_cpu(args->args[f->size_idx2]);
> +			else if (f->fixed_size)
> +				size = f->fixed_size;
> +			else
> +				size = 1;
> +			end = base + size - 1;
> +
> +			/*
> +			 * Special case for ibm,configure-connector where the
> +			 * address can be 0
> +			 */
> +			if (!strcmp(f->name, "ibm,configure-connector") &&
> +			    base == 0)
> +				return false;
> +
> +			if (!in_rmo_buf(base, end))
> +				goto err;
> +		}
> +
> +		return false;
> +	}
> +
> +err:
> +	pr_err_ratelimited("sys_rtas: RTAS call blocked - exploit attempt?\n");
> +	pr_err_ratelimited("sys_rtas: token=0x%x, nargs=%d (called by %s)\n",
> +			   token, nargs, current->comm);
> +	return true;
> +}
> +
> +#else
> +
> +static bool block_rtas_call(int token, int nargs,
> +			    struct rtas_args *args)
> +{
> +	return false;
> +}
> +
> +#endif /* CONFIG_PPC_RTAS_FILTER */
> +
>  /* We assume to be passed big endian arguments */
>  SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  {
> @@ -1029,6 +1170,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  	args.rets = &args.args[nargs];
>  	memset(args.rets, 0, nret * sizeof(rtas_arg_t));
> 
> +	if (block_rtas_call(token, nargs, &args))
> +		return -EINVAL;
> +
>  	/* Need to handle ibm,suspend_me call specially */
>  	if (token == ibm_suspend_me_token) {
> 
> @@ -1090,6 +1234,9 @@ void __init rtas_initialize(void)
>  	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
>  	u32 base, size, entry;
>  	int no_base, no_size, no_entry;
> +#ifdef CONFIG_PPC_RTAS_FILTER
> +	int i;
> +#endif
> 
>  	/* Get RTAS dev node and fill up our "rtas" structure with infos
>  	 * about it.
> @@ -1129,6 +1276,12 @@ void __init rtas_initialize(void)
>  #ifdef CONFIG_RTAS_ERROR_LOGGING
>  	rtas_last_error_token = rtas_token("rtas-last-error");
>  #endif
> +
> +#ifdef CONFIG_PPC_RTAS_FILTER
> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
> +		rtas_filters[i].token = rtas_token(rtas_filters[i].name);
> +	}
> +#endif
>  }
> 
>  int __init early_init_dt_scan_rtas(unsigned long node,
>

On 9/12/20 5:59 am, Tyrel Datwyler wrote:
>> +	{ "ibm,open-errinct", -1, -1, -1, -1, -1 },
> 
> There is a typo here. Should be ibm,open-errinjct.
> 
> kernel: [ 1100.408626] sys_rtas: RTAS call blocked - exploit attempt?
> kernel: [ 1100.408631] sys_rtas: token=0x26, nargs=0 (called by errinjct)
> 
> Which is producing this when trying to invoke the errinjct tool.
> 
> I'll send a fixes patch out shortly.

*sigh*

Thanks for picking this up!