linux-kernel - Re: [PATCH v5 17/19] irqchip/gic-v3-its: Share ITS tables with a non-trusted hypervisor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dccfad5a-cda8-436b-87c9-b588cf790e86@arm.com>
Date: Mon, 19 Aug 2024 23:19:51 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Marc Zyngier <maz@...nel.org>
Cc: Steven Price <steven.price@....com>, kvm@...r.kernel.org,
 kvmarm@...ts.linux.dev, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, James Morse <james.morse@....com>,
 Oliver Upton <oliver.upton@...ux.dev>, Zenghui Yu <yuzenghui@...wei.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Joey Gouly <joey.gouly@....com>, Alexandru Elisei
 <alexandru.elisei@....com>, Christoffer Dall <christoffer.dall@....com>,
 Fuad Tabba <tabba@...gle.com>, linux-coco@...ts.linux.dev,
 Ganapatrao Kulkarni <gankulkarni@...amperecomputing.com>,
 Gavin Shan <gshan@...hat.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 Alper Gun <alpergun@...gle.com>
Subject: Re: [PATCH v5 17/19] irqchip/gic-v3-its: Share ITS tables with a
 non-trusted hypervisor

Hi Marc

On 19/08/2024 16:24, Marc Zyngier wrote:
> On Mon, 19 Aug 2024 15:51:00 +0100,
> Suzuki K Poulose <suzuki.poulose@....com> wrote:
>>
>> Hi Steven,
>>
>> On 19/08/2024 14:19, Steven Price wrote:
>>> 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.
>>>
>>> For the ITT use a custom genpool-based allocator that calls
>>> set_memory_decrypted() for each page allocated, but then suballocates
>>> the size needed for each ITT. Note that there is no mechanism
>>> implemented to return pages from the genpool, but it is unlikely the
>>> peak number of devices will so much larger than the normal level - so
>>> this isn't expected to be an issue.
>>>
>>
>> This may not be sufficient to make it future proof. We need to detect if
>> the GIC is private vs shared, before we make the allocation
>> choice. Please see below :
> 
> What do you mean by that? Do you foresee a *GICv3* implementation on
> the realm side?

No, but it may be emulated in the Realm World (by a higher privileged 
component, with future RMM versions with Planes - Plane0) and this
"Realm guest" may run in a lesser privileged plane and must use
"protected" accesses to make sure the accesses are seen by the "Realm
world" emulator.

> 
> [...]
> 
>> How about something like this folded into this patch ? Or if this
>> patch goes in independently, we could carry the following as part of
>> the CCA
>> series.
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 6f4ddf7faed1..f1a779b52210 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -209,7 +209,7 @@ static struct page *its_alloc_pages_node(int node,
>> gfp_t gfp,
>>
>>   	page = alloc_pages_node(node, gfp, order);
>>
>> -	if (page)
>> +	if (gic_rdists->is_shared && page)
>>   		set_memory_decrypted((unsigned long)page_address(page),
>>   				     BIT(order));
>>   	return page;
>> @@ -222,7 +222,8 @@ static struct page *its_alloc_pages(gfp_t gfp,
>> unsigned int order)
>>
>>   static void its_free_pages(void *addr, unsigned int order)
>>   {
>> -	set_memory_encrypted((unsigned long)addr, BIT(order));
>> +	if (gic_rdists->is_shared)
>> +		set_memory_encrypted((unsigned long)addr, BIT(order));
>>   	free_pages((unsigned long)addr, order);
>>   }
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 6fb276504bcc..48c6b2c8dd8c 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -2015,6 +2015,8 @@ static int __init gic_init_bases(phys_addr_t
>> dist_phys_base,
>>   	typer = readl_relaxed(gic_data.dist_base + GICD_TYPER);
>>   	gic_data.rdists.gicd_typer = typer;
>>
>> +	gic_data.rdists.is_shared =
>> !arm64_is_iomem_private(gic_data.dist_phys_base,
>> +							    PAGE_SIZE);
> 
> Why would you base the status of the RDs on that of the distributor?

We expect that, the GIC as a whole is either Realm or non-secure, but
not split (like most of the devices). The only reason for using rdists
is because thats shared and available with the ITS driver code. (and
was an easy hack). Happy to change this to something better.

> 
>>   	gic_enable_quirks(readl_relaxed(gic_data.dist_base + GICD_IIDR),
>>   			  gic_quirks, &gic_data);
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h
>> b/include/linux/irqchip/arm-gic-v3.h
>> index 728691365464..1edc33608d52 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -631,6 +631,7 @@ struct rdists {
>>   	bool			has_rvpeid;
>>   	bool			has_direct_lpi;
>>   	bool			has_vpend_valid_dirty;
>> +	bool			is_shared;
>>   };
>>
>>   struct irq_domain;
>>
> 
> I really don't like this.
> 
> If we have to go down the route of identifying whether the GIC needs
> encryption or not based on the platform, then maybe we should bite the
> bullet and treat it as a first class device, given that we expect
> devices to be either realm or non-secure.

Agreed and that is exactly we would like. i.e., treat the GIC as either
Realm or NS (as a whole). Now, how do we make that decision is based on
whether GIC Distributor area is private or not. Like I mentioned above, 
we need a cleaner way of making this available in the ITS driver.

Thoughts ? Is that what you were hinting at ?

Suzuki


> Thanks,
> 
> 	M.
>