Re: [Xen-devel] [PATCH v10] x86/emulate: Send vm_event from emulate

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Sep 18, 2019 at 4:35 AM Alexandru Stefan ISAILA
<aisaila@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 18.09.2019 12:47, Jan Beulich wrote:
> > On 17.09.2019 17:09, Tamas K Lengyel wrote:
> >> On Tue, Sep 17, 2019 at 8:24 AM Razvan Cojocaru
> >> <rcojocaru@xxxxxxxxxxxxxxxxxxx> wrote:
> >>>
> >>> On 9/17/19 5:11 PM, Alexandru Stefan ISAILA wrote:
> >>>>>>>> +bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t 
> >>>>>>>> pfec,
> >>>>>>>> +                           uint16_t kind)
> >>>>>>>> +{
> >>>>>>>> +    xenmem_access_t access;
> >>>>>>>> +    vm_event_request_t req = {};
> >>>>>>>> +    paddr_t gpa = (gfn_to_gaddr(gfn) | (gla & ~PAGE_MASK));
> >>>>>>>> +
> >>>>>>>> +    ASSERT(current->arch.vm_event->send_event);
> >>>>>>>> +
> >>>>>>>> +    current->arch.vm_event->send_event = false;
> >>>>>>>> +
> >>>>>>>> +    if ( p2m_get_mem_access(current->domain, gfn, &access,
> >>>>>>>> +                            altp2m_vcpu_idx(current)) != 0 )
> >>>>>>>> +        return false;
> >>>>>>> ... next to the call here (but the maintainers of the file would
> >>>>>>> have to judge in the end). That said, I continue to not understand
> >>>>>>> why a not found entry means unrestricted access. Isn't it
> >>>>>>> ->default_access which controls what such a "virtual" entry would
> >>>>>>> permit?
> >>>>>> I'm sorry for this misleading comment. The code states that if entry 
> >>>>>> was
> >>>>>> not found the access will be default_access and return 0. So in this
> >>>>>> case the default_access will be checked.
> >>>>>>
> >>>>>> /* If request to get default access. */
> >>>>>> if ( gfn_eq(gfn, INVALID_GFN) )
> >>>>>> {
> >>>>>>         *access = memaccess[p2m->default_access];
> >>>>>>         return 0;
> >>>>>> }
> >>>>>>
> >>>>>> If this clears thing up I can remove the "NOTE" part if the comment.
> >>>>> I'm afraid it doesn't clear things up: I'm still lost as to why
> >>>>> "entry not found" implies "full access". And I'm further lost as
> >>>>> to what the code fragment above (dealing with INVALID_GFN, but
> >>>>> not really the "entry not found" case, which would be INVALID_MFN
> >>>>> coming back from a translation) is supposed to tell me.
> >>>>>
> >>>> It is safe enough to consider a invalid mfn from hostp2 to be a
> >>>> violation. There is still a small problem with having the altp2m view
> >>>> not having the page propagated from hostp2m. In this case we have to use
> >>>> altp2m_get_effective_entry().
> >>>
> >>> In the absence of clear guidance from the Intel SDM on what the hardware
> >>> default is for a page not present in the p2m, we should probably follow
> >>> Jan's advice and check violations against default_access for such pages.
> >>
> >> The SDM is very clear that pages that are not present in the EPT are a
> >> violation:
> >>
> >> 28.2.2
> >> An EPT paging-structure entry is present if any of bits 2:0 is 1;
> >> otherwise, the entry is not present. The processor
> >> ignores bits 62:3 and uses the entry neither to reference another EPT
> >> paging-structure entry nor to produce a
> >> physical address. A reference using a guest-physical address whose
> >> translation encounters an EPT paging-struc-
> >> ture that is not present causes an EPT violation (see Section 28.2.3.2).
> >>
> >> 28.2.3.2
> >> EPT Violations
> >> An EPT violation may occur during an access using a guest-physical
> >> address whose translation does not cause an
> >> EPT misconfiguration. An EPT violation occurs in any of the following
> >> situations:
> >> • Translation of the guest-physical address encounters an EPT
> >> paging-structure entry that is not present (see
> >> Section 28.2.2).
> >
> > I'm not sure if / how this helps (other than to answer Razvan's
> > immediate question): It was for a reason that I talked about
> > "virtual" entries, e.g. ones that would be there if they had
> > been propagated already. Albeit propagated ones probably aren't
> > a good case here, since those don't have default_access
> > permissions anyway.
> >
> > But anyway - what my original remark here was about was the
> > (missing) distinction of the different failure modes of
> > p2m_get_mem_access(). For example I'd expect a GFN mapping
> > to physical memory access to which is emulated to go the
> > -ESRCH return path, due to INVALID_MFN coming back. Yet such
> > GFNs still ought to have access controls (at least in theory).
> >
>
> I agree with this and I think they should be treated as XENMEM_access_n.
> If everyone is OK with this I will add a -ESRCH path that uses
> XENMEM_access_n as access.
>

Yeap, that sounds appropriate.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel