* Bug: fstenv is wrongly implemented
@ 2021-04-02 8:29 Ziqiao Kong
2021-04-02 8:45 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Ziqiao Kong @ 2021-04-02 8:29 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, ehabkost, rth
Hello everyone,
I am an active maintainer of Unicorn engine
(https://github.com/unicorn-engine). During my development, I found
that the fstenv implementation in qemu upstream is incorrect.
Below is the code snippet from target/i386/tcg/fpu_helper.c
(https://github.com/qemu/qemu/blob/266469947161aa10b1d36843580d369d5aa38589/target/i386/tcg/fpu_helper.c#L2393).
> cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
> cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
> cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
The value of fpip is wrongly set to 0, which should be env->fpip at
least I think. In real-world usage, the fstenv is often used to obtain
the current eip value from the FIP field in shellcode.
According to git blame, this bug is introduced about 13 years ago:
https://github.com/qemu/qemu/blame/633decd71119a4293e5e53e6059026c517a8bef0/target-i386/fpu_helper.c#L997.
We also had a patch for this bug:
https://github.com/unicorn-engine/unicorn/commit/59b09a71bfc6fd8b95357944f6be9aa54f424421
which you may refer to. I can also help draft a patch if necessary.
I'm pretty new to qemu-devel mail list and sorry for any violation of
your convention. Thanks in advance!
Ziqiao
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug: fstenv is wrongly implemented
2021-04-02 8:29 Bug: fstenv is wrongly implemented Ziqiao Kong
@ 2021-04-02 8:45 ` Paolo Bonzini
2021-04-02 8:56 ` Ziqiao Kong
2021-04-02 10:48 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-04-02 8:45 UTC (permalink / raw)
To: Ziqiao Kong, qemu-devel; +Cc: ehabkost, rth
On 02/04/21 10:29, Ziqiao Kong wrote:
>
> According to git blame, this bug is introduced about 13 years ago:
> https://github.com/qemu/qemu/blame/633decd71119a4293e5e53e6059026c517a8bef0/target-i386/fpu_helper.c#L997.
>
> We also had a patch for this bug:
> https://github.com/unicorn-engine/unicorn/commit/59b09a71bfc6fd8b95357944f6be9aa54f424421
> which you may refer to. I can also help draft a patch if necessary.
Hi!
Unfortunately the patch is incorrect, because fpu_update_ip is called
only at translation time and not at run-time. If more than one x87
instruction is present in the same translation block, or if a
translation block has been compiled after the one that is executing,
env->fpip will be incorrect.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug: fstenv is wrongly implemented
2021-04-02 8:45 ` Paolo Bonzini
@ 2021-04-02 8:56 ` Ziqiao Kong
2021-04-02 9:01 ` Paolo Bonzini
2021-04-02 10:48 ` Peter Maydell
1 sibling, 1 reply; 6+ messages in thread
From: Ziqiao Kong @ 2021-04-02 8:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, ehabkost, rth
Hi!
Thanks for your reply.
I read the IA32 manual just now and indeed the patch is not correct.
Is there any related patch for this bug?
Ziqiao
On Fri, Apr 2, 2021 at 4:45 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/04/21 10:29, Ziqiao Kong wrote:
> >
> > According to git blame, this bug is introduced about 13 years ago:
> > https://github.com/qemu/qemu/blame/633decd71119a4293e5e53e6059026c517a8bef0/target-i386/fpu_helper.c#L997.
> >
> > We also had a patch for this bug:
> > https://github.com/unicorn-engine/unicorn/commit/59b09a71bfc6fd8b95357944f6be9aa54f424421
> > which you may refer to. I can also help draft a patch if necessary.
>
> Hi!
>
> Unfortunately the patch is incorrect, because fpu_update_ip is called
> only at translation time and not at run-time. If more than one x87
> instruction is present in the same translation block, or if a
> translation block has been compiled after the one that is executing,
> env->fpip will be incorrect.
>
> Thanks,
>
> Paolo
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug: fstenv is wrongly implemented
2021-04-02 8:45 ` Paolo Bonzini
2021-04-02 8:56 ` Ziqiao Kong
@ 2021-04-02 10:48 ` Peter Maydell
2021-04-06 13:16 ` Ziqiao Kong
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2021-04-02 10:48 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Richard Henderson, Ziqiao Kong, Eduardo Habkost, QEMU Developers
On Fri, 2 Apr 2021 at 09:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/04/21 10:29, Ziqiao Kong wrote:
> >
> > According to git blame, this bug is introduced about 13 years ago:
> > https://github.com/qemu/qemu/blame/633decd71119a4293e5e53e6059026c517a8bef0/target-i386/fpu_helper.c#L997.
> >
> > We also had a patch for this bug:
> > https://github.com/unicorn-engine/unicorn/commit/59b09a71bfc6fd8b95357944f6be9aa54f424421
> > which you may refer to. I can also help draft a patch if necessary.
>
> Hi!
>
> Unfortunately the patch is incorrect, because fpu_update_ip is called
> only at translation time and not at run-time. If more than one x87
> instruction is present in the same translation block, or if a
> translation block has been compiled after the one that is executing,
> env->fpip will be incorrect.
I think this is https://bugs.launchpad.net/qemu/+bug/661696 ?
That had a patch attached which got some on-list discussion
back in 2010:
https://lists.gnu.org/archive/html/qemu-devel/2010-11/msg02497.html
The review comments may be of help in coming up with an updated patch.
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug: fstenv is wrongly implemented
2021-04-02 10:48 ` Peter Maydell
@ 2021-04-06 13:16 ` Ziqiao Kong
0 siblings, 0 replies; 6+ messages in thread
From: Ziqiao Kong @ 2021-04-06 13:16 UTC (permalink / raw)
To: Peter Maydell
Cc: Paolo Bonzini, Richard Henderson, QEMU Developers, Eduardo Habkost
Hi!
I checked that bug and list discussion and it indeed was the bug I
referred to. It seems that the patch should be quite straightforward
but no idea why the review of the patch was stalled at that time. I
would try to draft an updated patch these days.
Ziqiao
On Fri, Apr 2, 2021 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 2 Apr 2021 at 09:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 02/04/21 10:29, Ziqiao Kong wrote:
> > >
> > > According to git blame, this bug is introduced about 13 years ago:
> > > https://github.com/qemu/qemu/blame/633decd71119a4293e5e53e6059026c517a8bef0/target-i386/fpu_helper.c#L997.
> > >
> > > We also had a patch for this bug:
> > > https://github.com/unicorn-engine/unicorn/commit/59b09a71bfc6fd8b95357944f6be9aa54f424421
> > > which you may refer to. I can also help draft a patch if necessary.
> >
> > Hi!
> >
> > Unfortunately the patch is incorrect, because fpu_update_ip is called
> > only at translation time and not at run-time. If more than one x87
> > instruction is present in the same translation block, or if a
> > translation block has been compiled after the one that is executing,
> > env->fpip will be incorrect.
>
> I think this is https://bugs.launchpad.net/qemu/+bug/661696 ?
> That had a patch attached which got some on-list discussion
> back in 2010:
> https://lists.gnu.org/archive/html/qemu-devel/2010-11/msg02497.html
> The review comments may be of help in coming up with an updated patch.
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-06 13:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 8:29 Bug: fstenv is wrongly implemented Ziqiao Kong
2021-04-02 8:45 ` Paolo Bonzini
2021-04-02 8:56 ` Ziqiao Kong
2021-04-02 9:01 ` Paolo Bonzini
2021-04-02 10:48 ` Peter Maydell
2021-04-06 13:16 ` Ziqiao Kong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).