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>
---
 arch/powerpc/Kconfig       |  13 +++
 arch/powerpc/kernel/rtas.c | 198 +++++++++++++++++++++++++++++++++++++
 2 files changed, 211 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.7.7, v5.4.50, v4.19.131, v4.14.187, v4.9.229, v4.4.229.

v5.7.7: Build OK!
v5.4.50: Failed to apply! Possible dependencies:
    1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
    4238fad366a66 ("powerpc/ima: Add support to initialize ima policy rules")
    9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
    bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")

v4.19.131: Failed to apply! Possible dependencies:
    1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
    4238fad366a66 ("powerpc/ima: Add support to initialize ima policy rules")
    6f5f193e84d3d ("powerpc/opal: add MPIPL interface definitions")
    75d9fc7fd94eb ("powerpc/powernv: move OPAL call wrapper tracing and interrupt handling to C")
    8139046a5a347 ("powerpc/powernv: Make possible for user to force a full ipl cec reboot")
    88ec6b93c8e7d ("powerpc/xive: add OPAL extensions for the XIVE native exploitation support")
    9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
    bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
    fb0b0a73b223f ("powerpc: Enable kcov")

v4.14.187: Failed to apply! Possible dependencies:
    04baaf28f40c6 ("powerpc/powernv: Add support to enable sensor groups")
    10dac04c79b18 ("mips: fix an off-by-one in dma_capable")
    1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
    32ce3862af3c4 ("powerpc/lib: Implement PMEM API")
    5cdcb01e0af5a ("powernv: opal-sensor: Add support to read 64bit sensor values")
    656ecc16e8fc2 ("crypto/nx: Initialize 842 high and normal RxFIFO control registers")
    6f5f193e84d3d ("powerpc/opal: add MPIPL interface definitions")
    74d656d219b98 ("powerpc/powernv: Add opal calls for opencapi")
    77adbd2207e85 ("powerpc/powernv: Add OPAL_BUSY to opal_error_code()")
    88ec6b93c8e7d ("powerpc/xive: add OPAL extensions for the XIVE native exploitation support")
    8c4cdce8b1ab0 ("mtd: nand: qcom: add command elements in BAM transaction")
    9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
    92e3da3cf193f ("powerpc: initial pkey plumbing")
    bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
    d6a90bb83b508 ("powerpc/powernv: Enable tunneled operations")
    e36d0a2ed5019 ("powerpc/powernv: Implement NMI IPI with OPAL_SIGNAL_SYSTEM_RESET")
    ea8c64ace8664 ("dma-mapping: move swiotlb arch helpers to a new header")
    fb0b0a73b223f ("powerpc: Enable kcov")

v4.9.229: Failed to apply! Possible dependencies:
    1515ab9321562 ("powerpc/mm: Dump hash table")
    1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
    40565b5aedd6d ("sched/cputime, powerpc, s390: Make scaled cputime arch specific")
    4ad8622dc5489 ("powerpc/8xx: Implement hw_breakpoint")
    51c9c08439935 ("powerpc/kprobes: Implement Optprobes")
    5b9ff02785986 ("powerpc: Build-time sort the exception table")
    6533b7c16ee57 ("powerpc: Initial stack protector (-fstack-protector) support")
    65c059bcaa731 ("powerpc: Enable support for GCC plugins")
    8eb07b187000d ("powerpc/mm: Dump linux pagetables")
    92e3da3cf193f ("powerpc: initial pkey plumbing")
    981ee2d444408 ("sched/cputime, powerpc: Remove cputime_to_scaled()")
    a7d2475af7aed ("powerpc: Sort the selects under CONFIG_PPC")
    bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
    d6c569b99558b ("powerpc/64: Move HAVE_CONTEXT_TRACKING from pseries to common Kconfig")
    dd5ac03e09554 ("powerpc/mm: Fix page table dump build on non-Book3S")
    ea8c64ace8664 ("dma-mapping: move swiotlb arch helpers to a new header")
    ebfa50df435ee ("powerpc: Add helper to check if offset is within relative branch range")
    fa769d3f58e6b ("powerpc/32: Enable HW_BREAKPOINT on BOOK3S")
    fb0b0a73b223f ("powerpc: Enable kcov")

v4.4.229: Failed to apply! Possible dependencies:
    019132ff3daf3 ("x86/mm/pkeys: Fill in pkey field in siginfo")
    01c8f1c44b83a ("mm, dax, gpu: convert vm_insert_mixed to pfn_t")
    0e749e54244ee ("dax: increase granularity of dax_clear_blocks() operations")
    1874f6895c92d ("x86/mm/gup: Simplify get_user_pages() PTE bit handling")
    1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
    33a709b25a760 ("mm/gup, x86/mm/pkeys: Check VMAs and PTEs for protection keys")
    34c0fd540e79f ("mm, dax, pmem: introduce pfn_t")
    3565fce3a6597 ("mm, x86: get_user_pages() for dax mappings")
    52db400fcd502 ("pmem, dax: clean up clear_pmem()")
    7b2d0dbac4890 ("x86/mm/pkeys: Pass VMA down in to fault signal generation code")
    8f62c883222c9 ("x86/mm/pkeys: Add arch-specific VMA protection bits")
    92e3da3cf193f ("powerpc: initial pkey plumbing")
    b2e0d1625e193 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
    bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
    f25748e3c34eb ("mm, dax: convert vmf_insert_pfn_pmd() to pfn_t")


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

