From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events. Date: Wed, 24 Sep 2014 23:24:23 +0200 Message-ID: References: <1411478070-13836-1-git-send-email-tklengyel@sec.in.tum.de> <1411478070-13836-16-git-send-email-tklengyel@sec.in.tum.de> <5422E4AA.4060600@linaro.org> <5422F695.3000809@linaro.org> <54232F23.50205@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4642982749079128474==" Return-path: In-Reply-To: <54232F23.50205@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============4642982749079128474== Content-Type: multipart/alternative; boundary=001a113a53d2d084570503d64ef6 --001a113a53d2d084570503d64ef6 Content-Type: text/plain; charset=ISO-8859-1 On Wed, Sep 24, 2014 at 10:52 PM, Julien Grall wrote: > Hello Tamas, > > On 24/09/2014 18:13, Tamas K Lengyel wrote: > >> >> >> On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall > > wrote: >> > >> > > + && hypercall_preempt_check() ) >> > > + { >> > > + rc = progress; >> > > + goto out; >> > >> > Jumping directly to the label "out" will skip flushing the TLB >> for the >> > domain. While it wasn't critical until now, partial redo during >> > insertion/allocation or hypercall preemption only for >> relinquish, the >> > guest may use the wrong permission because the TLB hasn't been >> flushed. >> > >> > At the same time, it looks like you never request to flush for >> the >> > MEMACCESS operation (see *flush = true). Does memaccess does a >> TLB flush >> > somewhere else? >> > >> > >> > Yes, at the end of p2m_set_mem_access once all PTEs are updated >> > successfully. I guess we could flush the TLB as we are progressing >> as >> > well, it wouldn't hurt. >> >> We should flush the TLB as we are progressing because the guest may >> technically continue to run... >> >> >> Hm, I think the guest is always paused while mem_access is being set via >> memop but sure, it can't hurt. >> > > I didn't find any domain pause call neither in the hypervisor nor in > xen-access. > > > Unless the pause is done by the hypervisor via the same hypercall, it's > safer to flush the TLB if it's necessary. > Ack, and you are right, I don't know why I thought it was paused. > > This case made me also think about another possible issue. Permission >> are checked in raw_copy_{from,to}_guest_helper during virtual address >> translation to a physical address. >> >> As you modified the attribute in the P2M, the copy may failed because >> of >> the lake of permission. >> >> >> I'm not entire sure what you mean. Can you elaborate? >> > > Xen has a bunch of functions raw_copy_{from,to}_guest helpers which copy > data from/to the guest. > > Since XSA-98, Xen checks that the guest has effectively the right to > read/write (depending of the helpers) a specific mapping before copying the > data. > Ah yes I remember seeing that, it's passed through get_page_from_gva and uses the MMU to do the translation directly. > > If the guest page doesn't have the good right, the helper will fail and > therefore so do the hypercall. > > When memaccess is used, a RAM page may have its permission lower down. > When the helpers are called, the code to check memory access during > permission violation will never be called... and the guest will receive an > hypercall failure. > > This is not the right behavior, the hypercall should only fail if the > permissions are effectively wrong after check mem access has been called. > Ack, I guess the straight forward solution here would be to forward the read/write event to the mem_access listener in the inline gvirt_to_maddr function. For mem_access however I should really include both the gvaddr and gpaddr values, which means the translation would happen twice, once with gva_to_ipa (that uses the MMU without the flag-based permission checks to translate), forward to mem_access, then do the check with the flag-based permission check again if mem_access is clear. Would that be an acceptable approach in your opinion? Tamas > > Regards, > > -- > Julien Grall > --001a113a53d2d084570503d64ef6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Wed, Sep 24, 2014 at 10:52 PM, Julien Grall <julien.grall@lin= aro.org> wrote:
Hello Tamas,

On 24/09/2014 18:13, Tamas K Lengyel wrote:


On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall <julien.grall@linaro.org
<= span class=3D""> <mailto:jul= ien.grall@linaro.org>> wrote:
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&& = hypercall_preempt_check() )
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 {
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D progres= s;
=A0 =A0 >=A0 =A0 =A0> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0Jumping directly to the label "out" will s= kip flushing the TLB for the
=A0 =A0 >=A0 =A0 =A0domain. While it wasn't critical until now, part= ial redo during
=A0 =A0 >=A0 =A0 =A0insertion/allocation or hypercall preemption only fo= r relinquish, the
=A0 =A0 >=A0 =A0 =A0guest may use the wrong permission because the TLB h= asn't been flushed.
=A0 =A0 >
=A0 =A0 >=A0 =A0 =A0At the same time, it looks like you never request to= flush for the
=A0 =A0 >=A0 =A0 =A0MEMACCESS operation (see *flush =3D true). Does mema= ccess does a TLB flush
=A0 =A0 >=A0 =A0 =A0somewhere else?
=A0 =A0 >
=A0 =A0 >
=A0 =A0 > Yes, at the end of p2m_set_mem_access once all PTEs are update= d
=A0 =A0 > successfully. I guess we could flush the TLB as we are progres= sing as
=A0 =A0 > well, it wouldn't hurt.

=A0 =A0 We should flush the TLB as we are progressing because the guest may=
=A0 =A0 technically continue to run...


Hm, I think the guest is always paused while mem_access is being set via memop but sure, it can't hurt.

I didn't find any domain pause call neither in the hypervisor nor in xe= n-access.


Unless the pause is done by the hypervisor via the same hypercall, it's= safer to flush the TLB if it's necessary.
<= /blockquote>

Ack, and you are right, I don't know wh= y I thought it was paused.
=A0

=A0 =A0 This case made me also think about another possible issue. Permissi= on
=A0 =A0 are checked in raw_copy_{from,to}_guest_helper during virtua= l address
=A0 =A0 translation to a physical address.

=A0 =A0 As you modified the attribute in the P2M, the copy may failed becau= se of
=A0 =A0 the lake of permission.


I'm not entire sure what you mean. Can you elaborate?

 Xen has a bunch of functions raw_copy_{from,to}_guest helpers which copy da= ta from/to the guest.

Since XSA-98, Xen checks that the guest has effectively the right to read/w= rite (depending of the helpers) a specific mapping before copying the data.=

Ah yes I remember seeing that, it'= s passed through get_page_from_gva and uses the MMU to do the translation d= irectly.
=A0

If the guest page doesn't have the good right, the helper will fail and= therefore so do the hypercall.

When memaccess is used, a RAM page may have its permission lower down.
When the helpers are called, the code to check memory access during permiss= ion violation will never be called... and the guest will receive an hyperca= ll failure.

This is not the right behavior, the hypercall should only fail if the permi= ssions are effectively wrong after check mem access has been called.

Ack, I guess the straight forward solution here would be= to forward the read/write event to the mem_access listener in the inline g= virt_to_maddr function. For mem_access however I should really include both= the gvaddr and gpaddr values, which means the translation would happen twi= ce, once with gva_to_ipa (that uses the MMU without the flag-based permissi= on checks to translate), forward to mem_access, then do the check with the = flag-based permission check again if mem_access is clear. Would that be an = acceptable approach in your opinion?

Tamas
= =A0

Regards,

--
Julien Grall

--001a113a53d2d084570503d64ef6-- --===============4642982749079128474== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4642982749079128474==--