Bug: fstenv is wrongly implemented

Bug: fstenv is wrongly implemented

[Ziqiao Kong]

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

Re: Bug: fstenv is wrongly implemented

[Paolo Bonzini]

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

Re: Bug: fstenv is wrongly implemented

[Ziqiao Kong]

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
>

Re: Bug: fstenv is wrongly implemented

[Paolo Bonzini]

On 02/04/21 10:56, Ziqiao Kong wrote:
> 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?

No, there isn't.

Paolo

Re: Bug: fstenv is wrongly implemented

[Peter Maydell]

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

Re: Bug: fstenv is wrongly implemented

[Ziqiao Kong]

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