Patchwork linux-user: fix segmentation fault passing with g2h(x) != x

login
register
mail settings
Submitter Alexander Graf
Date June 25, 2012, 5:32 p.m.
Message ID <1340645559-5448-1-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/167182/
State New
Headers show

Comments

Alexander Graf - June 25, 2012, 5:32 p.m.
When forwarding a segmentation fault into the guest process, we were passing
the host's address directly into the guest process's signal descriptor.

That obviously confused the guest process, since it didn't know what to make
of the (usually 32-bit truncated) address. Passing in g2h(address) makes the
guest process a lot happier.

This fixes java running in arm-linux-user for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 user-exec.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)
Peter Maydell - June 25, 2012, 5:33 p.m.
On 25 June 2012 18:32, Alexander Graf <agraf@suse.de> wrote:
> When forwarding a segmentation fault into the guest process, we were passing
> the host's address directly into the guest process's signal descriptor.
>
> That obviously confused the guest process, since it didn't know what to make
> of the (usually 32-bit truncated) address. Passing in g2h(address) makes the
> guest process a lot happier.
>
> This fixes java running in arm-linux-user for me.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
陳韋任 - June 26, 2012, 2:24 a.m.
On Mon, Jun 25, 2012 at 07:32:39PM +0200, Alexander Graf wrote:
> When forwarding a segmentation fault into the guest process, we were passing
> the host's address directly into the guest process's signal descriptor.
> 
> That obviously confused the guest process, since it didn't know what to make
> of the (usually 32-bit truncated) address. Passing in g2h(address) makes the
> guest process a lot happier.                         

  Passing g2h or h2g? From the context and code, I think h2g should make more
sense.

Regards,
chenwj
Alexander Graf - June 26, 2012, 3:47 a.m.
On 26.06.2012, at 04:24, 陳韋任 (Wei-Ren Chen) wrote:

> On Mon, Jun 25, 2012 at 07:32:39PM +0200, Alexander Graf wrote:
>> When forwarding a segmentation fault into the guest process, we were passing
>> the host's address directly into the guest process's signal descriptor.
>> 
>> That obviously confused the guest process, since it didn't know what to make
>> of the (usually 32-bit truncated) address. Passing in g2h(address) makes the
>> guest process a lot happier.                         
> 
>  Passing g2h or h2g? From the context and code, I think h2g should make more
> sense.

Yeah, it's s/g2h/h2g/g in all of the patch description and subject :). The patch is correct though.


Alex
Alexander Graf - July 11, 2012, 9:19 p.m.
On 25.06.2012, at 19:32, Alexander Graf wrote:

> When forwarding a segmentation fault into the guest process, we were passing
> the host's address directly into the guest process's signal descriptor.
> 
> That obviously confused the guest process, since it didn't know what to make
> of the (usually 32-bit truncated) address. Passing in g2h(address) makes the
> guest process a lot happier.
> 
> This fixes java running in arm-linux-user for me.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> user-exec.c |   25 +++++++++++++------------
> 1 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/user-exec.c b/user-exec.c
> index 36d29b4..83d2d44 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -100,19 +100,20 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>     /* Maybe we're still holding the TB fiddling lock? */
>     spin_unlock_safe(&tb_lock);
> 
> -    /* XXX: locking issue */
> -    if (is_write && h2g_valid(address)
> -        && page_unprotect(h2g(address), pc, puc)) {
> -        return 1;
> -    }
> +    if (h2g_valid(address)) {

This is broken. The address can be outside of RESERVED_VA, but still inside of the guest virtual address space, thus a valid segv.


Alex

> +        /* XXX: locking issue */
> +        if (is_write && page_unprotect(h2g(address), pc, puc)) {
> +            return 1;
> +        }
> 
> -    /* see if it is an MMU fault */
> -    ret = cpu_handle_mmu_fault(env, address, is_write, MMU_USER_IDX);
> -    if (ret < 0) {
> -        return 0; /* not an MMU fault */
> -    }
> -    if (ret == 0) {
> -        return 1; /* the MMU fault was handled without causing real CPU fault */
> +        /* see if it is an MMU fault */
> +        ret = cpu_handle_mmu_fault(env, h2g(address), is_write, MMU_USER_IDX);
> +        if (ret < 0) {
> +            return 0; /* not an MMU fault */
> +        }
> +        if (ret == 0) {
> +            return 1; /* the MMU fault was handled without causing real CPU fault */
> +        }
>     }
>     /* now we have a real cpu fault */
>     tb = tb_find_pc(pc);
> -- 
> 1.6.0.2
> 
>

Patch

diff --git a/user-exec.c b/user-exec.c
index 36d29b4..83d2d44 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -100,19 +100,20 @@  static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     /* Maybe we're still holding the TB fiddling lock? */
     spin_unlock_safe(&tb_lock);
 
-    /* XXX: locking issue */
-    if (is_write && h2g_valid(address)
-        && page_unprotect(h2g(address), pc, puc)) {
-        return 1;
-    }
+    if (h2g_valid(address)) {
+        /* XXX: locking issue */
+        if (is_write && page_unprotect(h2g(address), pc, puc)) {
+            return 1;
+        }
 
-    /* see if it is an MMU fault */
-    ret = cpu_handle_mmu_fault(env, address, is_write, MMU_USER_IDX);
-    if (ret < 0) {
-        return 0; /* not an MMU fault */
-    }
-    if (ret == 0) {
-        return 1; /* the MMU fault was handled without causing real CPU fault */
+        /* see if it is an MMU fault */
+        ret = cpu_handle_mmu_fault(env, h2g(address), is_write, MMU_USER_IDX);
+        if (ret < 0) {
+            return 0; /* not an MMU fault */
+        }
+        if (ret == 0) {
+            return 1; /* the MMU fault was handled without causing real CPU fault */
+        }
     }
     /* now we have a real cpu fault */
     tb = tb_find_pc(pc);