| Message ID | CAFEAcA8qORk2eAU+7EKWGeM3dycdnRwza2WF824=RGFEa+50Ug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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 >
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 >
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
--- 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)