Patchwork Change to correct PowerPC on a 64bit host

login
register
mail settings
Submitter Peter Maydell
Date Jan. 2, 2013, noon
Message ID <CAFEAcA8qORk2eAU+7EKWGeM3dycdnRwza2WF824=RGFEa+50Ug@mail.gmail.com>
Download mbox | patch
Permalink /patch/209020/
State New
Headers show

Comments

Peter Maydell - Jan. 2, 2013, noon
On 2 January 2013 04:58, Samuel Seay <lightningth@gmail.com> wrote:
> Attached is a patch for fixing bug #1052857. My local tests show it working
> properly on 32 and 64bit.

     err |= __put_user(set->sig[0] >> 32, &sc->_unused[3]);

This looks OK...


@@ -4606,8 +4606,6 @@ 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);
-
     if (err)
         goto sigsegv;

...but this bit doesn't. We need to save the old SP to the stack frame,
and your patch just skips this step. You're right that the line in question
is broken though; it has two problems:
 * it's using newsp (a guest address) as an argument to __put_user(),
   which wants a host address
 * it's using __put_user() which works on locked addresses, but newsp
   is below the area we locked with lock_user_struct earlier

Another dodgy line in this function:
    env->gpr[4] = (target_ulong) h2g(sc);
Since sc is an offset into the struct returned by lock_user_struct(),
if DEBUG_REMAP is defined then we're passing the guest a pointer
to memory that is free()d by unlock_user_struct(). This should probably
be setting gpr[4] to frame_addr + offsetof(something) instead.

-- PMM
Samuel Seay - Jan. 2, 2013, 1:01 p.m.
The VM I did the work in doesn't have internet access and I was unsure how
to do a text only email with gmail. With that said, the line that removed
the env->gpr[1] is redudant as a few lines below in the original source it
is set with newsp. The removed line would seg fault due to trying to write
the value of env->gpr[1] into newsp, which is not valid in host.

I can not speak to the line a bit further with h2g(sc).

Samuel

On Wed, Jan 2, 2013 at 7:00 AM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 2 January 2013 04:58, Samuel Seay <lightningth@gmail.com> wrote:
> > Attached is a patch for fixing bug #1052857. My local tests show it
> working
> > properly on 32 and 64bit.
>
> --- 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]);
>
> This looks OK...
>
>
> @@ -4606,8 +4606,6 @@ 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);
> -
>      if (err)
>          goto sigsegv;
>
> ...but this bit doesn't. We need to save the old SP to the stack frame,
> and your patch just skips this step. You're right that the line in question
> is broken though; it has two problems:
>  * it's using newsp (a guest address) as an argument to __put_user(),
>    which wants a host address
>  * it's using __put_user() which works on locked addresses, but newsp
>    is below the area we locked with lock_user_struct earlier
>
> Another dodgy line in this function:
>     env->gpr[4] = (target_ulong) h2g(sc);
> Since sc is an offset into the struct returned by lock_user_struct(),
> if DEBUG_REMAP is defined then we're passing the guest a pointer
> to memory that is free()d by unlock_user_struct(). This should probably
> be setting gpr[4] to frame_addr + offsetof(something) instead.
>
> -- PMM
>
Peter Maydell - Jan. 2, 2013, 2:02 p.m.
On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> The VM I did the work in doesn't have internet access and I was unsure how
> to do a text only email with gmail. With that said, the line that removed
> the env->gpr[1] is redudant as a few lines below in the original source it
> is set with newsp. The removed line would seg fault due to trying to write
> the value of env->gpr[1] into newsp, which is not valid in host.

No, it's not redundant -- we must save the old value of gpr[1], exactly
because we are about to change it (set it to newsp). The code is trying
to do the right thing (copy the old env->gpr[1] value into the guest
stack frame it is setting up) but in a broken way, so it must be fixed,
not just removed.

-- PMM
Samuel Seay - Jan. 2, 2013, 2:34 p.m.
I did not catch that, somehow I managed to invert the logic when looking at
it. Maybe a g2h() (such a macro exist? what would be the proper method?)
around the newsp value would do it. I'll redo that this evening and attempt
to submit a newer patch. Considering I don't have direct internet access in
the VM, any suggestions to make everyone happy on the patch submission?

I might be able to redo the git setup on my mac and do it from there.

Samuel

On Wed, Jan 2, 2013 at 9:02 AM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> > The VM I did the work in doesn't have internet access and I was unsure
> how
> > to do a text only email with gmail. With that said, the line that removed
> > the env->gpr[1] is redudant as a few lines below in the original source
> it
> > is set with newsp. The removed line would seg fault due to trying to
> write
> > the value of env->gpr[1] into newsp, which is not valid in host.
>
> No, it's not redundant -- we must save the old value of gpr[1], exactly
> because we are about to change it (set it to newsp). The code is trying
> to do the right thing (copy the old env->gpr[1] value into the guest
> stack frame it is setting up) but in a broken way, so it must be fixed,
> not just removed.
>
> -- PMM
>
Alexander Graf - Jan. 2, 2013, 2:47 p.m.
Please don't top post.

Am 02.01.2013 um 15:34 schrieb Samuel Seay <lightningth@gmail.com>:

> I did not catch that, somehow I managed to invert the logic when looking at it. Maybe a g2h() (such a macro exist? what would be the proper method?) around the newsp value would do it.

Sounds reasonable OTOH ;).

> I'll redo that this evening and attempt to submit a newer patch. Considering I don't have direct internet access in the VM, any suggestions to make everyone happy on the patch submission?

Sure. Just export your patch using git format-patch, copy it to your host and run git send-mail from there ;).

> 
> I might be able to redo the git setup on my mac and do it from there.

There are easy to install git packages for osx readily available, yeah.

Alex

> 
> Samuel
> 
> On Wed, Jan 2, 2013 at 9:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> > The VM I did the work in doesn't have internet access and I was unsure how
> > to do a text only email with gmail. With that said, the line that removed
> > the env->gpr[1] is redudant as a few lines below in the original source it
> > is set with newsp. The removed line would seg fault due to trying to write
> > the value of env->gpr[1] into newsp, which is not valid in host.
> 
> No, it's not redundant -- we must save the old value of gpr[1], exactly
> because we are about to change it (set it to newsp). The code is trying
> to do the right thing (copy the old env->gpr[1] value into the guest
> stack frame it is setting up) but in a broken way, so it must be fixed,
> not just removed.
> 
> -- PMM
>
Peter Maydell - Jan. 2, 2013, 2:53 p.m.
On 2 January 2013 14:34, Samuel Seay <lightningth@gmail.com> wrote:
> I did not catch that, somehow I managed to invert the logic when looking at
> it. Maybe a g2h() (such a macro exist? what would be the proper method?)
> around the newsp value would do it.

You want to use put_user() (without the __) -- this (a) takes a guest address
and (b) does the error checking for unwritable address, which __put_user
does not. [Don't be fooled by all the err |= __put_user code, this is bogus
because __put_user never fails.]

> I'll redo that this evening and attempt
> to submit a newer patch. Considering I don't have direct internet access in
> the VM, any suggestions to make everyone happy on the patch submission?

Use git format-patch in the VM to write the patch to file, copy the file out of
the VM and use git send-email to actually send it.

-- PMM

Patch

--- 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)