[Qemu-devel] i386 emulation bug: mov reg, [addr]

[Qemu-devel] i386 emulation bug: mov reg, [addr]

[Clemens Kolbitsch]

Hi list,

I'm experiencing a strange emulation bug with the op-code below. The 
instruction raises a segfault in the application (running on the guest), 
however, if I enable KVM to run the exact same application, no segfault is 
raised.

0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]

where "11 22 33 44" is just some address. According to gdb (on a 32bit little-
endian machine), this instruction can be disassembled as a "mov address to 
reg-eax".

I have added some debugging code to the disas_insn function in translate.c to 
find out that the code is disassembled to the following blocks:

(NOTE: this debugging comes from an old qemu version where the old TB-style 
code was still used. HOWEVER, the same bug is still happening when used on the 
0.11.0 source branch).

0x0080023b: disassemble 7 bytes (to 0x00800242)
0x001: movl_A0_im 0x44332211
0x002: addl_A0_ESP_s1
0x003: ldl_user_T0_A0
0x004: movl_EAX_T0

So, as you can see, everything seems correct, but there is an additional 
(second) TB that messes everything up. In fact, the segfault happens because 
whatever is in ESP (shifted by one) is added to the address (which might then 
not be a valid address).

As I said, the code might crash in old versions of Qemu just like in the 
0.11.0 source branch and works fine if I use KVM (because the user code is not 
emulated of course).

Since this is such a fundamental problem, I don't quite understand how this 
could stay hidden so long... or maybe there is an error on my side :-/

Any help on this is greatly appreciated!!
--Clemens


---

some additional debugging information: I have traced down the problem to the 
following call in translate.c:

static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int 
*offset_ptr)
{
    ...
    if (s->aflag) {
        /// !!!!!!!!!! we take this path !!!!!!!!!!!!!!
        ...

        if (base == 4) {
            /// !!!!!!!!!! we take this path !!!!!!!!!!!!!!
            havesib = 1;
            code = ldub_code(s->pc++);
            scale = (code >> 6) & 3;
            index = ((code >> 3) & 7) | REX_X(s);
            base = (code & 7);
        }
        base |= REX_B(s);

        switch (mod) {
        ...
        default:
        case 2:
            /// !!!!!!!!!! 4byte load: we take this path !!!!!!!!!!!!!!
            disp = ldl_code(s->pc);
            s->pc += 4;
            break;
        }

        if (base >= 0) {
            /* for correct popl handling with esp */
            ...
        } else {
#ifdef TARGET_X86_64
            if (s->aflag == 2) {
                gen_op_movq_A0_im(disp);
            } else
#endif
            {
                /// !!!!!!!!!! this is still ok, we need the address !!!!!!!
                gen_op_movl_A0_im(disp);
            }
        }
        /* XXX: index == 4 is always invalid */
        if (havesib && (index != 4 || scale != 0)) {
#ifdef TARGET_X86_64
            if (s->aflag == 2) {
                gen_op_addq_A0_reg_sN(scale, index);
            } else
#endif
            {
                /// !!!!!!!!!! this does the evil !!!!!!!!!!!!!!
                gen_op_addl_A0_reg_sN(scale, index);
            }
        }

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Avi Kivity]

On 12/15/2009 08:48 PM, Clemens Kolbitsch wrote:
> Hi list,
>
> I'm experiencing a strange emulation bug with the op-code below. The
> instruction raises a segfault in the application (running on the guest),
> however, if I enable KVM to run the exact same application, no segfault is
> raised.
>
> 0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]
>
> where "11 22 33 44" is just some address. According to gdb (on a 32bit little-
> endian machine), this instruction can be disassembled as a "mov address to
> reg-eax".
>    

This is an odd encoding for this instruction, since there is a shorter 
one possible (8b 05 11 22 33 44).  So it is possible there is a bug in 
qemu that has never been triggered because compilers/assemblers don't 
generate this encoding.

btw, binutils disassembles this as

   8b 04 65 11 22 33 44     mov    0x44332211(,%eiz,2),%eax

I guess %eiz is some mnemonic for a "zero register" so the assembly can 
be reassembled into a 7-byte instruction later.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Jamie Lokier]

Avi Kivity wrote:
> On 12/15/2009 08:48 PM, Clemens Kolbitsch wrote:
> >Hi list,
> >
> >I'm experiencing a strange emulation bug with the op-code below. The
> >instruction raises a segfault in the application (running on the guest),
> >however, if I enable KVM to run the exact same application, no segfault is
> >raised.
> >
> >0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]
> >
> >where "11 22 33 44" is just some address. According to gdb (on a 32bit 
> >little-
> >endian machine), this instruction can be disassembled as a "mov address to
> >reg-eax".
> >   
> 
> This is an odd encoding for this instruction, since there is a shorter 
> one possible (8b 05 11 22 33 44).  So it is possible there is a bug in 
> qemu that has never been triggered because compilers/assemblers don't 
> generate this encoding.
> 
> btw, binutils disassembles this as
> 
>   8b 04 65 11 22 33 44     mov    0x44332211(,%eiz,2),%eax
> 
> I guess %eiz is some mnemonic for a "zero register" so the assembly can 
> be reassembled into a 7-byte instruction later.

