Re: [PATCH v10 3/7] firmware: arm_rmm: Move RSI support out of arch/arm64

[Jason Gunthorpe]

> The RSI SMCCC function IDs describe a firmware ABI and are not arm64
> architecture specific definitions. Follow-up changes need to use them from
> non-arch code, including drivers/firmware/smccc and the Arm CCA guest
> driver.
> 
> Move the complete Realm Service Interface (RSI) implementation from
> arch/arm64 to drivers/firmware/arm_rmm. The RSI SMCCC definitions and
> command helpers are also moved to include/linux so they can be shared by
> architecture code and firmware or driver code. This also keeps the
> firmware interface outside architecture code, as requested [1].
> 
> [1] https://lore.kernel.org/all/agsNO9cc7H-b0H8L@willie-the-truck
> 
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Acked-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b5a51b0ef9440a..ff9565d3ffa596 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -38,6 +38,7 @@ config ARM64
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	select ARCH_HAS_MEM_ENCRYPT
>  	select ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS
> +	select ARCH_SUPPORTS_RMM
>  	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_NONLEAF_PMD_YOUNG if ARM64_HAFT
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 49a7002661a928..1620537f0332d9 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -8,6 +8,7 @@
>  #ifndef __ASM_IO_H
>  #define __ASM_IO_H
>  
> +#include <linux/arm-rsi-cmds.h>
>  #include <linux/types.h>
>  #include <linux/pgtable.h>
>  
> @@ -17,7 +18,6 @@
>  #include <asm/early_ioremap.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeature.h>
> -#include <asm/rsi.h>
>  
>  /*
>   * Generic IO read/write.  These perform native-endian accesses.
> diff --git a/arch/arm64/include/asm/mem_encrypt.h b/arch/arm64/include/asm/mem_encrypt.h
> index 636f45b4d8afc7..f03b9d7b83b438 100644
> --- a/arch/arm64/include/asm/mem_encrypt.h
> +++ b/arch/arm64/include/asm/mem_encrypt.h
> @@ -2,8 +2,8 @@
>  #ifndef __ASM_MEM_ENCRYPT_H
>  #define __ASM_MEM_ENCRYPT_H
>  
> +#include <linux/arm-rsi-cmds.h>
>  #include <asm/hypervisor.h>
> -#include <asm/rsi.h>
>  
>  struct device;
>  
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 09d7c00cf40541..58e71e592175ef 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -66,9 +66,9 @@
>  
>  #ifndef __ASSEMBLER__
>  
> +#include <linux/arm-rsi-cmds.h>
>  #include <asm/cpufeature.h>
>  #include <asm/pgtable-types.h>
> -#include <asm/rsi.h>
>  
>  extern bool arm64_use_ng_mappings;
>  extern unsigned long prot_ns_shared;
> diff --git a/arch/arm64/include/asm/rsi.h b/arch/arm64/include/asm/rsi.h
> deleted file mode 100644
> index 88b50d660e85a0..00000000000000
> --- a/arch/arm64/include/asm/rsi.h
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2024 ARM Ltd.
> - */
> -
> -#ifndef __ASM_RSI_H_
> -#define __ASM_RSI_H_
> -
> -#include <linux/errno.h>
> -#include <linux/jump_label.h>
> -#include <asm/rsi_cmds.h>
> -
> -#define RSI_PDEV_NAME "arm-cca-dev"
> -
> -DECLARE_STATIC_KEY_FALSE(rsi_present);
> -
> -void __init arm64_rsi_init(void);
> -
> -bool arm64_rsi_is_protected(phys_addr_t base, size_t size);
> -
> -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 flags)
> -{
> -	unsigned long ret;
> -	phys_addr_t top;
> -
> -	while (start != end) {
> -		ret = rsi_set_addr_range_state(start, end, state, flags, &top);
> -		if (ret || top < start || top > end)
> -			return -EINVAL;
> -		start = top;
> -	}
> -
> -	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,
> -				    RSI_CHANGE_DESTROYED);
> -}
> -
> -/*
> - * 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,
> -				    RSI_CHANGE_DESTROYED);
> -}
> -#endif /* __ASM_RSI_H_ */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index d2690c3ec52885..21fc3a3d25c91f 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -34,7 +34,7 @@ obj-y			:= debug-monitors.o entry.o irq.o fpsimd.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/	\
> -			   rsi.o jump_label.o
> +			   jump_label.o
>  
>  obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
>  					   sys_compat.o
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 29c6100f0c50b2..a707deba6ae701 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/arm-rsi-cmds.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/stddef.h>
> @@ -43,7 +44,6 @@
>  #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>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index fbf215ecc7d061..2a8de20d2b447b 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -34,6 +34,8 @@
>  #include <linux/acpi_iort.h>
>  #include <linux/kmemleak.h>
>  #include <linux/execmem.h>
> +#include <linux/arm-rsi-cmds.h>
> +
>  
>  #include <asm/boot.h>
>  #include <asm/fixmap.h>
> @@ -43,7 +45,6 @@
>  #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>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index b7cc11e4fbfa65..62660bf520a8da 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -310,5 +310,6 @@ source "drivers/firmware/samsung/Kconfig"
>  source "drivers/firmware/smccc/Kconfig"
>  source "drivers/firmware/tegra/Kconfig"
>  source "drivers/firmware/xilinx/Kconfig"
> +source "drivers/firmware/arm_rmm/Kconfig"
>  
>  endmenu
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index be46f1e1dc77fa..196a650ccf025b 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -39,3 +39,4 @@ obj-y				+= samsung/
>  obj-y				+= smccc/
>  obj-y				+= tegra/
>  obj-y				+= xilinx/
> +obj-y				+= arm_rmm/
> diff --git a/drivers/firmware/arm_rmm/Kconfig b/drivers/firmware/arm_rmm/Kconfig
> new file mode 100644
> index 00000000000000..e981e12fdbe6a8
> --- /dev/null
> +++ b/drivers/firmware/arm_rmm/Kconfig
> @@ -0,0 +1,17 @@
> +
> +config ARCH_SUPPORTS_RMM
> +	bool
> +
> +config ARM_RMM_RSI
> +	bool "Realm Service Interface (RSI) Support"
> +	depends on ARCH_SUPPORTS_RMM
> +	default y
> +	help
> +	  Enable support for running Linux as a Realm guest under a Realm
> +	  Management Monitor (RMM), as defined by the Arm Confidential
> +	  Compute Architecture.
> +
> +	  The RMM exposes the Realm Service Interface (RSI) to Realm guests.
> +	  Linux uses RSI to negotiate the interface version, obtain the Realm
> +	  configuration, and manage the protected or shared state of guest
> +	  memory.
> diff --git a/drivers/firmware/arm_rmm/Makefile b/drivers/firmware/arm_rmm/Makefile
> new file mode 100644
> index 00000000000000..3708b37a9aafc6
> --- /dev/null
> +++ b/drivers/firmware/arm_rmm/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_ARM_RMM_RSI)	= rsi.o
> diff --git a/arch/arm64/kernel/rsi.c b/drivers/firmware/arm_rmm/rsi.c
> similarity index 99%
> rename from arch/arm64/kernel/rsi.c
> rename to drivers/firmware/arm_rmm/rsi.c
> index 25ca75ce1a4dff..8e716f1c1e319d 100644
> --- a/arch/arm64/kernel/rsi.c
> +++ b/drivers/firmware/arm_rmm/rsi.c
> @@ -8,11 +8,11 @@
>  #include <linux/psci.h>
>  #include <linux/swiotlb.h>
>  #include <linux/platform_device.h>
> +#include <linux/arm-rsi-cmds.h>
>  
>  #include <asm/io.h>
>  #include <asm/mem_encrypt.h>
>  #include <asm/pgtable.h>
> -#include <asm/rsi.h>
>  
>  static struct realm_config config;
>  
> diff --git a/drivers/virt/coco/arm-cca-guest/Kconfig b/drivers/virt/coco/arm-cca-guest/Kconfig
> index 3f0f013f03f159..2582ebb36922ea 100644
> --- a/drivers/virt/coco/arm-cca-guest/Kconfig
> +++ b/drivers/virt/coco/arm-cca-guest/Kconfig
> @@ -1,6 +1,6 @@
>  config ARM_CCA_GUEST
>  	tristate "Arm CCA Guest driver"
> -	depends on ARM64
> +	depends on ARM_RMM_RSI
>  	select TSM_REPORTS
>  	help
>  	  The driver provides userspace interface to request and
> diff --git a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> index 0eeddd1ff05b51..b741643d136476 100644
> --- a/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> +++ b/drivers/virt/coco/arm-cca-guest/arm-cca-guest.c
> @@ -3,7 +3,8 @@
>   * Copyright (C) 2023 ARM Ltd.
>   */
>  
> -#include <linux/arm-smccc.h>
> +#include <linux/arm-rsi-cmds.h>
> +#include <linux/arm-smccc-rsi.h>
>  #include <linux/cc_platform.h>
>  #include <linux/kernel.h>
>  #include <linux/device-id/platform.h>
> @@ -12,8 +13,6 @@
>  #include <linux/tsm.h>
>  #include <linux/types.h>
>  
> -#include <asm/rsi.h>
> -

  [Severity: High]
  This is a pre-existing issue, but is there a missing bounds check on the
  firmware-supplied length in this file?

  In arm_cca_attestation_continue(), the len value returned from the firmware
  (via rsi_attestation_token_continue) is added to info->offset without verifying
  that len <= size:

	  ret = rsi_attestation_token_continue(info->granule, info->offset, size,
					       &len);
	  info->offset += len;

  If the firmware returns a maliciously large len, info->offset will exceed
  RSI_GRANULE_SIZE (which is 4096 bytes). 

  Later in arm_cca_report_new(), this unvalidated info.offset is used as the
  memcpy() length:

	  if (WARN_ON(token_size + info.offset > max_size))
		  break;
	  memcpy(&token[token_size], buf, info.offset);

  The WARN_ON check only ensures it doesn't exceed max_size, but buf was
  allocated as exactly RSI_GRANULE_SIZE bytes. Can this lead to memcpy()
  reading past the end of buf and copying adjacent kernel memory into the
  attestation token returned to userspace?

Pre-existing, but seems worth fixing for robustness and avoiding
future LLM bot noise.

Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>

-- 
Jason