Re: [PATCH Part2 RFC v4 10/40] x86/fault: Add support to handle the RMP fault for user address

[Brijesh Singh]

On 7/12/21 11:00 AM, Dave Hansen wrote:
> On 7/12/21 8:43 AM, Brijesh Singh wrote:
> > > > +    /*
> > > > +     * The backing page level is higher than the RMP page level,
> > > > request
> > > > +     * to split the page.
> > > > +     */
> > > > +    if (level > rmp_level)
> > > > +        return RMP_FAULT_PAGE_SPLIT;
> > >
> > > This can theoretically trigger on a hugetlbfs page.  Right?
> >
> > Yes, theoretically.
> >
> > In the current implementation, the VMM is enlightened to not use the
> > hugetlbfs for backing page when creating the SEV-SNP guests.
>
> "The VMM"?

I was meaning a userspace qemu.

> We try to write kernel code so that it "works" and doesn't do unexpected
> things with whatever userspace might throw at it.  This seems to be
> written with an assumption that no VMM will ever use hugetlbfs with SEV-SNP.
>
> That worries me.  Not only because someone is sure to try it, but it's
> the kind of assumption that an attacker or a fuzzer might try.
>
> Could you please test this kernel code in practice with hugetblfs?

Yes, I will make sure that hugetlbfs path is tested in non-RFC version.


> > > I also suspect you can just set VM_FAULT_SIGBUS and let the do_sigbus()
> > > call later on in the function do its work.
> > > >    +static int handle_split_page_fault(struct vm_fault *vmf)
> > > > +{
> > > > +    if (!IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
> > > > +        return VM_FAULT_SIGBUS;
> > > > +
> > > > +    __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL);
> > > > +    return 0;
> > > > +}
> > >
> > > What will this do when you hand it a hugetlbfs page?
> >
> > VMM is updated to not use the hugetlbfs when creating SEV-SNP guests.
> > So, we should not run into it.
>
> Please fix this code to handle hugetlbfs along with any other non-THP
> source of level>0 mappings.  DAX comes to mind.  "Handle" can mean
> rejecting these.  You don't have to find some way to split them and make
> the VM work, just fail safely, ideally as early as possible.
>
> To me, this is a fundamental requirement before this code can be accepted.

Understood, if userspace decided to use the hugetlbfs backing pages then 
I believe earliest we can detect is when we go about adding the pages in 
the RMP table. I'll add a check, and fail the page state change.

-Brijesh