Re: [PATCH] x86/ioremap: Maintain consistent IORES_MAP_ENCRYPTED for BIOS data

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/1/25 02:57, Kirill Shutemov wrote:
> On Mon, Mar 31, 2025 at 04:14:40PM -0700, Dan Williams wrote:
>> Nikolay reports [1] that accessing BIOS data (first 1MB of the physical
>> address space) via /dev/mem results in an SEPT violation.
>>
>> The cause is ioremap() (via xlate_dev_mem_ptr()) establishing an
>> unencrypted mapping where the kernel had established an encrypted
>> mapping previously.
>>
>> Teach __ioremap_check_other() that this address space shall always be
>> mapped as encrypted as historically it is memory resident data, not MMIO
>> with side-effects.
> 
> I am not sure if all AMD platforms would survive that.
> 
> Tom?

I haven't tested this, yet, but with SME the BIOS is not encrypted, so
that would need an unencrypted mapping.

Could you qualify your mapping with a TDX check? Or can you do something
in the /dev/mem support to map appropriately?

I'm adding @Naveen since he is preparing a patch to prevent /dev/mem
from accessing ROM areas under SNP as those can trigger #VC for a page
that is mapped encrypted but has not been validated. He's looking at
possibly adding something to x86_platform_ops that can be overridden.
The application would get a bad return code vs an exception.

Thanks,
Tom

> 
>>
>> Cc: <x86@kernel.org>
>> Cc: Vishal Annapurve <vannapurve@google.com>
>> Cc: Kirill Shutemov <kirill.shutemov@linux.intel.com>
>> Reported-by: Nikolay Borisov <nik.borisov@suse.com>
>> Closes: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com [1]
>> Tested-by: Nikolay Borisov <nik.borisov@suse.com>
>> Fixes: 9aa6ea69852c ("x86/tdx: Make pages shared in ioremap()")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  arch/x86/mm/ioremap.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 42c90b420773..9e81286a631e 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -122,6 +122,10 @@ static void __ioremap_check_other(resource_size_t addr, struct ioremap_desc *des
>>  		return;
>>  	}
>>  
>> +	/* Ensure BIOS data (see devmem_is_allowed()) is consistently mapped */
>> +	if (PHYS_PFN(addr) < 256)
> 
> Maybe
> 	if (addr < BIOS_END)
> 
> ?
> 
>> +		desc->flags |= IORES_MAP_ENCRYPTED;
>> +
>>  	if (!IS_ENABLED(CONFIG_EFI))
>>  		return;
>>  
>>
>