[PATCH] irqchip/gic-v3-its: Mark some in-memory data structures as 'decrypted'

[Robin Murphy]

On 2021-12-08 15:59, Will Deacon wrote:
> The GICv3 ITS driver allocates memory for its tables using alloc_pages()
> and performs explicit cache maintenance if necessary. On systems such
> as those running pKVM, where the memory encryption API is implemented,
> memory shared with the ITS must first be transitioned to the "decrypted"
> state, as it would be if allocated via the DMA API.
> 
> Allow pKVM guests to interact with an ITS emulation by ensuring that the
> shared pages are decrypted at the point of allocation and encrypted
> again upon free().
> 
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: Marc Zyngier <maz at kernel.org>
> Signed-off-by: Will Deacon <will at kernel.org>
> ---
> 
> Although pKVM doesn't yet expose share/unshare hypercalls to the guest,
> this change is agnostic of the hypervisor and could be queued
> independently as it has no functional impact when the memory encryption
> API is not implemented.
> 
>   drivers/irqchip/irq-gic-v3-its.c | 40 ++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index eb0882d15366..4559b8dfb9bc 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -27,6 +27,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>
>   
> @@ -2166,6 +2167,7 @@ static void gic_reset_prop_table(void *va)
>   
>   	/* Make sure the GIC will observe the written configuration */
>   	gic_flush_dcache_to_poc(va, LPI_PROPBASE_SZ);
> +	set_memory_decrypted((unsigned long)va, LPI_PROPBASE_SZ >> PAGE_SHIFT);

Hmm, that would seem to imply we've just done the memset() and cache 
clean on encrypted memory... that's not right, surely?

>   }
>   
>   static struct page *its_allocate_prop_table(gfp_t gfp_flags)
> @@ -2183,8 +2185,10 @@ static struct page *its_allocate_prop_table(gfp_t gfp_flags)
>   
>   static void its_free_prop_table(struct page *prop_page)
>   {
> -	free_pages((unsigned long)page_address(prop_page),
> -		   get_order(LPI_PROPBASE_SZ));
> +	unsigned long va = (unsigned long)page_address(prop_page);
> +
> +	set_memory_encrypted(va, LPI_PROPBASE_SZ >> PAGE_SHIFT);

Similarly to [1], it might be worth at least pretending to consider 
failure in these instances. (Side note, might set_memory_encrypted() 
logically warrant a __must_check annotation?)

> +	free_pages(va, get_order(LPI_PROPBASE_SZ));
>   }
>   
>   static bool gic_check_reserved_range(phys_addr_t addr, unsigned long size)
> @@ -2377,6 +2381,8 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
>   		return -ENXIO;
>   	}
>   
> +	set_memory_decrypted((unsigned long)base,
> +			     PAGE_ORDER_TO_SIZE(order) >> PAGE_SHIFT);
>   	baser->order = order;
>   	baser->base = base;
>   	baser->psz = psz;
> @@ -2512,8 +2518,12 @@ static void its_free_tables(struct its_node *its)
>   
>   	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>   		if (its->tables[i].base) {
> -			free_pages((unsigned long)its->tables[i].base,
> -				   its->tables[i].order);
> +			unsigned long base = (unsigned long)its->tables[i].base;
> +			u32 order = its->tables[i].order;
> +			u32 npages = PAGE_ORDER_TO_SIZE(order) >> PAGE_SHIFT;
> +
> +			set_memory_encrypted(base, npages);
> +			free_pages(base, order);
>   			its->tables[i].base = NULL;
>   		}
>   	}
> @@ -2934,6 +2944,7 @@ static int its_alloc_collections(struct its_node *its)
>   static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>   {
>   	struct page *pend_page;
> +	void *va;
>   
>   	pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
>   				get_order(LPI_PENDBASE_SZ));
> @@ -2941,14 +2952,19 @@ static struct page *its_allocate_pending_table(gfp_t gfp_flags)
>   		return NULL;
>   
>   	/* Make sure the GIC will observe the zero-ed page */
> -	gic_flush_dcache_to_poc(page_address(pend_page), LPI_PENDBASE_SZ);
> +	va = page_address(pend_page);
> +	gic_flush_dcache_to_poc(va, LPI_PENDBASE_SZ);
> +	set_memory_decrypted((unsigned long)va, LPI_PENDBASE_SZ >> PAGE_SHIFT);

Again, it looks fundamentally sketchy to decrypt memory *after* already 
doing something to it - under a "real" encryption scheme the cleaned out 
zeros might now appear as a page full of gibberish ciphertext, which 
would not be what you want.

That said, aren't the pending tables the completely IMP-DEF ones that 
only the ITS itself ever touches? As-is should we even need to "decrypt" 
them at all?

The overall feeling here is that we're not being particularly consistent 
about whether we're readily leaning on the assumption of "encryption" 
only ever meaning CPU stage 2 protection here, or whether we're trying 
to uphold the illusion of the more general notion with the ITS only 
being able to observe "plaintext" shared memory.

Robin.

[1] https://lore.kernel.org/linux-iommu/20211111065028.32761-4-hch@lst.de/

>   
>   	return pend_page;
>   }
>   
>   static void its_free_pending_table(struct page *pt)
>   {
> -	free_pages((unsigned long)page_address(pt), get_order(LPI_PENDBASE_SZ));
> +	unsigned long va = (unsigned long)page_address(pt);
> +
> +	set_memory_encrypted(va, LPI_PENDBASE_SZ >> PAGE_SHIFT);
> +	free_pages(va, get_order(LPI_PENDBASE_SZ));
>   }
>   
>   /*
> @@ -3268,14 +3284,20 @@ static bool its_alloc_table_entry(struct its_node *its,
>   
>   	/* Allocate memory for 2nd level table */
>   	if (!table[idx]) {
> +		void *l2addr;
> +
>   		page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
>   					get_order(baser->psz));
>   		if (!page)
>   			return false;
>   
> +		l2addr = page_address(page);
> +		set_memory_decrypted((unsigned long)l2addr,
> +				     baser->psz >> PAGE_SHIFT);
> +
>   		/* Flush Lvl2 table to PoC if hw doesn't support coherency */
>   		if (!(baser->val & GITS_BASER_SHAREABILITY_MASK))
> -			gic_flush_dcache_to_poc(page_address(page), baser->psz);
> +			gic_flush_dcache_to_poc(l2addr, baser->psz);
>   
>   		table[idx] = cpu_to_le64(page_to_phys(page) | GITS_BASER_VALID);
>   
> @@ -5043,6 +5065,8 @@ static int __init its_probe_one(struct resource *res,
>   	its->fwnode_handle = handle;
>   	its->get_msi_base = its_irq_get_msi_base;
>   	its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
> +	set_memory_decrypted((unsigned long)its->cmd_base,
> +			     ITS_CMD_QUEUE_SZ >> PAGE_SHIFT);
>   
>   	its_enable_quirks(its);
>   
> @@ -5099,6 +5123,8 @@ static int __init its_probe_one(struct resource *res,
>   out_free_tables:
>   	its_free_tables(its);
>   out_free_cmd:
> +	set_memory_encrypted((unsigned long)its->cmd_base,
> +			     ITS_CMD_QUEUE_SZ >> PAGE_SHIFT);
>   	free_pages((unsigned long)its->cmd_base, get_order(ITS_CMD_QUEUE_SZ));
>   out_unmap_sgir:
>   	if (its->sgir_base)
>