Re: [PATCH RFC v9 13/51] x86/fault: Handle RMP page faults for user addresses

Dave Hansen
2023-06-12T16:41:00Z

On 6/11/23 21:25, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> When SEV-SNP is enabled globally, a write from the host is subject to
> checks performed by the hardware against the RMP table (APM2 15.36.10)
> at the end of a page walk:
>
> 1. Assigned bit in the RMP table is not set (i.e page is shared).
> 2. Immutable bit in the RMP table is not set.
> 3. If the page table entry that gives the sPA indicates that the
> target page size is a large page, then all RMP entries for the 4KB
> constituting pages of the target must have the assigned bit 0.
>
> Nothing constructive can come of an attempt by userspace to violate case
> 1) (which will result in writing garbage due to page encryption) or case
> 2) (userspace should not ever need or be allowed to write to a page that
> the host has specifically needed to mark immutable).

What does this _mean_? If nothing constructive can come of it, what
does that mean for the kernel?

> Case 3) is dependent on the hypervisor. In case of KVM, due to how
> shared/private pages are partitioned into separate memory pools via
> restricted/guarded memory, there should never be a case where a page in
> the private pool overlaps with a shared page: either it is a
> hugepage-sized allocation and all the sub-pages are private, or it is a
> single-page allocation, in which case it cannot overlap with anything
> but itself.
>
> Therefore, for all 3 cases, it is appropriate to simply kill the
> userspace process if it ever generates an RMP #PF. Implement that logic
> here.
...
> + if (error_code & X86_PF_RMP) {
> + pr_err("Unexpected RMP page fault for address 0x%lx, terminating process\n",
> + address);
> + do_sigbus(regs, error_code, address, VM_FAULT_SIGBUS);
> + return;
> + }
> +

This is special-snowflake code. You're making the argument that an RMP
fault is a special snowflake and needs special handling.

Why should an RMP violation be any different than, say a write to a
read-only page (that also ends in signal delivery)?

I kinda dislike the entire changelog here. I really don't know what
point it's making or what it is arguing.