How should we proceed with this patch?

Andrew Donnellan <ajd@linux.ibm.com> writes:

> 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>
> ---
>  arch/powerpc/Kconfig       |  13 +++
>  arch/powerpc/kernel/rtas.c | 198 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 211 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9fa23eb320ff..0e2dfe497357 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -973,6 +973,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 a09eba03f180..ec1cae52d8bd 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -324,6 +324,23 @@ int rtas_token(const char *service)
>  }
>  EXPORT_SYMBOL(rtas_token);
>  
> +#ifdef CONFIG_PPC_RTAS_FILTER
> +
> +static char *rtas_token_name(int token)
> +{
> +	struct property *prop;
> +
> +	for_each_property_of_node(rtas.dev, prop) {
> +		const __be32 *tokp = prop->value;
> +
> +		if (tokp && be32_to_cpu(*tokp) == token)
> +			return prop->name;
> +	}
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_PPC_RTAS_FILTER */
> +
>  int rtas_service_present(const char *service)
>  {
>  	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
> @@ -1110,6 +1127,184 @@ 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[32];
> +
> +	/* Indexes into the args buffer, -1 if not used */
> +	int rmo_buf_idx1;
> +	int rmo_size_idx1;
> +	int rmo_buf_idx2;
> +	int rmo_size_idx2;
> +};
> +
> +struct rtas_filter rtas_filters[] = {
> +	{ "ibm,activate-firmware", -1, -1, -1, -1 },
> +	{ "ibm,configure-connector", 0, -1, 1, -1 },	/* Special cased, size 4096 */
> +	{ "display-character", -1, -1, -1, -1 },
> +	{ "ibm,display-message", 0, -1, -1, -1 },
> +	{ "ibm,errinjct", 2, -1, -1, -1 },		/* Fixed size of 1024 */
> +	{ "ibm,close-errinjct", -1, -1, -1, -1 },
> +	{ "ibm,open-errinct", -1, -1, -1, -1 },
> +	{ "ibm,get-config-addr-info2", -1, -1, -1, -1 },
> +	{ "ibm,get-dynamic-sensor-state", 1, -1, -1, -1 },
> +	{ "ibm,get-indices", 2, 3, -1, -1 },
> +	{ "get-power-level", -1, -1, -1, -1 },
> +	{ "get-sensor-state", -1, -1, -1, -1 },
> +	{ "ibm,get-system-parameter", 1, 2, -1, -1 },
> +	{ "get-time-of-day", -1, -1, -1, -1 },
> +	{ "ibm,get-vpd", 0, -1, 1, 2 },
> +	{ "ibm,lpar-perftools", 2, 3, -1, -1 },
> +	{ "ibm,platform-dump", 4, 5, -1, -1 },
> +	{ "ibm,read-slot-reset-state", -1, -1, -1, -1 },
> +	{ "ibm,scan-log-dump", 0, 1, -1, -1 },
> +	{ "ibm,set-dynamic-indicator", 2, -1, -1, -1 },
> +	{ "ibm,set-eeh-option", -1, -1, -1, -1 },
> +	{ "set-indicator", -1, -1, -1, -1 },
> +	{ "set-power-level", -1, -1, -1, -1 },
> +	{ "set-time-for-power-on", -1, -1, -1, -1 },
> +	{ "ibm,set-system-parameter", 1, -1, -1, -1 },
> +	{ "set-time-of-day", -1, -1, -1, -1 },
> +	{ "ibm,suspend-me", -1, -1, -1, -1 },
> +	{ "ibm,update-nodes", 0, -1, -1, -1 },		/* Fixed size of 4096 */
> +	{ "ibm,update-properties", 0, -1, -1, -1 },	/* Fixed size of 4096 */
> +	{ "ibm,physical-attestation", 0, 1, -1, -1 },
> +};
> +
> +static void dump_rtas_params(int token, int nargs, int nret,
> +			     struct rtas_args *args)
> +{
> +	int i;
> +	char *token_name = rtas_token_name(token);
> +
> +	pr_err_ratelimited("sys_rtas: token=0x%x (%s), nargs=%d, nret=%d (called by %s)\n",
> +			   token, token_name ? token_name : "unknown", nargs,
> +			   nret, current->comm);
> +	pr_err_ratelimited("sys_rtas: args: ");
> +
> +	for (i = 0; i < nargs; i++) {

I wondered if it was possible for me to specify nargs == 0x7fffffff, but
the syscall definition in rtas.c limits nargs to 16.

Other than that, I checked:
 - NULL return values from rtas_token_name were properly handled. 

 - the math around in_rmo_buf. It might be simpler to pass (addr, size)
   rather than (start, end), but I think it's called correctly atm.

 - I did a brief read-over of the basic logic, which makes sense to me.

I did not go through and compare the RTAS paramemter numbering with the
PAPR.

On that basis,
Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> +		u32 arg = be32_to_cpu(args->args[i]);
> +
> +		pr_cont("%08x ", arg);
> +		if (arg >= rtas_rmo_buf &&
> +		    arg < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
> +			pr_cont("(buf+0x%lx) ", arg - rtas_rmo_buf);
> +	}
> +
> +	pr_cont("\n");
> +}
> +
> +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;
> +	const char *reason;
> +	char *token_name = rtas_token_name(token);
> +
> +	if (!token_name)
> +		goto err_notpermitted;
> +
> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
> +		struct rtas_filter *f = &rtas_filters[i];
> +		u32 base, size, end;
> +
> +		if (strcmp(token_name, f->name))
> +			continue;
> +
> +		if (f->rmo_buf_idx1 != -1) {
> +			base = be32_to_cpu(args->args[f->rmo_buf_idx1]);
> +			if (f->rmo_size_idx1 != -1)
> +				size = be32_to_cpu(args->args[f->rmo_size_idx1]);
> +			else if (!strcmp(token_name, "ibm,errinjct"))
> +				size = 1024;
> +			else if (!strcmp(token_name, "ibm,update-nodes") ||
> +				 !strcmp(token_name, "ibm,update-properties") ||
> +				 !strcmp(token_name, "ibm,configure-connector"))
> +				size = 4096;
> +			else
> +				size = 1;
> +
> +			end = base + size - 1;
> +			if (!in_rmo_buf(base, end)) {
> +				reason = "address pair 1 out of range";
> +				goto err;
> +			}
> +		}
> +
> +		if (f->rmo_buf_idx2 != -1) {
> +			base = be32_to_cpu(args->args[f->rmo_buf_idx2]);
> +			if (f->rmo_size_idx2 != -1)
> +				size = be32_to_cpu(args->args[f->rmo_size_idx2]);
> +			else if (!strcmp(token_name, "ibm,configure-connector"))
> +				size = 4096;
> +			else
> +				size = 1;
> +			end = base + size - 1;
> +
> +			/*
> +			 * Special case for ibm,configure-connector where the
> +			 * address can be 0
> +			 */
> +			if (!strcmp(token_name, "ibm,configure-connector") &&
> +			    base == 0)
> +				return false;
> +
> +			if (!in_rmo_buf(base, end)) {
> +				reason = "address pair 2 out of range";
> +				goto err;
> +			}
> +		}
> +
> +		return false;
> +	}
> +
> +err_notpermitted:
> +	reason = "call not permitted";
> +
> +err:
> +	pr_err_ratelimited("sys_rtas: RTAS call blocked - exploit attempt? (%s)\n",
> +			   reason);
> +	dump_rtas_params(token, nargs, 0, args);
> +	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)
>  {
> @@ -1147,6 +1342,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) {
>  
> -- 
> 2.20.1

Hi ajd,

Thanks for taking care of this.

I was going to merge this as-is, but given it's fixing a long standing
issue there's not really a big rush. So a few comments below.

Andrew Donnellan <ajd@linux.ibm.com> writes:
> 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>
> ---
>  arch/powerpc/Kconfig       |  13 +++
>  arch/powerpc/kernel/rtas.c | 198 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 211 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9fa23eb320ff..0e2dfe497357 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -973,6 +973,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 a09eba03f180..ec1cae52d8bd 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -324,6 +324,23 @@ int rtas_token(const char *service)
>  }
>  EXPORT_SYMBOL(rtas_token);
>  
> +#ifdef CONFIG_PPC_RTAS_FILTER
> +

