Patchwork [v3] Modifications to fix PowerPC on 64bit host, bug 1052857

login
register
mail settings
Submitter Samuel Seay
Date Jan. 2, 2013, 8:53 p.m.
Message ID <1357160026-44371-1-git-send-email-LightningTH@GMail.com>
Download mbox | patch
Permalink /patch/209146/
State New
Headers show

Comments

Samuel Seay - Jan. 2, 2013, 8:53 p.m.
Removed h2g() macro around the ka->_sa_handler due to the _sa_handler being a guest memory address
Changed the __put_user to put_user as it was attempting to put a value at the stack address but the new address is a guest memory address, __put_user is for host memory addresses.
Patch v2 -> v3, removed unneeded casting in the put_user call left over from __put_user

Signed-off-by: Samuel Seay <LightningTH@GMail.com>
---
 linux-user/signal.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Alexander Graf - Jan. 3, 2013, 2:45 p.m.
On 02.01.2013, at 21:53, Samuel Seay wrote:

> Removed h2g() macro around the ka->_sa_handler due to the _sa_handler being a guest memory address
> Changed the __put_user to put_user as it was attempting to put a value at the stack address but the new address is a guest memory address, __put_user is for host memory addresses.
> Patch v2 -> v3, removed unneeded casting in the put_user call left over from __put_user
> 
> Signed-off-by: Samuel Seay <LightningTH@GMail.com>

Looks reasonable to me, but I'd like an ack from Peter.

Alex

> ---
> linux-user/signal.c |    4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 95e2ffa..c43b8ac 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -4584,7 +4584,7 @@ static void setup_frame(int sig, struct target_sigaction *ka,
> 
>     signal = current_exec_domain_sig(sig);
> 
> -    err |= __put_user(h2g(ka->_sa_handler), &sc->handler);
> +    err |= __put_user(ka->_sa_handler, &sc->handler);
>     err |= __put_user(set->sig[0], &sc->oldmask);
> #if defined(TARGET_PPC64)
>     err |= __put_user(set->sig[0] >> 32, &sc->_unused[3]);
> @@ -4606,7 +4606,7 @@ static void setup_frame(int sig, struct target_sigaction *ka,
> 
>     /* Create a stack frame for the caller of the handler.  */
>     newsp = frame_addr - SIGNAL_FRAMESIZE;
> -    err |= __put_user(env->gpr[1], (target_ulong *)(uintptr_t) newsp);
> +    err |= put_user(env->gpr[1], newsp, target_ulong);
> 
>     if (err)
>         goto sigsegv;
> -- 
> 1.7.9.5
> 
>
Peter Maydell - Jan. 3, 2013, 5:28 p.m.
On 3 January 2013 14:45, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.01.2013, at 21:53, Samuel Seay wrote:
>
>> Removed h2g() macro around the ka->_sa_handler due to the _sa_handler being a guest memory address
>> Changed the __put_user to put_user as it was attempting to put a value at the stack address but the new address is a guest memory address, __put_user is for host memory addresses.
>> Patch v2 -> v3, removed unneeded casting in the put_user call left over from __put_user
>>
>> Signed-off-by: Samuel Seay <LightningTH@GMail.com>
>
> Looks reasonable to me, but I'd like an ack from Peter.

Untested, and I haven't looked up the PPC ABI to check that the
function is overall doing the right thing, but with those caveats:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM
Alexander Graf - Jan. 3, 2013, 5:37 p.m.
On 03.01.2013, at 18:28, Peter Maydell wrote:

> On 3 January 2013 14:45, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 02.01.2013, at 21:53, Samuel Seay wrote:
>> 
>>> Removed h2g() macro around the ka->_sa_handler due to the _sa_handler being a guest memory address
>>> Changed the __put_user to put_user as it was attempting to put a value at the stack address but the new address is a guest memory address, __put_user is for host memory addresses.
>>> Patch v2 -> v3, removed unneeded casting in the put_user call left over from __put_user
>>> 
>>> Signed-off-by: Samuel Seay <LightningTH@GMail.com>
>> 
>> Looks reasonable to me, but I'd like an ack from Peter.
> 
> Untested, and I haven't looked up the PPC ABI to check that the
> function is overall doing the right thing, but with those caveats:
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Considering the state it was in before, I'd say applying the patch is an improvement regardless on how broken any code around it might be :).

Applied to ppc-next.


Alex
Peter Maydell - Jan. 3, 2013, 5:51 p.m.
On 3 January 2013 17:37, Alexander Graf <agraf@suse.de> wrote:
> On 03.01.2013, at 18:28, Peter Maydell wrote:
>> Untested, and I haven't looked up the PPC ABI to check that the
>> function is overall doing the right thing, but with those caveats:
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Considering the state it was in before, I'd say applying the patch
> is an improvement regardless on how broken any code around it
> might be :).

Forgot, this patch doesn't address the other issue I mentioned,
where

    env->gpr[4] = (target_ulong) h2g(sc);

is passing the guest a pointer to potentially about to be freed
memory and should be doing something like
   env->gpr[4] = frame_addr + offsetof(struct target_sigframe, sctx);
instead.

-- PMM

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 95e2ffa..c43b8ac 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -4584,7 +4584,7 @@  static void setup_frame(int sig, struct target_sigaction *ka,
 
     signal = current_exec_domain_sig(sig);
 
-    err |= __put_user(h2g(ka->_sa_handler), &sc->handler);
+    err |= __put_user(ka->_sa_handler, &sc->handler);
     err |= __put_user(set->sig[0], &sc->oldmask);
 #if defined(TARGET_PPC64)
     err |= __put_user(set->sig[0] >> 32, &sc->_unused[3]);
@@ -4606,7 +4606,7 @@  static void setup_frame(int sig, struct target_sigaction *ka,
 
     /* Create a stack frame for the caller of the handler.  */
     newsp = frame_addr - SIGNAL_FRAMESIZE;
-    err |= __put_user(env->gpr[1], (target_ulong *)(uintptr_t) newsp);
+    err |= put_user(env->gpr[1], newsp, target_ulong);
 
     if (err)
         goto sigsegv;