linux-kernel - Re: [PATCH v4 05/15] arm64: Mark all I/O as non-secure shared

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: <f8dca28c-e5d6-4a1b-8bd3-6a711dae7078@arm.com>
Date: Tue, 9 Jul 2024 13:54:14 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Will Deacon <will@...nel.org>, Steven Price <steven.price@....com>
Cc: kvm@...r.kernel.org, kvmarm@...ts.linux.dev,
 Catalin Marinas <catalin.marinas@....com>, Marc Zyngier <maz@...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>
Subject: Re: [PATCH v4 05/15] arm64: Mark all I/O as non-secure shared

Hi Will

On 09/07/2024 12:39, Will Deacon wrote:
> On Mon, Jul 01, 2024 at 10:54:55AM +0100, Steven Price wrote:
>> All I/O is by default considered non-secure for realms. As such
>> mark them as shared with the host.
>>
>> Co-developed-by: Suzuki K Poulose <suzuki.poulose@....com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>> Signed-off-by: Steven Price <steven.price@....com>
>> ---
>> Changes since v3:
>>   * Add PROT_NS_SHARED to FIXMAP_PAGE_IO rather than overriding
>>     set_fixmap_io() with a custom function.
>>   * Modify ioreamp_cache() to specify PROT_NS_SHARED too.
>> ---
>>   arch/arm64/include/asm/fixmap.h | 2 +-
>>   arch/arm64/include/asm/io.h     | 8 ++++----
>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
>> index 87e307804b99..f2c5e653562e 100644
>> --- a/arch/arm64/include/asm/fixmap.h
>> +++ b/arch/arm64/include/asm/fixmap.h
>> @@ -98,7 +98,7 @@ enum fixed_addresses {
>>   #define FIXADDR_TOT_SIZE	(__end_of_fixed_addresses << PAGE_SHIFT)
>>   #define FIXADDR_TOT_START	(FIXADDR_TOP - FIXADDR_TOT_SIZE)
>>   
>> -#define FIXMAP_PAGE_IO     __pgprot(PROT_DEVICE_nGnRE)
>> +#define FIXMAP_PAGE_IO     __pgprot(PROT_DEVICE_nGnRE | PROT_NS_SHARED)
>>   
>>   void __init early_fixmap_init(void);
>>   
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 4ff0ae3f6d66..07fc1801c6ad 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -277,12 +277,12 @@ static inline void __const_iowrite64_copy(void __iomem *to, const void *from,
>>   
>>   #define ioremap_prot ioremap_prot
>>   
>> -#define _PAGE_IOREMAP PROT_DEVICE_nGnRE
>> +#define _PAGE_IOREMAP (PROT_DEVICE_nGnRE | PROT_NS_SHARED)
>>   
>>   #define ioremap_wc(addr, size)	\
>> -	ioremap_prot((addr), (size), PROT_NORMAL_NC)
>> +	ioremap_prot((addr), (size), (PROT_NORMAL_NC | PROT_NS_SHARED))
>>   #define ioremap_np(addr, size)	\
>> -	ioremap_prot((addr), (size), PROT_DEVICE_nGnRnE)
>> +	ioremap_prot((addr), (size), (PROT_DEVICE_nGnRnE | PROT_NS_SHARED))
> 
> Hmm. I do wonder whether you've pushed the PROT_NS_SHARED too far here.
> 
> There's nothing _architecturally_ special about the top address bit.
> Even if the RSI divides the IPA space in half, the CPU doesn't give two
> hoots about it in the hardware. In which case, it feels wrong to bake
> PROT_NS_SHARED into ioremap_prot -- it feels much better to me if the
> ioremap() code OR'd that into the physical address when passing it down

Actually we would like to push the decision of applying the 
"pgprot_decrypted" vs pgprot_encrypted into ioremap_prot(), rather
than sprinkling every user of ioremap_prot().

This could be made depending on the address that is passed on to the
ioremap_prot(). I guess we would need explicit requests from the callers
to add "encrypted vs decrypted". Is that what you guys are looking at ?

> 
> There's a selfish side of that argument, in that we need to hook
> ioremap() for pKVM protected guests, but I do genuinely feel that
> treating address bits as protection bits is arbitrary and doesn't belong
> in these low-level definitions. In a similar vein, AMD has its
> sme_{set,clr}() macros that operate on the PA (e.g. via dma_to_phys()),
> which feels like a more accurate abstraction to me.

I believe that doesn't solve all the problems. They do have a hook in
__ioremap_caller() that implicitly applies pgprot_{en,de}crypted
depending on other info.

Cheers
Suzuki

> 
> Will