[Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Aurelien Jarno]

Hi,

In qemu 0.10.X, sha1sum or gnupg segfault (both user and system mode) 
when used on i386 host / x86_64 target.

This has been fixed in trunk by the following commit.

| commit 8cd6345d00a25ffa8828bce31154c88f76fb7fc6
| Author: malc <malc@c046a42c-6fe2-441c-8c8c-71466251a162>
| Date:   Thu Apr 2 22:54:35 2009 +0000
|
|    Immediate versions of ro[lr]
|
|    git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@6968 c046a42c-6fe2-441c-8c8c-71466251a162


Actually I am not really convinced it has been fixed, I really think the
bug is still present, but not triggerable anymore this way.

It looks like very long translation are not stopped correctly. This part
of code looks suspicious:

        /* if too long translation, stop generation too */
        if (gen_opc_ptr >= gen_opc_end ||
            (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32) ||
            num_insns >= max_insns) {
            gen_jmp_im(pc_ptr - dc->cs_base);
            gen_eob(dc);
            break;
        }

If I understand correctly, when the end of the buffer is reached, the
translation is stopped, but some more opc are added by gen_jmp_im()
and gen_eob().

OTOH, on MIPS the following code leaves some space at the end of the 
buffer for a few more opc:

    /* Leave some spare opc slots for branch handling. */
    gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;

Applying the same changes to the x86_64 target fixes the bug. However, I
am not sure it is fully correct. Any comment?

Regards,
Aurelien

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Laurent Desnogues]

On Tue, Sep 22, 2009 at 5:49 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
[...]
>
> Actually I am not really convinced it has been fixed, I really think the
> bug is still present, but not triggerable anymore this way.
>
> It looks like very long translation are not stopped correctly. This part
> of code looks suspicious:
>
>        /* if too long translation, stop generation too */
>        if (gen_opc_ptr >= gen_opc_end ||
>            (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32) ||
>            num_insns >= max_insns) {
>            gen_jmp_im(pc_ptr - dc->cs_base);
>            gen_eob(dc);
>            break;
>        }
>
> If I understand correctly, when the end of the buffer is reached, the
> translation is stopped, but some more opc are added by gen_jmp_im()
> and gen_eob().
>
> OTOH, on MIPS the following code leaves some space at the end of the
> buffer for a few more opc:
>
>    /* Leave some spare opc slots for branch handling. */
>    gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;
>
> Applying the same changes to the x86_64 target fixes the bug. However, I
> am not sure it is fully correct. Any comment?

You mean that if you sub 16 and go back just previous malc's commit
you don't experience the crash anymore?

To me it looks like using gen_opc_buf + OPC_MAX_SIZE is rather
safe given that it gives room for 64 extra ops (cf exec-all.h).


Laurent

Re: [Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Aurelien Jarno]

On Tue, Sep 22, 2009 at 07:48:52PM +0200, Laurent Desnogues wrote:
> On Tue, Sep 22, 2009 at 5:49 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> [...]
> >
> > Actually I am not really convinced it has been fixed, I really think the
> > bug is still present, but not triggerable anymore this way.
> >
> > It looks like very long translation are not stopped correctly. This part
> > of code looks suspicious:
> >
> >        /* if too long translation, stop generation too */
> >        if (gen_opc_ptr >= gen_opc_end ||
> >            (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32) ||
> >            num_insns >= max_insns) {
> >            gen_jmp_im(pc_ptr - dc->cs_base);
> >            gen_eob(dc);
> >            break;
> >        }
> >
> > If I understand correctly, when the end of the buffer is reached, the
> > translation is stopped, but some more opc are added by gen_jmp_im()
> > and gen_eob().
> >
> > OTOH, on MIPS the following code leaves some space at the end of the
> > buffer for a few more opc:
> >
> >    /* Leave some spare opc slots for branch handling. */
> >    gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;
> >
> > Applying the same changes to the x86_64 target fixes the bug. However, I
> > am not sure it is fully correct. Any comment?
> 
> You mean that if you sub 16 and go back just previous malc's commit
> you don't experience the crash anymore?

Exactly, or even reverting his commit on the current tree.

> To me it looks like using gen_opc_buf + OPC_MAX_SIZE is rather
> safe given that it gives room for 64 extra ops (cf exec-all.h).
> 

I have played a bit with this number, it seems the buffer is too short
by 3 opc.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Aurelien Jarno]

