Re: [PATCH Part2 v5 00/45] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support

[Brijesh Singh]

On 11/29/21 8:58 AM, Vlastimil Babka wrote:
> On 11/29/21 15:44, Brijesh Singh wrote:
> > On 11/25/21 4:05 AM, Joerg Roedel wrote:
> > > On Wed, Nov 24, 2021 at 09:48:14AM -0800, Dave Hansen wrote:
> > > > That covers things like copy_from_user().  It does not account for
> > > > things where kernel mappings are used, like where a
> > > > get_user_pages()/kmap() is in play.
> > >
> > > The kmap case is guarded by KVM code, which locks the page first so that
> > > the guest can't change the page state, then checks the page state, and
> > > if it is shared does the kmap and the access.
> >
> >
> > The KVM use-case is well covered in the series, but I believe Dave is
> > highlighting what if the access happens outside of the KVM driver (such as a
> > ptrace() or others).
>
> AFAIU ptrace() is a scenario where the userspace mapping is being gup-ped,
> not a kernel page being kmap()ed?

Yes that is correct.

> > One possible approach to fix this is to enlighten the kmap/unmap().
> > Basically, move the per page locking mechanism used by the KVM in the
> > arch-specific code and have kmap/kunmap() call the arch hooks. The arch
> > hooks will do this:
> >
> > Before the map, check whether the page is added as a shared in the RMP
> > table. If not shared, then error.
> > Acquire a per-page map_lock.
> > Release the per-page map_lock on the kunmap().
> >
> > The current patch set provides helpers to change the page from private to
> > shared. Enhance the helpers to check for the per-page map_lock, if the
> > map_lock is held then do not allow changing the page from shared to private.
>
> That could work for the kmap() context.
> What to do for the userspace context (host userspace)?
> - shared->private transition - page has to be unmapped from all userspace,
> elevated refcount (gup() in progress) can block this unmap until it goes
> away - could be doable

An unmap of the page from all the userspace process during the page 
state transition will be great. If we can somehow store the state 
information in the 'struct page' then it can be later used to make 
better decision. I am not sure that relying on the elevated refcount is 
the correct approach. e.g in the case of encrypted guests, the HV may 
pin the page to prevent it from migration.

Thoughts on how you want to approach unmaping the page from userspace 
page table?


> - still, what to do if host userspace then tries to access the unmapped
> page? SIGSEGV instead of SIGBUS and it can recover?

Yes, SIGSEGV makes sense to me.


> > Thoughts ?
> >
> > > This should turn an RMP fault in the kernel which is not covered in the
> > > uaccess exception table into a fatal error.
> > >
> > > Regards,