On 17/06/2024 04:54, Michael Kelley wrote:

From: Steven Price <steven.price@arm.com> Sent: Wednesday, June 5, 2024 2:30 AM

quoted

Within a realm guest the ITS is emulated by the host. This means the
allocations must have been made available to the host by a call to
set_memory_decrypted(). Introduce an allocation function which performs
this extra call.

Co-developed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
Changes since v2:
 * Drop 'shared' from the new its_xxx function names as they are used
   for non-realm guests too.
 * Don't handle the NUMA_NO_NODE case specially - alloc_pages_node()
   should do the right thing.
 * Drop a pointless (void *) cast.
---
 drivers/irqchip/irq-gic-v3-its.c | 90 ++++++++++++++++++++++++--------
 1 file changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 40ebf1726393..ca72f830f4cc 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c

@@ -18,6 +18,7 @@
 #include <linux/irqdomain.h>
 #include <linux/list.h>
 #include <linux/log2.h>
+#include <linux/mem_encrypt.h>
 #include <linux/memblock.h>
 #include <linux/mm.h>
 #include <linux/msi.h>

@@ -27,6 +28,7 @@
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/percpu.h>
+#include <linux/set_memory.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>

@@ -163,6 +165,7 @@ struct its_device {
 	struct its_node		*its;
 	struct event_lpi_map	event_map;
 	void			*itt;
+	u32			itt_order;
 	u32			nr_ites;
 	u32			device_id;
 	bool			shared;

@@ -198,6 +201,30 @@ static DEFINE_IDA(its_vpeid_ida);
 #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
 #define gic_data_rdist_vlpi_base()	(gic_data_rdist_rd_base() + SZ_128K)

+static struct page *its_alloc_pages_node(int node, gfp_t gfp,
+					 unsigned int order)
+{
+	struct page *page;
+
+	page = alloc_pages_node(node, gfp, order);
+
+	if (page)
+		set_memory_decrypted((unsigned long)page_address(page),
+				     1 << order);

There's been considerable discussion on the x86 side about
what to do when set_memory_decrypted() or set_memory_encrypted()
fails. The conclusions are:

1) set_memory_decrypted()/encrypted() *could* fail due to a
compromised/malicious host, due to a bug somewhere in the
software stack, or due to resource constraints (x86 might need to
split a large page mapping, and need to allocate additional page
table pages, which could fail).

2) The guest memory that was the target of such a failed call could
be left in an indeterminate state that the guest could not reliably
undo or correct. The guest's view of the page's decrypted/encrypted
state might not match the host's view. Therefore, any such guest
memory must be leaked rather than being used or put back on the
free list.

3) After some discussion, we decided not to panic in such a case.
Instead, set_memory_decrypted()/encrypted() generates a WARN,
as well as returns a failure result. The most security conscious
users could set panic_on_warn=1 in their VMs, and thereby stop
further operation if there any indication that the transition between
encrypted and decrypt is suspect. The caller of these functions
also can take explicit action in the case of a failure.

It seems like the same guidelines should apply here. On the x86
side we've also cleaned up cases where the return value isn't
checked, like here and the use of set_memory_encrypted() below.

Very good points - this code was lacking error handling. I think you are
also right that the correct situation when set_memory_{en,de}crypted()
fails is to WARN() and leak the page. It's something that shouldn't
happen with a well behaving host and it's unclear how to safely recover
the page - so leaking the page is the safest result. And the WARN()
approach gives the user the option as to whether this is fatal via
panic_on_warn.

Thanks,
Steve