That's right.  Gas accepts it if given the undocumented -mindex-reg
flag, apparently.  %eiz / eiz appears to be a Gas-specific invention,
not standard AT&T or Intel syntax.

-- Jamie

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Jamie Lokier]

Clemens Kolbitsch wrote:
>         /* XXX: index == 4 is always invalid */
>         if (havesib && (index != 4 || scale != 0)) {
> #ifdef TARGET_X86_64
>             if (s->aflag == 2) {
>                 gen_op_addq_A0_reg_sN(scale, index);
>             } else
> #endif
>             {
>                 /// !!!!!!!!!! this does the evil !!!!!!!!!!!!!!
>                 gen_op_addl_A0_reg_sN(scale, index);
>             }
>         }

This is indeed a bug.  Avi's explained why it doesn't trigger in
normal code.

When the index register is 4, which normally means %esp, in the SIB
encoding it means "no index".  Independent of the shift (scale).

So it should say:

         /* index == 4 means no index. */
         if (havesib && index != 4) {

But that said, I'm not sure if this line from earlier breaks the test:

            index = ((code >> 3) & 7) | REX_X(s);

When is REX_X(s) not zero, and does it break the index != 4 test?

-- Jamie

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[malc]

On Tue, 15 Dec 2009, Jamie Lokier wrote:

> Clemens Kolbitsch wrote:
> >         /* XXX: index == 4 is always invalid */
> >         if (havesib && (index != 4 || scale != 0)) {
> > #ifdef TARGET_X86_64
> >             if (s->aflag == 2) {
> >                 gen_op_addq_A0_reg_sN(scale, index);
> >             } else
> > #endif
> >             {
> >                 /// !!!!!!!!!! this does the evil !!!!!!!!!!!!!!
> >                 gen_op_addl_A0_reg_sN(scale, index);
> >             }
> >         }
> 
> This is indeed a bug.  Avi's explained why it doesn't trigger in
> normal code.
> 
> When the index register is 4, which normally means %esp, in the SIB
> encoding it means "no index".  Independent of the shift (scale).
> 
> So it should say:
> 
>          /* index == 4 means no index. */
>          if (havesib && index != 4) {
> 
> But that said, I'm not sure if this line from earlier breaks the test:
> 
>             index = ((code >> 3) & 7) | REX_X(s);
> 
> When is REX_X(s) not zero, and does it break the index != 4 test?

http://sandpile.org/aa64/opc_sib.htm

The code above is definitely incorrect in 32bit case.

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: i386 emulation bug: mov reg, [addr]

[Paolo Bonzini]

On 12/15/2009 10:26 PM, Jamie Lokier wrote:
> But that said, I'm not sure if this line from earlier breaks the test:
>
>              index = ((code>>  3)&  7) | REX_X(s);
>
> When is REX_X(s) not zero, and does it break the index != 4 test?

When %r12 (4+8=12) is used as an index.  That's a valid statement, so 
it's okay to test index != 4 (when REX_X(s) is not zero it is always 
eight, and you'd get index == 12):

    0:	8b 04 65 11 22 33 44    	mov    0x44332211(,%riz,2),%eax
    7:	42 8b 04 65 11 22 33 44 	mov    0x44332211(,%r12,2),%eax

(BTW %eiz/%riz are not accepted by my GAS, only produced by the 
disassembler).

Paolo

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Clemens Kolbitsch]

On Tuesday 15 December 2009 08:54:04 pm Avi Kivity wrote:
> On 12/15/2009 08:48 PM, Clemens Kolbitsch wrote:
> > Hi list,
> >
> > I'm experiencing a strange emulation bug with the op-code below. The
> > instruction raises a segfault in the application (running on the guest),
> > however, if I enable KVM to run the exact same application, no segfault
> > is raised.
> >
> > 0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]
> >
> > where "11 22 33 44" is just some address. According to gdb (on a 32bit
> > little- endian machine), this instruction can be disassembled as a "mov
> > address to reg-eax".
> 
> This is an odd encoding for this instruction, since there is a shorter
> one possible (8b 05 11 22 33 44).  So it is possible there is a bug in
> qemu that has never been triggered because compilers/assemblers don't
> generate this encoding.
> 
> btw, binutils disassembles this as
> 
>    8b 04 65 11 22 33 44     mov    0x44332211(,%eiz,2),%eax
> 
> I guess %eiz is some mnemonic for a "zero register" so the assembly can
> be reassembled into a 7-byte instruction later.