On Tue, Sep 22, 2009 at 09:06:20PM +0200, Aurelien Jarno wrote:
> On Tue, Sep 22, 2009 at 07:48:52PM +0200, Laurent Desnogues wrote:
> > On Tue, Sep 22, 2009 at 5:49 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > [...]
> > >
> > > Actually I am not really convinced it has been fixed, I really think the
> > > bug is still present, but not triggerable anymore this way.
> > >
> > > It looks like very long translation are not stopped correctly. This part
> > > of code looks suspicious:
> > >
> > >        /* if too long translation, stop generation too */
> > >        if (gen_opc_ptr >= gen_opc_end ||
> > >            (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32) ||
> > >            num_insns >= max_insns) {
> > >            gen_jmp_im(pc_ptr - dc->cs_base);
> > >            gen_eob(dc);
> > >            break;
> > >        }
> > >
> > > If I understand correctly, when the end of the buffer is reached, the
> > > translation is stopped, but some more opc are added by gen_jmp_im()
> > > and gen_eob().
> > >
> > > OTOH, on MIPS the following code leaves some space at the end of the
> > > buffer for a few more opc:
> > >
> > >    /* Leave some spare opc slots for branch handling. */
> > >    gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;
> > >
> > > Applying the same changes to the x86_64 target fixes the bug. However, I
> > > am not sure it is fully correct. Any comment?
> > 
> > You mean that if you sub 16 and go back just previous malc's commit
> > you don't experience the crash anymore?
> 
> Exactly, or even reverting his commit on the current tree.
> 
> > To me it looks like using gen_opc_buf + OPC_MAX_SIZE is rather
> > safe given that it gives room for 64 extra ops (cf exec-all.h).
> > 
> 
> I have played a bit with this number, it seems the buffer is too short
> by 3 opc.
> 

After some debugging with Laurent on IRC (thanks!), the problem is that
some instructions (e.g. ror $0x1b,%eax) can generate up to 77 tcg op.
This is not consistent with this line from exec-all.h:
#define MAX_OP_PER_INSTR 64

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] sha1sum segfaults on x86_64 target / i386 host

[Aurelien Jarno]

On Tue, Sep 22, 2009 at 11:16:12PM +0200, Aurelien Jarno wrote:
> On Tue, Sep 22, 2009 at 09:06:20PM +0200, Aurelien Jarno wrote:
> > On Tue, Sep 22, 2009 at 07:48:52PM +0200, Laurent Desnogues wrote:
> > > On Tue, Sep 22, 2009 at 5:49 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > > [...]
> > > >
> > > > Actually I am not really convinced it has been fixed, I really think the
> > > > bug is still present, but not triggerable anymore this way.
> > > >
> > > > It looks like very long translation are not stopped correctly. This part
> > > > of code looks suspicious:
> > > >
> > > >        /* if too long translation, stop generation too */
> > > >        if (gen_opc_ptr >= gen_opc_end ||
> > > >            (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32) ||
> > > >            num_insns >= max_insns) {
> > > >            gen_jmp_im(pc_ptr - dc->cs_base);
> > > >            gen_eob(dc);
> > > >            break;
> > > >        }
> > > >
> > > > If I understand correctly, when the end of the buffer is reached, the
> > > > translation is stopped, but some more opc are added by gen_jmp_im()
> > > > and gen_eob().
> > > >
> > > > OTOH, on MIPS the following code leaves some space at the end of the
> > > > buffer for a few more opc:
> > > >
> > > >    /* Leave some spare opc slots for branch handling. */
> > > >    gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;
> > > >
> > > > Applying the same changes to the x86_64 target fixes the bug. However, I
> > > > am not sure it is fully correct. Any comment?
> > > 
> > > You mean that if you sub 16 and go back just previous malc's commit
> > > you don't experience the crash anymore?
> > 
> > Exactly, or even reverting his commit on the current tree.
> > 
> > > To me it looks like using gen_opc_buf + OPC_MAX_SIZE is rather
> > > safe given that it gives room for 64 extra ops (cf exec-all.h).
> > > 
> > 
> > I have played a bit with this number, it seems the buffer is too short
> > by 3 opc.
> > 
> 
> After some debugging with Laurent on IRC (thanks!), the problem is that
> some instructions (e.g. ror $0x1b,%eax) can generate up to 77 tcg op.
> This is not consistent with this line from exec-all.h:
> #define MAX_OP_PER_INSTR 64
> 

And you also have to add the size of jmp_im to this value.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net