I think this could be combined with the #ifdef block below?

> +static char *rtas_token_name(int token)
> +{
> +	struct property *prop;
> +
> +	for_each_property_of_node(rtas.dev, prop) {
> +		const __be32 *tokp = prop->value;
> +
> +		if (tokp && be32_to_cpu(*tokp) == token)
> +			return prop->name;
> +	}
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_PPC_RTAS_FILTER */
> +
>  int rtas_service_present(const char *service)
>  {
>  	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
> @@ -1110,6 +1127,184 @@ 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[32];

Using a const char * for the name would be more typical, meaning the
strings would end up in .rodata, and could be merged with other uses of
the same strings.

> +
> +	/* Indexes into the args buffer, -1 if not used */
> +	int rmo_buf_idx1;
> +	int rmo_size_idx1;
> +	int rmo_buf_idx2;
> +	int rmo_size_idx2;

The "rmo" prefix is probably unnecessary?

> +};
> +
> +struct rtas_filter rtas_filters[] = {

Should be static, and __ro_after_init ?

> +	{ "ibm,activate-firmware", -1, -1, -1, -1 },

Would it be worth making the indices 1-based, allowing 0 to be the
unused value, meaning you only have to initialise the used fields?

It would require adjusting them before use, but there's only 4 places
they're used, and you could probably use a macro to do the - 1.

> +	{ "ibm,configure-connector", 0, -1, 1, -1 },	/* Special cased, size 4096 */

Does it make sense to put the hard coded sizes in the structure as well?

eg. fixed_size1 = 4096,

I think that would avoid the need for any strcmps in the code.

> +	{ "display-character", -1, -1, -1, -1 },
> +	{ "ibm,display-message", 0, -1, -1, -1 },
> +	{ "ibm,errinjct", 2, -1, -1, -1 },		/* Fixed size of 1024 */
> +	{ "ibm,close-errinjct", -1, -1, -1, -1 },
> +	{ "ibm,open-errinct", -1, -1, -1, -1 },
> +	{ "ibm,get-config-addr-info2", -1, -1, -1, -1 },
> +	{ "ibm,get-dynamic-sensor-state", 1, -1, -1, -1 },
> +	{ "ibm,get-indices", 2, 3, -1, -1 },
> +	{ "get-power-level", -1, -1, -1, -1 },
> +	{ "get-sensor-state", -1, -1, -1, -1 },
> +	{ "ibm,get-system-parameter", 1, 2, -1, -1 },
> +	{ "get-time-of-day", -1, -1, -1, -1 },
> +	{ "ibm,get-vpd", 0, -1, 1, 2 },
> +	{ "ibm,lpar-perftools", 2, 3, -1, -1 },
> +	{ "ibm,platform-dump", 4, 5, -1, -1 },
> +	{ "ibm,read-slot-reset-state", -1, -1, -1, -1 },
> +	{ "ibm,scan-log-dump", 0, 1, -1, -1 },
> +	{ "ibm,set-dynamic-indicator", 2, -1, -1, -1 },
> +	{ "ibm,set-eeh-option", -1, -1, -1, -1 },
> +	{ "set-indicator", -1, -1, -1, -1 },
> +	{ "set-power-level", -1, -1, -1, -1 },
> +	{ "set-time-for-power-on", -1, -1, -1, -1 },
> +	{ "ibm,set-system-parameter", 1, -1, -1, -1 },
> +	{ "set-time-of-day", -1, -1, -1, -1 },
> +	{ "ibm,suspend-me", -1, -1, -1, -1 },
> +	{ "ibm,update-nodes", 0, -1, -1, -1 },		/* Fixed size of 4096 */
> +	{ "ibm,update-properties", 0, -1, -1, -1 },	/* Fixed size of 4096 */
> +	{ "ibm,physical-attestation", 0, 1, -1, -1 },
> +};
> +
> +static void dump_rtas_params(int token, int nargs, int nret,
> +			     struct rtas_args *args)
> +{
> +	int i;
> +	char *token_name = rtas_token_name(token);
> +
> +	pr_err_ratelimited("sys_rtas: token=0x%x (%s), nargs=%d, nret=%d (called by %s)\n",
> +			   token, token_name ? token_name : "unknown", nargs,
> +			   nret, current->comm);
> +	pr_err_ratelimited("sys_rtas: args: ");
> +
> +	for (i = 0; i < nargs; i++) {
> +		u32 arg = be32_to_cpu(args->args[i]);
> +
> +		pr_cont("%08x ", arg);
> +		if (arg >= rtas_rmo_buf &&
> +		    arg < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
> +			pr_cont("(buf+0x%lx) ", arg - rtas_rmo_buf);
> +	}

This can leak the location of the RMO buf into dmesg. I know it's
visible via /proc, but the /proc file is 0400.

So I think it's probably safer if we just don't dump the args, or their
relation to the RMO buf.

> +
> +	pr_cont("\n");
> +}
> +
> +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;
> +	const char *reason;
> +	char *token_name = rtas_token_name(token);

This code isn't particularly performance critical, but I think it would
be cleaner to do the token lookup once at init time, and store the token
in the filter array?

Then this code would only be doing token comparisons.

> +
> +	if (!token_name)
> +		goto err_notpermitted;
> +
> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
> +		struct rtas_filter *f = &rtas_filters[i];
> +		u32 base, size, end;
> +
> +		if (strcmp(token_name, f->name))
> +			continue;
> +
> +		if (f->rmo_buf_idx1 != -1) {
> +			base = be32_to_cpu(args->args[f->rmo_buf_idx1]);
> +			if (f->rmo_size_idx1 != -1)
> +				size = be32_to_cpu(args->args[f->rmo_size_idx1]);
> +			else if (!strcmp(token_name, "ibm,errinjct"))
> +				size = 1024;
> +			else if (!strcmp(token_name, "ibm,update-nodes") ||
> +				 !strcmp(token_name, "ibm,update-properties") ||
> +				 !strcmp(token_name, "ibm,configure-connector"))
> +				size = 4096;
> +			else
> +				size = 1;
> +
> +			end = base + size - 1;
> +			if (!in_rmo_buf(base, end)) {
> +				reason = "address pair 1 out of range";

I don't think we need to give the user this much detail about what they
did wrong, all cases can just print "call not permitted" IMO.

> +				goto err;
> +			}
> +		}
> +
> +		if (f->rmo_buf_idx2 != -1) {
> +			base = be32_to_cpu(args->args[f->rmo_buf_idx2]);
> +			if (f->rmo_size_idx2 != -1)
> +				size = be32_to_cpu(args->args[f->rmo_size_idx2]);
> +			else if (!strcmp(token_name, "ibm,configure-connector"))
> +				size = 4096;
> +			else
> +				size = 1;
> +			end = base + size - 1;
> +
> +			/*
> +			 * Special case for ibm,configure-connector where the
> +			 * address can be 0
> +			 */
> +			if (!strcmp(token_name, "ibm,configure-connector") &&
> +			    base == 0)
> +				return false;
> +
> +			if (!in_rmo_buf(base, end)) {
> +				reason = "address pair 2 out of range";
> +				goto err;
> +			}
> +		}
> +
> +		return false;
> +	}
> +
> +err_notpermitted:
> +	reason = "call not permitted";
> +
> +err:
> +	pr_err_ratelimited("sys_rtas: RTAS call blocked - exploit attempt? (%s)\n",
> +			   reason);
> +	dump_rtas_params(token, nargs, 0, args);
> +	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)
>  {
> @@ -1147,6 +1342,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) {
>  

cheers

On 10/8/20 4:40 pm, Michael Ellerman wrote:
> Hi ajd,
> 
> Thanks for taking care of this.
> 
> I was going to merge this as-is, but given it's fixing a long standing
> issue there's not really a big rush. So a few comments below.

Thanks for the review.

>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index a09eba03f180..ec1cae52d8bd 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -324,6 +324,23 @@ int rtas_token(const char *service)
>>   }
>>   EXPORT_SYMBOL(rtas_token);
>>   
>> +#ifdef CONFIG_PPC_RTAS_FILTER
>> +
> 
> I think this could be combined with the #ifdef block below?

I put it here to keep it next to rtas_token() as it seemed thematically 
appropriate. Anyway per below I'll probably get rid of this function 
altogether.

> 
>> +static char *rtas_token_name(int token)
>> +{
>> +	struct property *prop;
>> +
>> +	for_each_property_of_node(rtas.dev, prop) {
>> +		const __be32 *tokp = prop->value;
>> +
>> +		if (tokp && be32_to_cpu(*tokp) == token)
>> +			return prop->name;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +#endif /* CONFIG_PPC_RTAS_FILTER */
>> +
>>   int rtas_service_present(const char *service)
>>   {
>>   	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
>> @@ -1110,6 +1127,184 @@ 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[32];
> 
> Using a const char * for the name would be more typical, meaning the
> strings would end up in .rodata, and could be merged with other uses of
> the same strings.

Will fix

> 
>> +
>> +	/* Indexes into the args buffer, -1 if not used */
>> +	int rmo_buf_idx1;
>> +	int rmo_size_idx1;
>> +	int rmo_buf_idx2;
>> +	int rmo_size_idx2;
> 
> The "rmo" prefix is probably unnecessary?
> 

Ack

>> +};
>> +
>> +struct rtas_filter rtas_filters[] = {
> 
> Should be static, and __ro_after_init ?
> 
>> +	{ "ibm,activate-firmware", -1, -1, -1, -1 },
> 
> Would it be worth making the indices 1-based, allowing 0 to be the
> unused value, meaning you only have to initialise the used fields?

1-based array indices are morally reprehensible. It would make it 
cleaner but I kind of prefer an obvious and clear constant for unused, idk

> It would require adjusting them before use, but there's only 4 places
> they're used, and you could probably use a macro to do the - 1.
> 
>> +	{ "ibm,configure-connector", 0, -1, 1, -1 },	/* Special cased, size 4096 */
> 
> Does it make sense to put the hard coded sizes in the structure as well?
> 
> eg. fixed_size1 = 4096,
> 
> I think that would avoid the need for any strcmps in the code.

Not quite - we still have a special case for ibm,configure-connector 
passing a base address of 0.

But yes that's a good idea.

> 
>> +	{ "display-character", -1, -1, -1, -1 },
>> +	{ "ibm,display-message", 0, -1, -1, -1 },
>> +	{ "ibm,errinjct", 2, -1, -1, -1 },		/* Fixed size of 1024 */
>> +	{ "ibm,close-errinjct", -1, -1, -1, -1 },
>> +	{ "ibm,open-errinct", -1, -1, -1, -1 },
>> +	{ "ibm,get-config-addr-info2", -1, -1, -1, -1 },
>> +	{ "ibm,get-dynamic-sensor-state", 1, -1, -1, -1 },
>> +	{ "ibm,get-indices", 2, 3, -1, -1 },
>> +	{ "get-power-level", -1, -1, -1, -1 },
>> +	{ "get-sensor-state", -1, -1, -1, -1 },
>> +	{ "ibm,get-system-parameter", 1, 2, -1, -1 },
>> +	{ "get-time-of-day", -1, -1, -1, -1 },
>> +	{ "ibm,get-vpd", 0, -1, 1, 2 },
>> +	{ "ibm,lpar-perftools", 2, 3, -1, -1 },
>> +	{ "ibm,platform-dump", 4, 5, -1, -1 },
>> +	{ "ibm,read-slot-reset-state", -1, -1, -1, -1 },
>> +	{ "ibm,scan-log-dump", 0, 1, -1, -1 },
>> +	{ "ibm,set-dynamic-indicator", 2, -1, -1, -1 },
>> +	{ "ibm,set-eeh-option", -1, -1, -1, -1 },
>> +	{ "set-indicator", -1, -1, -1, -1 },
>> +	{ "set-power-level", -1, -1, -1, -1 },
>> +	{ "set-time-for-power-on", -1, -1, -1, -1 },
>> +	{ "ibm,set-system-parameter", 1, -1, -1, -1 },
>> +	{ "set-time-of-day", -1, -1, -1, -1 },
>> +	{ "ibm,suspend-me", -1, -1, -1, -1 },
>> +	{ "ibm,update-nodes", 0, -1, -1, -1 },		/* Fixed size of 4096 */
>> +	{ "ibm,update-properties", 0, -1, -1, -1 },	/* Fixed size of 4096 */
>> +	{ "ibm,physical-attestation", 0, 1, -1, -1 },
>> +};
>> +
>> +static void dump_rtas_params(int token, int nargs, int nret,
>> +			     struct rtas_args *args)
>> +{
>> +	int i;
>> +	char *token_name = rtas_token_name(token);
>> +
>> +	pr_err_ratelimited("sys_rtas: token=0x%x (%s), nargs=%d, nret=%d (called by %s)\n",
>> +			   token, token_name ? token_name : "unknown", nargs,
>> +			   nret, current->comm);
>> +	pr_err_ratelimited("sys_rtas: args: ");
>> +
>> +	for (i = 0; i < nargs; i++) {
>> +		u32 arg = be32_to_cpu(args->args[i]);
>> +
>> +		pr_cont("%08x ", arg);
>> +		if (arg >= rtas_rmo_buf &&
>> +		    arg < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
>> +			pr_cont("(buf+0x%lx) ", arg - rtas_rmo_buf);
>> +	}
> 
> This can leak the location of the RMO buf into dmesg. I know it's
> visible via /proc, but the /proc file is 0400.
> 
> So I think it's probably safer if we just don't dump the args, or their
> relation to the RMO buf.

Good point, removed.

> 
>> +
>> +	pr_cont("\n");
>> +}
>> +
>> +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;
>> +	const char *reason;
>> +	char *token_name = rtas_token_name(token);
> 
> This code isn't particularly performance critical, but I think it would
> be cleaner to do the token lookup once at init time, and store the token
> in the filter array?
> 
> Then this code would only be doing token comparisons.

Yeah that would be cleaner, can get rid of rtas_token_name().

> 
>> +
>> +	if (!token_name)
>> +		goto err_notpermitted;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
>> +		struct rtas_filter *f = &rtas_filters[i];
>> +		u32 base, size, end;
>> +
>> +		if (strcmp(token_name, f->name))
>> +			continue;
>> +
>> +		if (f->rmo_buf_idx1 != -1) {
>> +			base = be32_to_cpu(args->args[f->rmo_buf_idx1]);
>> +			if (f->rmo_size_idx1 != -1)
>> +				size = be32_to_cpu(args->args[f->rmo_size_idx1]);
>> +			else if (!strcmp(token_name, "ibm,errinjct"))
>> +				size = 1024;
>> +			else if (!strcmp(token_name, "ibm,update-nodes") ||
>> +				 !strcmp(token_name, "ibm,update-properties") ||
>> +				 !strcmp(token_name, "ibm,configure-connector"))
>> +				size = 4096;
>> +			else
>> +				size = 1;
>> +
>> +			end = base + size - 1;
>> +			if (!in_rmo_buf(base, end)) {
>> +				reason = "address pair 1 out of range";
> 
> I don't think we need to give the user this much detail about what they
> did wrong, all cases can just print "call not permitted" IMO.

Ack

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On 10/8/20 4:40 pm, Michael Ellerman wrote:
>> Hi ajd,
>> 
>> Thanks for taking care of this.
>> 
>> I was going to merge this as-is, but given it's fixing a long standing
>> issue there's not really a big rush. So a few comments below.
>
> Thanks for the review.
>
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index a09eba03f180..ec1cae52d8bd 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
...
>> 
>>> +	{ "ibm,activate-firmware", -1, -1, -1, -1 },
>> 
>> Would it be worth making the indices 1-based, allowing 0 to be the
>> unused value, meaning you only have to initialise the used fields?
>
> 1-based array indices are morally reprehensible. It would make it 
> cleaner but I kind of prefer an obvious and clear constant for unused, idk

In my defence they wouldn't be 1-based, they'd be 0-based but off-by-one :P

I'm happy either way, your choice.

cheers

Andrew Donnellan <ajd@linux.ibm.com> writes:

> On 10/8/20 4:40 pm, Michael Ellerman wrote:
>> Hi ajd,
>> 
>> Thanks for taking care of this.
>> 
>> I was going to merge this as-is, but given it's fixing a long standing
>> issue there's not really a big rush. So a few comments below.
>
> Thanks for the review.
>
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index a09eba03f180..ec1cae52d8bd 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -324,6 +324,23 @@ int rtas_token(const char *service)
>>>   }
>>>   EXPORT_SYMBOL(rtas_token);
>>>   
>>> +#ifdef CONFIG_PPC_RTAS_FILTER
>>> +
>> 
>> I think this could be combined with the #ifdef block below?
>
> I put it here to keep it next to rtas_token() as it seemed thematically 
> appropriate. Anyway per below I'll probably get rid of this function 
> altogether.
>
>> 
>>> +static char *rtas_token_name(int token)
>>> +{
>>> +	struct property *prop;
>>> +
>>> +	for_each_property_of_node(rtas.dev, prop) {
>>> +		const __be32 *tokp = prop->value;
>>> +
>>> +		if (tokp && be32_to_cpu(*tokp) == token)
>>> +			return prop->name;
>>> +	}
>>> +	return NULL;
>>> +}
>>> +
>>> +#endif /* CONFIG_PPC_RTAS_FILTER */
>>> +
>>>   int rtas_service_present(const char *service)
>>>   {
>>>   	return rtas_token(service) != RTAS_UNKNOWN_SERVICE;
>>> @@ -1110,6 +1127,184 @@ 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[32];
>> 
>> Using a const char * for the name would be more typical, meaning the
>> strings would end up in .rodata, and could be merged with other uses of
>> the same strings.
>
> Will fix
>
>> 
>>> +
>>> +	/* Indexes into the args buffer, -1 if not used */
>>> +	int rmo_buf_idx1;
>>> +	int rmo_size_idx1;
>>> +	int rmo_buf_idx2;
>>> +	int rmo_size_idx2;
>> 
>> The "rmo" prefix is probably unnecessary?
>> 
>
> Ack
>
>>> +};
>>> +
>>> +struct rtas_filter rtas_filters[] = {
>> 
>> Should be static, and __ro_after_init ?
>> 
>>> +	{ "ibm,activate-firmware", -1, -1, -1, -1 },
>> 
>> Would it be worth making the indices 1-based, allowing 0 to be the
>> unused value, meaning you only have to initialise the used fields?
>
> 1-based array indices are morally reprehensible. It would make it 
> cleaner but I kind of prefer an obvious and clear constant for unused, idk
>
>> It would require adjusting them before use, but there's only 4 places
>> they're used, and you could probably use a macro to do the - 1.
>> 
>>> +	{ "ibm,configure-connector", 0, -1, 1, -1 },	/* Special cased, size 4096 */
>> 
>> Does it make sense to put the hard coded sizes in the structure as well?
>> 
>> eg. fixed_size1 = 4096,
>> 
>> I think that would avoid the need for any strcmps in the code.
>
> Not quite - we still have a special case for ibm,configure-connector 
> passing a base address of 0.
>
> But yes that's a good idea.
>
>> 
>>> +	{ "display-character", -1, -1, -1, -1 },
>>> +	{ "ibm,display-message", 0, -1, -1, -1 },
>>> +	{ "ibm,errinjct", 2, -1, -1, -1 },		/* Fixed size of 1024 */
>>> +	{ "ibm,close-errinjct", -1, -1, -1, -1 },
>>> +	{ "ibm,open-errinct", -1, -1, -1, -1 },
>>> +	{ "ibm,get-config-addr-info2", -1, -1, -1, -1 },
>>> +	{ "ibm,get-dynamic-sensor-state", 1, -1, -1, -1 },
>>> +	{ "ibm,get-indices", 2, 3, -1, -1 },
>>> +	{ "get-power-level", -1, -1, -1, -1 },
>>> +	{ "get-sensor-state", -1, -1, -1, -1 },
>>> +	{ "ibm,get-system-parameter", 1, 2, -1, -1 },
>>> +	{ "get-time-of-day", -1, -1, -1, -1 },
>>> +	{ "ibm,get-vpd", 0, -1, 1, 2 },
>>> +	{ "ibm,lpar-perftools", 2, 3, -1, -1 },
>>> +	{ "ibm,platform-dump", 4, 5, -1, -1 },
>>> +	{ "ibm,read-slot-reset-state", -1, -1, -1, -1 },
>>> +	{ "ibm,scan-log-dump", 0, 1, -1, -1 },
>>> +	{ "ibm,set-dynamic-indicator", 2, -1, -1, -1 },
>>> +	{ "ibm,set-eeh-option", -1, -1, -1, -1 },
>>> +	{ "set-indicator", -1, -1, -1, -1 },
>>> +	{ "set-power-level", -1, -1, -1, -1 },
>>> +	{ "set-time-for-power-on", -1, -1, -1, -1 },
>>> +	{ "ibm,set-system-parameter", 1, -1, -1, -1 },
>>> +	{ "set-time-of-day", -1, -1, -1, -1 },
>>> +	{ "ibm,suspend-me", -1, -1, -1, -1 },
>>> +	{ "ibm,update-nodes", 0, -1, -1, -1 },		/* Fixed size of 4096 */
>>> +	{ "ibm,update-properties", 0, -1, -1, -1 },	/* Fixed size of 4096 */
>>> +	{ "ibm,physical-attestation", 0, 1, -1, -1 },
>>> +};
>>> +
>>> +static void dump_rtas_params(int token, int nargs, int nret,
>>> +			     struct rtas_args *args)
>>> +{
>>> +	int i;
>>> +	char *token_name = rtas_token_name(token);
>>> +
>>> +	pr_err_ratelimited("sys_rtas: token=0x%x (%s), nargs=%d, nret=%d (called by %s)\n",
>>> +			   token, token_name ? token_name : "unknown", nargs,
>>> +			   nret, current->comm);
>>> +	pr_err_ratelimited("sys_rtas: args: ");
>>> +
>>> +	for (i = 0; i < nargs; i++) {
>>> +		u32 arg = be32_to_cpu(args->args[i]);
>>> +
>>> +		pr_cont("%08x ", arg);
>>> +		if (arg >= rtas_rmo_buf &&
>>> +		    arg < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
>>> +			pr_cont("(buf+0x%lx) ", arg - rtas_rmo_buf);
>>> +	}
>> 
>> This can leak the location of the RMO buf into dmesg. I know it's
>> visible via /proc, but the /proc file is 0400.
>> 
>> So I think it's probably safer if we just don't dump the args, or their
>> relation to the RMO buf.
>
> Good point, removed.
>
>> 
>>> +
>>> +	pr_cont("\n");
>>> +}
>>> +
>>> +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;
>>> +	const char *reason;
>>> +	char *token_name = rtas_token_name(token);
>> 
>> This code isn't particularly performance critical, but I think it would
>> be cleaner to do the token lookup once at init time, and store the token
>> in the filter array?
>> 
>> Then this code would only be doing token comparisons.
>
> Yeah that would be cleaner, can get rid of rtas_token_name().

I'm not sure I quite understand what you're suggesting.

You still need to do a string->token lookup at least once as the tokens
differ between PowerVM and qemu. Are you saying that you can fold the
token name lookup into the init function?

Kind regards,
Daniel

>> 
>>> +
>>> +	if (!token_name)
>>> +		goto err_notpermitted;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(rtas_filters); i++) {
>>> +		struct rtas_filter *f = &rtas_filters[i];
>>> +		u32 base, size, end;
>>> +
>>> +		if (strcmp(token_name, f->name))
>>> +			continue;
>>> +
>>> +		if (f->rmo_buf_idx1 != -1) {
>>> +			base = be32_to_cpu(args->args[f->rmo_buf_idx1]);
>>> +			if (f->rmo_size_idx1 != -1)
>>> +				size = be32_to_cpu(args->args[f->rmo_size_idx1]);
>>> +			else if (!strcmp(token_name, "ibm,errinjct"))
>>> +				size = 1024;
>>> +			else if (!strcmp(token_name, "ibm,update-nodes") ||
>>> +				 !strcmp(token_name, "ibm,update-properties") ||
>>> +				 !strcmp(token_name, "ibm,configure-connector"))
>>> +				size = 4096;
>>> +			else
>>> +				size = 1;
>>> +
>>> +			end = base + size - 1;
>>> +			if (!in_rmo_buf(base, end)) {
>>> +				reason = "address pair 1 out of range";
>> 
>> I don't think we need to give the user this much detail about what they
>> did wrong, all cases can just print "call not permitted" IMO.
>
> Ack
>
> -- 
> Andrew Donnellan              OzLabs, ADL Canberra
> ajd@linux.ibm.com             IBM Australia Limited

On 11/8/20 11:41 pm, Daniel Axtens wrote:
>>>> +static bool block_rtas_call(int token, int nargs,
>>>> +			    struct rtas_args *args)
>>>> +{
>>>> +	int i;
>>>> +	const char *reason;
>>>> +	char *token_name = rtas_token_name(token);
>>>
>>> This code isn't particularly performance critical, but I think it would
>>> be cleaner to do the token lookup once at init time, and store the token
>>> in the filter array?
>>>
>>> Then this code would only be doing token comparisons.
>>
>> Yeah that would be cleaner, can get rid of rtas_token_name().
> 
> I'm not sure I quite understand what you're suggesting.
> 
> You still need to do a string->token lookup at least once as the tokens
> differ between PowerVM and qemu. Are you saying that you can fold the
> token name lookup into the init function?

Yeah, mpe is suggesting adding a member to the struct to cache the token 
value, and then just looping through all of them to populate that field 
at init time.