Hi all,
thanks for the quick replies. I also saw that the instruction is disassembled 
to the above instruction, but did not want to complicate my problem 
description :)
Is there anything I can provide to help testing possible patches?
--Clemens

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Avi Kivity]

On 12/16/2009 10:56 AM, Clemens Kolbitsch wrote:
> On Tuesday 15 December 2009 08:54:04 pm Avi Kivity wrote:
>    
>> On 12/15/2009 08:48 PM, Clemens Kolbitsch wrote:
>>      
>>> Hi list,
>>>
>>> I'm experiencing a strange emulation bug with the op-code below. The
>>> instruction raises a segfault in the application (running on the guest),
>>> however, if I enable KVM to run the exact same application, no segfault
>>> is raised.
>>>
>>> 0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]
>>>
>>> where "11 22 33 44" is just some address. According to gdb (on a 32bit
>>> little- endian machine), this instruction can be disassembled as a "mov
>>> address to reg-eax".
>>>        
>> This is an odd encoding for this instruction, since there is a shorter
>> one possible (8b 05 11 22 33 44).  So it is possible there is a bug in
>> qemu that has never been triggered because compilers/assemblers don't
>> generate this encoding.
>>
>> btw, binutils disassembles this as
>>
>>     8b 04 65 11 22 33 44     mov    0x44332211(,%eiz,2),%eax
>>
>> I guess %eiz is some mnemonic for a "zero register" so the assembly can
>> be reassembled into a 7-byte instruction later.
>>      
> Hi all,
> thanks for the quick replies. I also saw that the instruction is disassembled
> to the above instruction, but did not want to complicate my problem
> description :)
> Is there anything I can provide to help testing possible patches?
>    

A good first step is to write those possible patches.  It shouldn't be 
difficult, start in target-i386/translate.c:disas_insn().

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: i386 emulation bug: mov reg, [addr]

[Paolo Bonzini]

>> Is there anything I can provide to help testing possible patches?
>
> A good first step is to write those possible patches. It shouldn't be
> difficult, start in target-i386/translate.c:disas_insn().

And see Jamie's suggestion at

http://permalink.gmane.org/gmane.comp.emulators.qemu/59522

which is basically a patch written in English. :-)

Paolo

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Avi Kivity]

On 12/15/2009 11:26 PM, Jamie Lokier wrote:
>
> When is REX_X(s) not zero, and does it break the index != 4 test?
>
>    

Some rex bits do affect these tests, and some don't.  It's the usual 
'consistently inconsistent' rules of x86.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] i386 emulation bug: mov reg, [addr]

[Aurelien Jarno]

On Tue, Dec 15, 2009 at 07:48:53PM +0100, Clemens Kolbitsch wrote:
> Hi list,
> 
> I'm experiencing a strange emulation bug with the op-code below. The 
> instruction raises a segfault in the application (running on the guest), 
> however, if I enable KVM to run the exact same application, no segfault is 
> raised.
> 
> 0x0080023b:       8b 04 65 11 22 33 44    mov regEAX, [0x44332211]
> 
> where "11 22 33 44" is just some address. According to gdb (on a 32bit little-
> endian machine), this instruction can be disassembled as a "mov address to 
> reg-eax".
> 
> I have added some debugging code to the disas_insn function in translate.c to 
> find out that the code is disassembled to the following blocks:
> 
> (NOTE: this debugging comes from an old qemu version where the old TB-style 
> code was still used. HOWEVER, the same bug is still happening when used on the 
> 0.11.0 source branch).
> 
> 0x0080023b: disassemble 7 bytes (to 0x00800242)
> 0x001: movl_A0_im 0x44332211
> 0x002: addl_A0_ESP_s1
> 0x003: ldl_user_T0_A0
> 0x004: movl_EAX_T0
> 
> So, as you can see, everything seems correct, but there is an additional 
> (second) TB that messes everything up. In fact, the segfault happens because 
> whatever is in ESP (shifted by one) is added to the address (which might then 
> not be a valid address).
> 
> As I said, the code might crash in old versions of Qemu just like in the 
> 0.11.0 source branch and works fine if I use KVM (because the user code is not 
> emulated of course).
> 
> Since this is such a fundamental problem, I don't quite understand how this 
> could stay hidden so long... or maybe there is an error on my side :-/
> 
> Any help on this is greatly appreciated!!

I have just noticed the problem is not yet fixed, even if Jamie proposed
a patch in English. I have built a testcase (see below) and I have just
sent a patch to the mailing list.

Compile with: gcc -static -nostartfiles -m32 -o test test.S

        .data
msg_addr:	.long msg0

msg0:           .ascii "Hello World\n"
msg1:

        .text
        .globl _start

_start:
	mov  $4, %eax
	mov  $1, %ebx
	.byte 0x8b 
	.byte 0x0c
	.byte 0x65 
	.long msg_addr
        mov $(msg1-msg0), %edx
	int  $0x80

        mov $1, %eax
        int $0x80

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