| Message ID | 20190916081024.20931-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v10] x86/emulate: Send vm_event from emulate | expand |
On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( > return HVMTRANS_bad_gfn_to_mfn; > } > > + if ( unlikely(v->arch.vm_event) && > + v->arch.vm_event->send_event && > + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) > + { > + put_page(page); > + return HVMTRANS_gfn_paged_out; I'm sorry, but there is _still_ no comment next to this apparent mis-use of HVMTRANS_gfn_paged_out. > @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, > monitor_traps(current, 1, &req); > } > > +/* > + * Send memory access vm_events based on pfec. Returns true if the event was > + * sent and false for p2m_get_mem_access() error, no violation and event send > + * error. Assumes the caller will check arch.vm_event->send_event. > + * > + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT > + * (in which case access to it is unrestricted, so no violations can occur). > + * In this cases it is fine to continue the emulation. > + */ I think this part of the comment would better go ... > +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? > + switch ( access ) > + { > + case XENMEM_access_x: > + case XENMEM_access_rx: > + if ( pfec & PFEC_write_access ) > + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W; > + break; > + > + case XENMEM_access_w: > + case XENMEM_access_rw: > + if ( pfec & PFEC_insn_fetch ) > + req.u.mem_access.flags = MEM_ACCESS_X; > + break; > + > + case XENMEM_access_r: > + case XENMEM_access_n: > + if ( pfec & PFEC_write_access ) > + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W; > + if ( pfec & PFEC_insn_fetch ) > + req.u.mem_access.flags |= MEM_ACCESS_X; > + break; > + > + case XENMEM_access_wx: > + case XENMEM_access_rwx: > + case XENMEM_access_rx2rw: > + case XENMEM_access_n2rwx: > + case XENMEM_access_default: > + break; > + } > + > + if ( !req.u.mem_access.flags ) > + return false; /* no violation */ > + > + if ( kind == npfec_kind_with_gla ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA | > + MEM_ACCESS_GLA_VALID; > + else if ( kind == npfec_kind_in_gpt ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT | > + MEM_ACCESS_GLA_VALID; > + > + > + req.reason = VM_EVENT_REASON_MEM_ACCESS; > + req.u.mem_access.gfn = gfn_x(gfn); > + req.u.mem_access.gla = gla; > + req.u.mem_access.offset = gpa & ~PAGE_MASK; > + > + return monitor_traps(current, true, &req) >= 0; > +} There are quite a few uses of "current" in here - please consider latching into a local variable named "curr". Jan
On 16.09.2019 18:58, Jan Beulich wrote: > On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( >> return HVMTRANS_bad_gfn_to_mfn; >> } >> >> + if ( unlikely(v->arch.vm_event) && >> + v->arch.vm_event->send_event && >> + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) >> + { >> + put_page(page); >> + return HVMTRANS_gfn_paged_out; > > I'm sorry, but there is _still_ no comment next to this apparent > mis-use of HVMTRANS_gfn_paged_out. I will add this comment here: "/* * In case a vm event was sent return paged_out so the emulation will * stop with no side effect */" > >> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, >> monitor_traps(current, 1, &req); >> } >> >> +/* >> + * Send memory access vm_events based on pfec. Returns true if the event was >> + * sent and false for p2m_get_mem_access() error, no violation and event send >> + * error. Assumes the caller will check arch.vm_event->send_event. >> + * >> + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT >> + * (in which case access to it is unrestricted, so no violations can occur). >> + * In this cases it is fine to continue the emulation. >> + */ > > I think this part of the comment would better go ... > >> +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. > >> + switch ( access ) >> + { >> + case XENMEM_access_x: >> + case XENMEM_access_rx: >> + if ( pfec & PFEC_write_access ) >> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W; >> + break; >> + >> + case XENMEM_access_w: >> + case XENMEM_access_rw: >> + if ( pfec & PFEC_insn_fetch ) >> + req.u.mem_access.flags = MEM_ACCESS_X; >> + break; >> + >> + case XENMEM_access_r: >> + case XENMEM_access_n: >> + if ( pfec & PFEC_write_access ) >> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W; >> + if ( pfec & PFEC_insn_fetch ) >> + req.u.mem_access.flags |= MEM_ACCESS_X; >> + break; >> + >> + case XENMEM_access_wx: >> + case XENMEM_access_rwx: >> + case XENMEM_access_rx2rw: >> + case XENMEM_access_n2rwx: >> + case XENMEM_access_default: >> + break; >> + } >> + >> + if ( !req.u.mem_access.flags ) >> + return false; /* no violation */ >> + >> + if ( kind == npfec_kind_with_gla ) >> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA | >> + MEM_ACCESS_GLA_VALID; >> + else if ( kind == npfec_kind_in_gpt ) >> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT | >> + MEM_ACCESS_GLA_VALID; >> + >> + >> + req.reason = VM_EVENT_REASON_MEM_ACCESS; >> + req.u.mem_access.gfn = gfn_x(gfn); >> + req.u.mem_access.gla = gla; >> + req.u.mem_access.offset = gpa & ~PAGE_MASK; >> + >> + return monitor_traps(current, true, &req) >= 0; >> +} > > There are quite a few uses of "current" in here - please consider > latching into a local variable named "curr". I will add a local variable here. Thanks, Alex
On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote: > On 16.09.2019 18:58, Jan Beulich wrote: >> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( >>> return HVMTRANS_bad_gfn_to_mfn; >>> } >>> >>> + if ( unlikely(v->arch.vm_event) && >>> + v->arch.vm_event->send_event && >>> + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) >>> + { >>> + put_page(page); >>> + return HVMTRANS_gfn_paged_out; >> >> I'm sorry, but there is _still_ no comment next to this apparent >> mis-use of HVMTRANS_gfn_paged_out. > > I will add this comment here: > > "/* > * In case a vm event was sent return paged_out so the emulation will > * stop with no side effect > */" First of all - why "was sent"? The event is yet to be sent, isn't it? And then I'm afraid this still isn't enough. __hvm_copy() gets used for many purposes. For example, while looking into this again when preparing the reply here, I've noticed that above you may wrongly call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no linear address when HVMCOPY_linear is not set. If, while putting together what the comment needs to explain (i.e. everything that can't be implied from the code you add), you considered all cases you should have noticed this yourself. >>> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, >>> monitor_traps(current, 1, &req); >>> } >>> >>> +/* >>> + * Send memory access vm_events based on pfec. Returns true if the event was >>> + * sent and false for p2m_get_mem_access() error, no violation and event send >>> + * error. Assumes the caller will check arch.vm_event->send_event. >>> + * >>> + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT >>> + * (in which case access to it is unrestricted, so no violations can occur). >>> + * In this cases it is fine to continue the emulation. >>> + */ >> >> I think this part of the comment would better go ... >> >>> +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. Jan
On 17.09.2019 11:09, Jan Beulich wrote: > On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote: >> On 16.09.2019 18:58, Jan Beulich wrote: >>> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: >>>> --- a/xen/arch/x86/hvm/hvm.c >>>> +++ b/xen/arch/x86/hvm/hvm.c >>>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( >>>> return HVMTRANS_bad_gfn_to_mfn; >>>> } >>>> >>>> + if ( unlikely(v->arch.vm_event) && >>>> + v->arch.vm_event->send_event && >>>> + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) >>>> + { >>>> + put_page(page); >>>> + return HVMTRANS_gfn_paged_out; >>> >>> I'm sorry, but there is _still_ no comment next to this apparent >>> mis-use of HVMTRANS_gfn_paged_out. >> >> I will add this comment here: >> >> "/* >> * In case a vm event was sent return paged_out so the emulation will >> * stop with no side effect >> */" > > First of all - why "was sent"? The event is yet to be sent, isn't it? Yes it should state "if the event is sent". > And then I'm afraid this still isn't enough. __hvm_copy() gets used > for many purposes. For example, while looking into this again when > preparing the reply here, I've noticed that above you may wrongly > call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no > linear address when HVMCOPY_linear is not set. If, while putting You are right, a check for HVMCOPY_linear should go in the if so we are sure that it is called with a linear address only. > together what the comment needs to explain (i.e. everything that > can't be implied from the code you add), you considered all cases > you should have noticed this yourself. With this new check in place (HVMCOPY_linear) __hvm_copy() will be called from linear_read() linear_write() where it will pass down X86EMUL_RETRY. The comment can change to: "If a event is sent return paged_out. The emulation will have no side effect and will return X86EMUL_RETRY" > >>>> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, >>>> monitor_traps(current, 1, &req); >>>> } >>>> >>>> +/* >>>> + * Send memory access vm_events based on pfec. Returns true if the event was >>>> + * sent and false for p2m_get_mem_access() error, no violation and event send >>>> + * error. Assumes the caller will check arch.vm_event->send_event. >>>> + * >>>> + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT >>>> + * (in which case access to it is unrestricted, so no violations can occur). >>>> + * In this cases it is fine to continue the emulation. >>>> + */ >>> >>> I think this part of the comment would better go ... >>> >>>> +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(). Alex
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. There are indeed - as discussed privately - two cases for an altp2m though: 1. Page not found in the altp2m, but set in the hostp2m - in which case I suggest that we take the hostp2m value into account (with or without propagation to the altp2m; I see no harm in propagating the entry but other may see something I'm missing). 2. Page not found in both altp2m and hostp2m - in which case we should probably check against default_access. Thanks, Razvan
On 17.09.2019 16:11, Alexandru Stefan ISAILA wrote: > > > On 17.09.2019 11:09, Jan Beulich wrote: >> On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote: >>> On 16.09.2019 18:58, Jan Beulich wrote: >>>> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: >>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( >>>>> return HVMTRANS_bad_gfn_to_mfn; >>>>> } >>>>> >>>>> + if ( unlikely(v->arch.vm_event) && >>>>> + v->arch.vm_event->send_event && >>>>> + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) >>>>> + { >>>>> + put_page(page); >>>>> + return HVMTRANS_gfn_paged_out; >>>> >>>> I'm sorry, but there is _still_ no comment next to this apparent >>>> mis-use of HVMTRANS_gfn_paged_out. >>> >>> I will add this comment here: >>> >>> "/* >>> * In case a vm event was sent return paged_out so the emulation will >>> * stop with no side effect >>> */" >> >> First of all - why "was sent"? The event is yet to be sent, isn't it? > > Yes it should state "if the event is sent". "is sent" is still not indicating this is something yet to happen. "is to be sent" would be to me (caveat - I'm not a native speaker). >> And then I'm afraid this still isn't enough. __hvm_copy() gets used >> for many purposes. For example, while looking into this again when >> preparing the reply here, I've noticed that above you may wrongly >> call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no >> linear address when HVMCOPY_linear is not set. If, while putting > > You are right, a check for HVMCOPY_linear should go in the if so we are > sure that it is called with a linear address only. > >> together what the comment needs to explain (i.e. everything that >> can't be implied from the code you add), you considered all cases >> you should have noticed this yourself. > > With this new check in place (HVMCOPY_linear) __hvm_copy() will be > called from linear_read() linear_write() where it will pass down > X86EMUL_RETRY. > > The comment can change to: > "If a event is sent return paged_out. The emulation will have no side > effect and will return X86EMUL_RETRY" I'm sorry to be a pain in your neck, but no, this still is not sufficient imo. The comment, whatever wording you choose, should make clear that you have understood the possible effects of using a suspicious return value, and it should also make clear to readers that this is in fact not going to cause a problem _for any caller_. Jan
On 17.09.2019 17:32, Jan Beulich wrote: > On 17.09.2019 16:11, Alexandru Stefan ISAILA wrote: >> >> >> On 17.09.2019 11:09, Jan Beulich wrote: >>> On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote: >>>> On 16.09.2019 18:58, Jan Beulich wrote: >>>>> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote: >>>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( >>>>>> return HVMTRANS_bad_gfn_to_mfn; >>>>>> } >>>>>> >>>>>> + if ( unlikely(v->arch.vm_event) && >>>>>> + v->arch.vm_event->send_event && >>>>>> + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) >>>>>> + { >>>>>> + put_page(page); >>>>>> + return HVMTRANS_gfn_paged_out; >>>>> >>>>> I'm sorry, but there is _still_ no comment next to this apparent >>>>> mis-use of HVMTRANS_gfn_paged_out. >>>> >>>> I will add this comment here: >>>> >>>> "/* >>>> * In case a vm event was sent return paged_out so the emulation will >>>> * stop with no side effect >>>> */" >>> >>> First of all - why "was sent"? The event is yet to be sent, isn't it? >> >> Yes it should state "if the event is sent". > > "is sent" is still not indicating this is something yet to happen. > "is to be sent" would be to me (caveat - I'm not a native speaker). > >>> And then I'm afraid this still isn't enough. __hvm_copy() gets used >>> for many purposes. For example, while looking into this again when >>> preparing the reply here, I've noticed that above you may wrongly >>> call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no >>> linear address when HVMCOPY_linear is not set. If, while putting >> >> You are right, a check for HVMCOPY_linear should go in the if so we are >> sure that it is called with a linear address only. >> >>> together what the comment needs to explain (i.e. everything that >>> can't be implied from the code you add), you considered all cases >>> you should have noticed this yourself. >> >> With this new check in place (HVMCOPY_linear) __hvm_copy() will be >> called from linear_read() linear_write() where it will pass down >> X86EMUL_RETRY. >> >> The comment can change to: >> "If a event is sent return paged_out. The emulation will have no side >> effect and will return X86EMUL_RETRY" > > I'm sorry to be a pain in your neck, but no, this still is not > sufficient imo. The comment, whatever wording you choose, > should make clear that you have understood the possible effects > of using a suspicious return value, and it should also make > clear to readers that this is in fact not going to cause a > problem _for any caller_. > There is no problem, I understand the risk of having suspicious return values. I am not hanged on having this return. I used this to skip adding a new return value. I can do this if we agree on a suitable name for a new return value in enum hvm_translation_result. I can propose HVMTRANS_bad_gfn_access. Alex
On 17.09.2019 17:00, Alexandru Stefan ISAILA wrote: > There is no problem, I understand the risk of having suspicious return > values. I am not hanged on having this return. I used this to skip > adding a new return value. I can do this if we agree on a suitable name > for a new return value in enum hvm_translation_result. I can propose > HVMTRANS_bad_gfn_access. How intrusive would such a change be? Jan
On Tue, Sep 17, 2019 at 8:24 AM Razvan Cojocaru <rcojocaru@bbu.bitdefender.biz> 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). Tamas
On 17.09.2019 18:04, Jan Beulich wrote: > On 17.09.2019 17:00, Alexandru Stefan ISAILA wrote: >> There is no problem, I understand the risk of having suspicious return >> values. I am not hanged on having this return. I used this to skip >> adding a new return value. I can do this if we agree on a suitable name >> for a new return value in enum hvm_translation_result. I can propose >> HVMTRANS_bad_gfn_access. > > How intrusive would such a change be? > Only the return for hvm_copy_to_guest_linear() and hvm_copy_from_guest_linear() will be affected. There are 2 places to add checks, in linear_write() and linear_read(). The new return value can turn up only in hvm_emulate_one_vm_event() context, when vm_event->send_event flag is true. Alex
On 9/17/19 6:09 PM, Tamas K Lengyel wrote: > On Tue, Sep 17, 2019 at 8:24 AM Razvan Cojocaru > <rcojocaru@bbu.bitdefender.biz> 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). Mystery solved. Thanks!
On 17.09.2019 17:39, Alexandru Stefan ISAILA wrote: > On 17.09.2019 18:04, Jan Beulich wrote: >> On 17.09.2019 17:00, Alexandru Stefan ISAILA wrote: >>> There is no problem, I understand the risk of having suspicious return >>> values. I am not hanged on having this return. I used this to skip >>> adding a new return value. I can do this if we agree on a suitable name >>> for a new return value in enum hvm_translation_result. I can propose >>> HVMTRANS_bad_gfn_access. >> >> How intrusive would such a change be? >> > > Only the return for hvm_copy_to_guest_linear() and > hvm_copy_from_guest_linear() will be affected. > There are 2 places to add checks, in linear_write() and linear_read(). > The new return value can turn up only in hvm_emulate_one_vm_event() > context, when vm_event->send_event flag is true. But why would e.g. the switch() in hvmemul_map_linear_addr() not also need adjustment? And if the new return value is to be mostly similar to HVMTRANS_gfn_paged_out, things like hvmemul_rep_movs() ought to need adjustment too (possibly just in the form of an added ASSERT(), but anyway). Jan
On 17.09.2019 17:09, Tamas K Lengyel wrote: > On Tue, Sep 17, 2019 at 8:24 AM Razvan Cojocaru > <rcojocaru@bbu.bitdefender.biz> 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). Jan
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@bbu.bitdefender.biz> 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. Alex
On Wed, Sep 18, 2019 at 4:35 AM Alexandru Stefan ISAILA <aisaila@bitdefender.com> 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@bbu.bitdefender.biz> 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
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 36bcb526d3..22c85937ad 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -548,6 +548,7 @@ static void *hvmemul_map_linear_addr( unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) - (linear >> PAGE_SHIFT) + 1; unsigned int i; + gfn_t gfn; /* * mfn points to the next free slot. All used slots have a page reference @@ -582,7 +583,7 @@ static void *hvmemul_map_linear_addr( ASSERT(mfn_x(*mfn) == 0); res = hvm_translate_get_page(curr, addr, true, pfec, - &pfinfo, &page, NULL, &p2mt); + &pfinfo, &page, &gfn, &p2mt); switch ( res ) { @@ -626,6 +627,14 @@ static void *hvmemul_map_linear_addr( ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt)); } + + if ( unlikely(curr->arch.vm_event) && + curr->arch.vm_event->send_event && + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) + { + err = ERR_PTR(~X86EMUL_RETRY); + goto out; + } } /* Entire access within a single frame? */ diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 452ac4833d..195a07c64d 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy( return HVMTRANS_bad_gfn_to_mfn; } + if ( unlikely(v->arch.vm_event) && + v->arch.vm_event->send_event && + hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) ) + { + put_page(page); + return HVMTRANS_gfn_paged_out; + } + p = (char *)__map_domain_page(page) + (addr & ~PAGE_MASK); if ( flags & HVMCOPY_to_guest ) diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 2a41ccc930..8c9d2284d1 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -23,8 +23,10 @@ */ #include <xen/vm_event.h> +#include <xen/mem_access.h> #include <xen/monitor.h> #include <asm/hvm/monitor.h> +#include <asm/altp2m.h> #include <asm/monitor.h> #include <asm/paging.h> #include <asm/vm_event.h> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, monitor_traps(current, 1, &req); } +/* + * Send memory access vm_events based on pfec. Returns true if the event was + * sent and false for p2m_get_mem_access() error, no violation and event send + * error. Assumes the caller will check arch.vm_event->send_event. + * + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT + * (in which case access to it is unrestricted, so no violations can occur). + * In this cases it is fine to continue the emulation. + */ +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; + + switch ( access ) + { + case XENMEM_access_x: + case XENMEM_access_rx: + if ( pfec & PFEC_write_access ) + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W; + break; + + case XENMEM_access_w: + case XENMEM_access_rw: + if ( pfec & PFEC_insn_fetch ) + req.u.mem_access.flags = MEM_ACCESS_X; + break; + + case XENMEM_access_r: + case XENMEM_access_n: + if ( pfec & PFEC_write_access ) + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W; + if ( pfec & PFEC_insn_fetch ) + req.u.mem_access.flags |= MEM_ACCESS_X; + break; + + case XENMEM_access_wx: + case XENMEM_access_rwx: + case XENMEM_access_rx2rw: + case XENMEM_access_n2rwx: + case XENMEM_access_default: + break; + } + + if ( !req.u.mem_access.flags ) + return false; /* no violation */ + + if ( kind == npfec_kind_with_gla ) + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA | + MEM_ACCESS_GLA_VALID; + else if ( kind == npfec_kind_in_gpt ) + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT | + MEM_ACCESS_GLA_VALID; + + + req.reason = VM_EVENT_REASON_MEM_ACCESS; + req.u.mem_access.gfn = gfn_x(gfn); + req.u.mem_access.gla = gla; + req.u.mem_access.offset = gpa & ~PAGE_MASK; + + return monitor_traps(current, true, &req) >= 0; +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 0144f92b98..94c7f2a80c 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -210,10 +210,16 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, return true; } } + + /* + * Try to avoid sending a mem event. Suppress events caused by page-walks + * by emulating but still checking mem_access violations. + */ if ( vm_event_check_ring(d->vm_event_monitor) && d->arch.monitor.inguest_pagefault_disabled && - npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */ + npfec.kind == npfec_kind_in_gpt ) { + v->arch.vm_event->send_event = true; hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op, X86_EVENT_NO_EC); return true; diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index f1af4f812a..325b44674d 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -49,6 +49,9 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type, unsigned int err, uint64_t cr2); bool hvm_monitor_emul_unimplemented(void); +bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec, + uint16_t kind); + #endif /* __ASM_X86_HVM_MONITOR_H__ */ /* diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index 23e655710b..66db9e1e25 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -36,6 +36,8 @@ struct arch_vm_event { bool set_gprs; /* A sync vm_event has been sent and we're not done handling it. */ bool sync_event; + /* Send mem access events from emulator */ + bool send_event; }; int vm_event_init_domain(struct domain *d);
A/D bit writes (on page walks) can be considered benign by an introspection agent, so receiving vm_events for them is a pessimization. We try here to optimize by filtering these events out. Currently, we are fully emulating the instruction at RIP when the hardware sees an EPT fault with npfec.kind != npfec_kind_with_gla. This is, however, incorrect, because the instruction at RIP might legitimately cause an EPT fault of its own while accessing a _different_ page from the original one, where A/D were set. The solution is to perform the whole emulation, while ignoring EPT restrictions for the walk part, and taking them into account for the "actual" emulating of the instruction at RIP. When we send out a vm_event, we don't want the emulation to complete, since in that case we won't be able to veto whatever it is doing. That would mean that we can't actually prevent any malicious activity, instead we'd only be able to report on it. When we see a "send-vm_event" case while emulating, we need to first send the event out and then suspend the emulation (return X86EMUL_RETRY). After the emulation stops we'll call hvm_vm_event_do_resume() again after the introspection agent treats the event and resumes the guest. There, the instruction at RIP will be fully emulated (with the EPT ignored) if the introspection application allows it, and the guest will continue to run past the instruction. A common example is if the hardware exits because of an EPT fault caused by a page walk, p2m_mem_access_check() decides if it is going to send a vm_event. If the vm_event was sent and it would be treated so it runs the instruction at RIP, that instruction might also hit a protected page and provoke a vm_event. Now if npfec.kind == npfec_kind_in_gpt and d->arch.monitor.inguest_pagefault_disabled is true then we are in the page walk case and we can do this emulation optimization and emulate the page walk while ignoring the EPT, but don't ignore the EPT for the emulation of the actual instruction. In the first case we would have 2 EPT events, in the second case we would have 1 EPT event if the instruction at the RIP triggers an EPT event. We use hvmemul_map_linear_addr() to intercept write access and __hvm_copy() to intercept exec and read access. hvm_emulate_send_vm_event() can return false if there was no violation, if there was an error from monitor_traps() or p2m_get_mem_access(). Returning false if p2m_get_mem_access() fails is needed because the EPT entry will have rwx memory access rights. NOTE: hvm_emulate_send_vm_event() assumes the caller will check arch.vm_event->send_event Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- Changes since V9: - Remove the changes caused by moving the "goto" out of the loop in hvmemul_map_linear_addr(). - Update comment and commit message - Change function name to hvm_monitor_check_p2m(). --- xen/arch/x86/hvm/emulate.c | 11 ++++- xen/arch/x86/hvm/hvm.c | 8 ++++ xen/arch/x86/hvm/monitor.c | 75 +++++++++++++++++++++++++++++++ xen/arch/x86/mm/mem_access.c | 8 +++- xen/include/asm-x86/hvm/monitor.h | 3 ++ xen/include/asm-x86/vm_event.h | 2 + 6 files changed, 105 insertions(+), 2 deletions(-)