by Catalin Marinas

[permalink] [raw]

Subject: Re: [PATCH v3 06/14] arm64: Override set_fixmap_io

On Wed, Jun 05, 2024 at 10:29:58AM +0100, Steven Price wrote:
> +void set_fixmap_io(enum fixed_addresses idx, phys_addr_t phys)
> +{
> + pgprot_t prot = FIXMAP_PAGE_IO;
> +
> + /*
> + * For now we consider all I/O as non-secure. For future
> + * filter the I/O base for setting appropriate permissions.
> + */
> + prot = __pgprot(pgprot_val(prot) | PROT_NS_SHARED);
> +
> + return __set_fixmap(idx, phys, prot);
> +}

In v2, Suzuki said that we want to keep this as a function rather than
just adding PROT_NS_SHARED to FIXMAP_PAGE_IO in case we want to change
this function in the future to allow protected MMIO.

https://lore.kernel.org/linux-arm-kernel/[email protected]/

What I don't understand is that all the other MMIO cases just statically
assume unprotected/shard MMIO. Should we drop this patch here as well,
adjust FIXMAP_PAGE_IO and think about protected MMIO later when we
actually have to do device assignment?

--
Catalin

2024-06-12 10:40:21

Hi Steven

On 05/06/2024 10:29, Steven Price wrote:
> From: Suzuki K Poulose <[email protected]>
>
> Detect that the VM is a realm guest by the presence of the RSI
> interface.
>
> If in a realm then all memory needs to be marked as RIPAS RAM initially,
> the loader may or may not have done this for us. To be sure iterate over
> all RAM and mark it as such. Any failure is fatal as that implies the
> RAM regions passed to Linux are incorrect - which would mean failing
> later when attempting to access non-existent RAM.
>
> Signed-off-by: Suzuki K Poulose <[email protected]>
> Co-developed-by: Steven Price <[email protected]>
> Signed-off-by: Steven Price <[email protected]>
> ---
> Changes since v2:
> * Use DECLARE_STATIC_KEY_FALSE rather than "extern struct
> static_key_false".
> * Rename set_memory_range() to rsi_set_memory_range().
> * Downgrade some BUG()s to WARN()s and handle the condition by
> propagating up the stack. Comment the remaining case that ends in a
> BUG() to explain why.
> * Rely on the return from rsi_request_version() rather than checking
> the version the RMM claims to support.
> * Rename the generic sounding arm64_setup_memory() to
> arm64_rsi_setup_memory() and move the call site to setup_arch().
> ---
> arch/arm64/include/asm/rsi.h | 48 +++++++++++++++++++++
> arch/arm64/include/asm/rsi_cmds.h | 22 ++++++++++
> arch/arm64/kernel/Makefile | 3 +-
> arch/arm64/kernel/rsi.c | 69 +++++++++++++++++++++++++++++++
> arch/arm64/kernel/setup.c | 8 ++++
> arch/arm64/mm/init.c | 1 +
> 6 files changed, 150 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/rsi.h
> create mode 100644 arch/arm64/kernel/rsi.c
>
> diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
> new file mode 100644
> index 000000000000..ce2cdb501d84
> --- /dev/null
> +++ b/arch/arm64/include/asm/rsi.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 ARM Ltd.
> + */
> +
> +#ifndef __ASM_RSI_H_
> +#define __ASM_RSI_H_
> +
> +#include <linux/jump_label.h>
> +#include <asm/rsi_cmds.h>
> +
> +DECLARE_STATIC_KEY_FALSE(rsi_present);
> +
> +void __init arm64_rsi_init(void);
> +void __init arm64_rsi_setup_memory(void);
> +static inline bool is_realm_world(void)
> +{
> + return static_branch_unlikely(&rsi_present);
> +}
> +
> +static inline int rsi_set_memory_range(phys_addr_t start, phys_addr_t end,
> + enum ripas state)
> +{
> + unsigned long ret;
> + phys_addr_t top;
> +
> + while (start != end) {
> + ret = rsi_set_addr_range_state(start, end, state, &top);
> + if (WARN_ON(ret || top < start || top > end))
> + return -EINVAL;
> + start = top;
> + }
> +
> + return 0;
> +}
> +
> +static inline int rsi_set_memory_range_protected(phys_addr_t start,
> + phys_addr_t end)
> +{
> + return rsi_set_memory_range(start, end, RSI_RIPAS_RAM);
> +}
> +
> +static inline int rsi_set_memory_range_shared(phys_addr_t start,
> + phys_addr_t end)
> +{
> + return rsi_set_memory_range(start, end, RSI_RIPAS_EMPTY);
> +}
> +#endif
> diff --git a/arch/arm64/include/asm/rsi_cmds.h b/arch/arm64/include/asm/rsi_cmds.h
> index ad425c5d6f1b..ab8ad435f10e 100644
> --- a/arch/arm64/include/asm/rsi_cmds.h
> +++ b/arch/arm64/include/asm/rsi_cmds.h
> @@ -10,6 +10,11 @@
>
> #include <asm/rsi_smc.h>
>
> +enum ripas {
> + RSI_RIPAS_EMPTY,
> + RSI_RIPAS_RAM,
> +};
> +
> static inline void invoke_rsi_fn_smc_with_res(unsigned long function_id,
> unsigned long arg0,
> unsigned long arg1,
> @@ -44,4 +49,21 @@ static inline unsigned long rsi_get_realm_config(struct realm_config *cfg)
> return res.a0;
> }
>
> +static inline unsigned long rsi_set_addr_range_state(phys_addr_t start,
> + phys_addr_t end,
> + enum ripas state,
> + phys_addr_t *top)
> +{
> + struct arm_smccc_res res;
> +
> + invoke_rsi_fn_smc_with_res(SMC_RSI_IPA_STATE_SET,
> + start, end, state, RSI_NO_CHANGE_DESTROYED,

Though this is fine from the KVM as NS Host perspective, it may be
unnecessarily restrictive in general for a Host implementation. We only
need that RSI_NO_CHANGE_DESTROYED flag for "init all DRAM range as RAM"
(where we want to prevent a host from "destroying pages" that were
populated before activation, without consent). But in all other cases
where we do not rely on the content of the "newly" encrypted page,
we could drop the flag.

I think we need could have variants of this helper one which allows
"DESTROYED" granules to be converted, which must be only used while
"transitioning" a page to encrypted, where we don't rely on the contents
of the page.

Something like :

rsi_set_memory_range_protected_safe() : Do not allow DESTROYED contents
to be converted.

rsi_set_memory_range_protected().

Something like:

---8>---

diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
index ce2cdb501d84..dea2ed99f6d1 100644
--- a/arch/arm64/include/asm/rsi.h
+++ b/arch/arm64/include/asm/rsi.h
@@ -19,13 +19,13 @@ static inline bool is_realm_world(void)
}

static inline int rsi_set_memory_range(phys_addr_t start, phys_addr_t end,
- enum ripas state)
+ enum ripas state, unsigned long flags)
{
unsigned long ret;
phys_addr_t top;

while (start != end) {
- ret = rsi_set_addr_range_state(start, end, state, &top);
+ ret = rsi_set_addr_range_state(start, end, state, flags, &top);
if (WARN_ON(ret || top < start || top > end))
return -EINVAL;
start = top;
@@ -34,15 +34,29 @@ static inline int rsi_set_memory_range(phys_addr_t
start, phys_addr_t end,
return 0;
}

+/*
+ * Convert the specified range to RAM. Do not use this if you rely on
the contents
+ * of a page that may already be in RAM state.
+ */
static inline int rsi_set_memory_range_protected(phys_addr_t start,
phys_addr_t end)
{
- return rsi_set_memory_range(start, end, RSI_RIPAS_RAM);
+ return rsi_set_memory_range(start, end, RSI_RIPAS_RAM, 0);
+}
+
+/*
+ * Convert the specified range to RAM. Do not convert any pages that
may have
+ * been DESTROYED, without our permission.
+ */
+static inline int rsi_set_memory_range_protected_safe(phys_addr_t start,
+ phys_addr_t end)
+{
+ return rsi_set_memory_range(start, end, RSI_RIPAS_RAM,
RSI_NO_CHANGE_DESTROYED);
}

static inline int rsi_set_memory_range_shared(phys_addr_t start,
phys_addr_t end)
{
- return rsi_set_memory_range(start, end, RSI_RIPAS_EMPTY);
+ return rsi_set_memory_range(start, end, RSI_RIPAS_EMPTY, 0);
}
#endif
diff --git a/arch/arm64/include/asm/rsi_cmds.h
b/arch/arm64/include/asm/rsi_cmds.h
index ab8ad435f10e..466615ff90de 100644
--- a/arch/arm64/include/asm/rsi_cmds.h
+++ b/arch/arm64/include/asm/rsi_cmds.h
@@ -52,12 +52,13 @@ static inline unsigned long
rsi_get_realm_config(struct realm_config *cfg)
static inline unsigned long rsi_set_addr_range_state(phys_addr_t start,
phys_addr_t end,
enum ripas state,
+ unsigned long flags,
phys_addr_t *top)
{
struct arm_smccc_res res;

invoke_rsi_fn_smc_with_res(SMC_RSI_IPA_STATE_SET,
- start, end, state, RSI_NO_CHANGE_DESTROYED,
+ start, end, state, flags,
&res);

if (top)
diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
index 3a992bdfd6bb..e6a6681524a0 100644
--- a/arch/arm64/kernel/rsi.c
+++ b/arch/arm64/kernel/rsi.c
@@ -46,8 +46,9 @@ void __init arm64_rsi_setup_memory(void)
return;

/*
- * Iterate over the available memory ranges
- * and convert the state to protected memory.
+ * Iterate over the available memory ranges and convert the state to
+ * protected memory. We should take extra care to ensure that we DO NOT
+ * permit any "DESTROYED" pages to be converted to "RAM".
*
* BUG_ON is used because if the attempt to switch the memory to
* protected has failed here, then future accesses to the memory are
@@ -55,7 +56,7 @@ void __init arm64_rsi_setup_memory(void)
* Bailing out early prevents the guest limping on and dieing later.
*/
for_each_mem_range(i, &start, &end) {
- BUG_ON(rsi_set_memory_range_protected(start, end));
+ BUG_ON(rsi_set_memory_range_protected_safe(start, end));
}
}

Kind regards

Suzuki

> + &res);
> +
> + if (top)
> + *top = res.a1;
> +
> + return res.a0;
> +}
> +
> #endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 763824963ed1..a483b916ed11 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -33,7 +33,8 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
> return_address.o cpuinfo.o cpu_errata.o \
> cpufeature.o alternative.o cacheinfo.o \
> smp.o smp_spin_table.o topology.o smccc-call.o \
> - syscall.o proton-pack.o idle.o patching.o pi/
> + syscall.o proton-pack.o idle.o patching.o pi/ \
> + rsi.o
>
> obj-$(CONFIG_COMPAT) += sys32.o signal32.o \
> sys_compat.o
> diff --git a/arch/arm64/kernel/rsi.c b/arch/arm64/kernel/rsi.c
> new file mode 100644
> index 000000000000..3a992bdfd6bb
> --- /dev/null
> +++ b/arch/arm64/kernel/rsi.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 ARM Ltd.
> + */
> +
> +#include <linux/jump_label.h>
> +#include <linux/memblock.h>
> +#include <asm/rsi.h>
> +
> +DEFINE_STATIC_KEY_FALSE_RO(rsi_present);
> +EXPORT_SYMBOL(rsi_present);
> +
> +static bool rsi_version_matches(void)
> +{
> + unsigned long ver_lower, ver_higher;
> + unsigned long ret = rsi_request_version(RSI_ABI_VERSION,
> + &ver_lower,
> + &ver_higher);
> +
> + if (ret == SMCCC_RET_NOT_SUPPORTED)
> + return false;
> +
> + if (ret != RSI_SUCCESS) {
> + pr_err("RME: RMM doesn't support RSI version %u.%u. Supported range: %lu.%lu-%lu.%lu\n",
> + RSI_ABI_VERSION_MAJOR, RSI_ABI_VERSION_MINOR,
> + RSI_ABI_VERSION_GET_MAJOR(ver_lower),
> + RSI_ABI_VERSION_GET_MINOR(ver_lower),
> + RSI_ABI_VERSION_GET_MAJOR(ver_higher),
> + RSI_ABI_VERSION_GET_MINOR(ver_higher));
> + return false;
> + }
> +
> + pr_info("RME: Using RSI version %lu.%lu\n",
> + RSI_ABI_VERSION_GET_MAJOR(ver_lower),
> + RSI_ABI_VERSION_GET_MINOR(ver_lower));
> +
> + return true;
> +}
> +
> +void __init arm64_rsi_setup_memory(void)
> +{
> + u64 i;
> + phys_addr_t start, end;
> +
> + if (!is_realm_world())
> + return;
> +
> + /*
> + * Iterate over the available memory ranges
> + * and convert the state to protected memory.
> + *
> + * BUG_ON is used because if the attempt to switch the memory to
> + * protected has failed here, then future accesses to the memory are
> + * simply going to be reflected as a fault which we can't handle.
> + * Bailing out early prevents the guest limping on and dieing later.
> + */
> + for_each_mem_range(i, &start, &end) {
> + BUG_ON(rsi_set_memory_range_protected(start, end));
> + }
> +}
> +
> +void __init arm64_rsi_init(void)
> +{
> + if (!rsi_version_matches())
> + return;
> +
> + static_branch_enable(&rsi_present);
> +}
> +
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index a096e2451044..143f87615af0 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -43,6 +43,7 @@
> #include <asm/cpu_ops.h>
> #include <asm/kasan.h>
> #include <asm/numa.h>
> +#include <asm/rsi.h>
> #include <asm/scs.h>
> #include <asm/sections.h>
> #include <asm/setup.h>
> @@ -293,6 +294,11 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> * cpufeature code and early parameters.
> */
> jump_label_init();
> + /*
> + * Init RSI before early param so that "earlycon" console uses the
> + * shared alias when in a realm
> + */
> + arm64_rsi_init();
> parse_early_param();
>
> dynamic_scs_init();
> @@ -328,6 +334,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>
> arm64_memblock_init();
>
> + arm64_rsi_setup_memory();
> +
> paging_init();
>
> acpi_table_upgrade();
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 9b5ab6818f7f..9d8d38e3bee2 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -41,6 +41,7 @@
> #include <asm/kvm_host.h>
> #include <asm/memory.h>
> #include <asm/numa.h>
> +#include <asm/rsi.h>
> #include <asm/sections.h>
> #include <asm/setup.h>
> #include <linux/sizes.h>