* Adam Steen <a...@adamsteen.com.au> [2020-04-07 08:18:19 +0000]:
On Fri, Feb 07, 2020 at 01:25:38PM -0800, Mike Larkin wrote:
> On Fri, Feb 07, 2020 at 04:20:16AM +0000, Adam Steen wrote:
> > Hi
> >
> > Please see the attached patch to add an 'IOCTL handler to sets the access
> > protections of the ept'
> >
> > vmd(8) does not make use of this change, but solo5, which uses vmm(4) as
> > a backend hypervisor. The code calling 'VMM_IOC_MPROTECT_EPT' is
> > available here
https://github.com/Solo5/solo5/compare/master...adamsteen:wnox
> >
> > there are changes to vmd too, but this is just to ensure completeness,
> > if mprotect ept is called in the future, we would want the vm to be
> > stopped if we get a protection fault.
> >
> > I was unsure what todo if called with execute only permissions on a cpu that
> > does not support it. I went with add read permissions and logging the
> > fact, instead of returning EINVAL.
> >
> > Cheers
> > Adam
> >
>
> I have been giving Adam feedback on this diff for a while. There are a few
> minor comments below, but I think this is ok if someone wants to commit it
after
> the fixes below are incorporated.
>
> -ml
>
See updated comment below.
-ml
<SNIP>
> > + /* No W^X permissions */
> > + if ((prot & PROT_MASK) != prot &&
> > + (prot & (PROT_WRITE | PROT_EXEC)) == (PROT_WRITE | PROT_EXEC)) {
> > + DPRINTF("%s: No W^X permissions\n", __func__);
> > + return (EINVAL);
> > + }
>
> I would probably reword this to "W+X permission requested".
>
<SNIP>
> > +/*
> > + * vmx_mprotect_ept
> > + *
> > + * apply the ept protections to the requested pages, faulting the page if
>
> "faulting in"
>
> > + * required.
> > + */
> > +int
> > +vmx_mprotect_ept(vm_map_t vm_map, paddr_t sgpa, paddr_t egpa, int prot)
> > +{
> > + struct vmx_invept_descriptor vid;
> > + pmap_t pmap;
> > + pt_entry_t *pte;
> > + paddr_t addr;
> > + int ret = 0;
> > > +
> > + pmap = vm_map->pmap;
> > +
> > + for (addr = sgpa; addr < egpa; addr += PAGE_SIZE) {
> > + pte = vmx_pmap_find_pte_ept(pmap, addr);
> > + if (pte == NULL) {
>
> if (pte & PG_V) == 0
>
After reading a reply from Adam, I think the original suggested way is fine.
My idea of checking PG_V is fine for RWX EPT entries, but as soon as anyone
uses XO entries, this check wouldn't work.
-ml
Hi
I have incorporated the fixes ml requested above and a fixed a few nits
from pd@, but with the crazyness of life these days, i haven't been able to it
commited, so i attaching it again below.
tested ok last week. I can commit it (this and cr0 one) tonight and
watch for any fallout
Thanks and apologies for delay!
--
Pratik
