Re: [RFC PATCH] x86/sev: Disallow userspace access to BIOS region for SEV-SNP guests

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

On 4/7/25 08:13, Naveen N Rao wrote:
> On Thu, Apr 03, 2025 at 12:06:29PM -0700, Dan Williams wrote:
>> Naveen N Rao (AMD) wrote:
>>> Commit 9704c07bf9f7 ("x86/kernel: Validate ROM memory before accessing
>>> when SEV-SNP is active") added code to validate the ROM region from
>>> 0xc0000 to 0xfffff in a SEV-SNP guest since that region can be accessed
>>> during kernel boot. That address range is not part of the system RAM, so
>>> it needed to be validated separately.
>>>
>>> Commit 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
>>> SEV-SNP guests") reverted those changes and instead chose to prevent the
>>> guest from accessing the ROM region since SEV-SNP guests did not rely on
>>> data from that region. However, while the kernel itself no longer
>>> accessed the ROM region, there are userspace programs that probe this
>>> region through /dev/mem and they started crashing due to this change. In
>>> particular, fwupd (up until versions released last year that no longer
>>> link against libsmbios) and smbios utilities such as smbios-sys-info
>>> crash with a cryptic message in dmesg:
>>>   Wrong/unhandled opcode bytes: 0x8b, exit_code: 0x404, rIP: 0x7fe5404d3840
>>>   SEV: Unsupported exit-code 0x404 in #VC exception (IP: 0x7fe5404d3840)
>>>
>>> Deny access to the BIOS region (rather than just the video ROM range)
>>> via /dev/mem to address this. Restrict changes to CONFIG_STRICT_DEVMEM=y
>>> which is enabled by default on x86. Add a new x86_platform_ops callback
>>> so Intel can customize the address range to block.
>>>
>>> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
>>> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
>>> ---
>>>  arch/x86/coco/sev/core.c        | 13 +++++++++++++
>>>  arch/x86/include/asm/sev.h      |  2 ++
>>>  arch/x86/include/asm/x86_init.h |  2 ++
>>>  arch/x86/kernel/x86_init.c      |  2 ++
>>>  arch/x86/mm/init.c              |  3 +++
>>>  arch/x86/mm/mem_encrypt_amd.c   |  1 +
>>>  6 files changed, 23 insertions(+)
>>>
> <snip>
>>
>> Is there any driving need to allow devmem at all for TVM access at this
>> point?
>>
>> I would be in favor of making this clearly tied to devmem, call it
>> ".devmem_is_allowed" for symmetry with the mm/init.c helper, and make
>> the default implementation be:
>>
>> static bool platform_devmem_is_allowed(unsigned long pfn)
>> {
>> 	return !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>> }
>>
>> ...if a TVM technology wants more leniency, it can override.
> 
> I'm not fully aware of the history here, but I suppose a TVM should 
> appear as any other VM for userspace. For that reason, I didn't want to 
> block access to /dev/mem any more than was necessary. Admittedly, I have 
> limited insight into which utilities may be using /dev/mem today.
> 
> Tom/Boris, do you see a problem blocking access to /dev/mem for SEV 
> guests?

Not sure why we would suddenly not allow that.

Thanks,
Tom

> 
> 
> - Naveen
>