[PATCH]Fix the bug of guest os installation failure and win2k boot failure

[PATCH]Fix the bug of guest os installation failure and win2k boot failure

[Xu, Dongxiao]

[-- Attachment #1: Type: text/plain, Size: 2760 bytes --]

Hi, Keir,

      This patch is to fix the problem of Linux guest installation failure and Windows 2000 boot failure.

      In the early code, we use vmx_vmexit_handler() -> vmx_io_instruction() function to emulate I/O instructions. But now, we use vmx_vmexit_handler() -> handle_mmio -> hvm_emulate_one() -> x86_emulate() to emulate I/O instructions. Also nowadays, the realmode emulation code walks through the path: vmx_realmode() -> realmode_emulate_one() -> hvm_emulate_one() -> x86_emulate().

      The I/O handle code in x86_emulate() checks the cpl and iopl value, and if cpl > iopl, it will generate a GP fault. This causes Linux guest installation failure and Windows 2000 boot failure. I think this check code may be not reasonable for two aspects: 

      1. If x86_emulate() is called from vmexit or from realmode emulation, I think this line of code is not needed, because:

            a). In I/O emulation, the cpu has already checked the cpl, iopl, and also the I/O bitmap before vmx_vmexit_handler() is called,
            b). For realmode, we shouldn't check the cpl and iopl, because any I/O operation is permitted in realmode.

      2. If x86_emulate() is called from multi.c, which emulates up to four instructions when dealing with PAE guest page tables. In this condition, the check is needed, but it is not correct, it should follow the code as follows, which is stated in the Intel SDM:

　If (cpl <= iopl)
　    Do I/O operation;
　Else {
　    If (I/O permission bit for the port == 0)
　        Do I/O operation;
　    Else 
　        Generate GP fault;
　} 

      Now this patch remove the cpl and iopl check in I/O handler code in x86_emulate() function. And it checks the four instructions which would be emulated by multi.c, if any of them is IN/INS/OUT/OUTS, or REP IN/INS/OUT/OUTS, we will break that four-instruction emulation, and let the I/O instruction walk through the path of vmx_vmexit_handler() -> handle_mmio -> hvm_emulate_one() -> x86_emulate().

      Another way to solve this issue could be that, we put the entire io permission check in x86_emulate(), and use a flag to indicate whether we should do the check. If x86_emulate() is called by vmexit or realmode emulation, we skip this check; if it is called by multi.c, then we do the io permission check. But it may be a bit complex for hypervisor to read guest process’s TSS and find and check its io bitmap. 

BTW: Why the existence code doesn't check the LOCK prefix (which should cause #UD injected to guest)
      
      Welcome for your comment, thanks! 

Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>

Best Regards,
--Dongxiao


[-- Attachment #2: io_check.patch --]
[-- Type: application/octet-stream, Size: 2618 bytes --]

Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>

diff -r 59b8768d0d0d xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Wed Mar 05 11:18:25 2008 +0000
+++ b/xen/arch/x86/mm/shadow/multi.c	Mon Mar 17 14:59:03 2008 +0800
@@ -3165,8 +3165,40 @@ static int sh_page_fault(struct vcpu *v,
          * the "second half" of a 64-bit pagetable write. */
         for ( i = 0 ; i < 4 ; i++ )
         {
+            unsigned long ins, rc;
+            uint8_t b;
             shadow_continue_emulation(&emul_ctxt, regs);
             v->arch.paging.last_write_was_pt = 0;
+
+            rc = emul_ops->insn_fetch(x86_seg_cs, 
+                regs->eip + sizeof(uint8_t), 
+                &ins, sizeof(uint8_t), &emul_ctxt.ctxt);
+            if ( rc )
+                break;
+
+            b = (uint8_t)ins;
+
+            /* If the instruction is IN/INS/OUT/OUTS */
+            if ((b >= 0xe4 && b <= 0xe7) || (b >= 0xec && b <= 0xef) 
+                || (b >= 0x6c && b <= 0x6f))
+                break;
+            /* If the instruction is a REP prefix */
+            else if (b == 0xf3)
+            {
+                rc = emul_ops->insn_fetch(x86_seg_cs, 
+                    regs->eip + 2*sizeof(uint8_t), 
+                    &ins, sizeof(uint8_t), &emul_ctxt.ctxt);
+                if ( rc )
+                    break;
+
+                b = (uint8_t)ins;
+
+                /* If the instruction after the REP prefix is IN/INS/OUT/OUTS */
+                if ((b >= 0xe4 && b <= 0xe7) || (b >= 0xec && b <= 0xef) 
+                    || (b >= 0x6c && b <= 0x6f)) 
+                    break;
+            } 
+
             r = x86_emulate(&emul_ctxt.ctxt, emul_ops);
             if ( r == X86EMUL_OKAY )
             {
diff -r 59b8768d0d0d xen/arch/x86/x86_emulate.c
--- a/xen/arch/x86/x86_emulate.c	Wed Mar 05 11:18:25 2008 +0000
+++ b/xen/arch/x86/x86_emulate.c	Mon Mar 17 14:59:03 2008 +0800
@@ -2255,7 +2255,6 @@ x86_emulate(
 
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
         unsigned long nr_reps = get_rep_prefix();
-        generate_exception_if(!mode_iopl(), EXC_GP, 0);
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
@@ -2818,7 +2817,6 @@ x86_emulate(
         unsigned int port = ((b < 0xe8)
                              ? insn_fetch_type(uint8_t)
                              : (uint16_t)_regs.edx);
-        generate_exception_if(!mode_iopl(), EXC_GP, 0);
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         if ( b & 2 )
         {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]Fix the bug of guest os installation failure and win2k boot failure

[Keir Fraser]

Hi Dongxiao,

Thanks for tracking this one down this far! I'm not fully convinced by your
analysis however, for the following reasons:

 1. In real mode we must surely have CPL==SS.DPL==0, otherwise the write of
CR0.PE would have been disallowed. Hence all I/O port operations must surely
succeed in our emulated real mode.

 2. I don't understand your changes to the shadow emulation path. Firstly,
the shadow emulator does not provide callback functions for I/O-port
operations, so they can never be emulated. Secondly, even if we do fall into
one of the generate_exception_if(!mode_iopl(), EXC_GP) statements, the
shadow emulator does not provide an inject_hw_exception() callback and hence
no exception will be generated and instead the emulation will (correctly)
fail.

This makes sense, since the nasty guest-installation problems have only
appeared (as far as I know) since the old I/O vmexit path was changed to use
x86_emulate(). Before that the 4-instruction shadow emulator and also the
real-mode emulator were working pretty much fine!

So... with regard to what is wrong with the I/O vmexit path. I agree that
the CPL-IOPL check is redundant, but it *should* work! Are we simply falling
down because we are not also checking the TSS bitmap?

Removing the IOPL checks from x86_emulate() may be the right thing to do,
but I would like to really understand the underlying root-cause problem
first.

 -- Keir

On 17/3/08 08:08, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir,
> 
>       This patch is to fix the problem of Linux guest installation failure and
> Windows 2000 boot failure.
> 
> ????? In the early code, we use vmx_vmexit_handler() -> vmx_io_instruction()
> function to emulate I/O instructions. But now, we use vmx_vmexit_handler() ->
> handle_mmio -> hvm_emulate_one() -> x86_emulate() to emulate I/O instructions.
> Also nowadays, the realmode emulation code walks through the path:
> vmx_realmode() -> realmode_emulate_one() -> hvm_emulate_one() ->
> x86_emulate().
> 
> ????? The I/O handle code in x86_emulate() checks the cpl and iopl value, and
> if cpl > iopl, it will generate a GP fault. This causes Linux guest
> installation failure and Windows 2000 boot failure. I think this check code
> may be not reasonable for two aspects:
> 
> ????? 1. If x86_emulate() is called from vmexit or from realmode emulation, I
> think this line of code is not needed, because:
> 
> ??????????? a). In I/O emulation, the cpu has already checked the cpl, iopl,
> and also the I/O bitmap before vmx_vmexit_handler() is called,
> ??????????? b). For realmode, we shouldn't check the cpl and iopl, because any
> I/O operation is permitted in realmode.
> 
> ????? 2. If x86_emulate() is called from multi.c, which emulates up to four
> instructions when dealing with PAE guest page tables. In this condition, the
> check is needed, but it is not correct, it should follow the code as follows,
> which is stated in the Intel SDM:
> 
> 　If (cpl <= iopl)
> 　??? Do I/O operation;
> 　Else {
> 　??? If (I/O permission bit for the port == 0)
> 　??????? Do I/O operation;
> 　??? Else 
> 　??????? Generate GP fault;
> 　} 
> 
> ????? Now this patch remove the cpl and iopl check in I/O handler code in
> x86_emulate() function. And it checks the four instructions which would be
> emulated by multi.c, if any of them is IN/INS/OUT/OUTS, or REP
> IN/INS/OUT/OUTS, we will break that four-instruction emulation, and let the
> I/O instruction walk through the path of vmx_vmexit_handler() -> handle_mmio
> -> hvm_emulate_one() -> x86_emulate().
> 
>       Another way to solve this issue could be that, we put the entire io
> permission check in x86_emulate(), and use a flag to indicate whether we
> should do the check. If x86_emulate() is called by vmexit or realmode
> emulation, we skip this check; if it is called by multi.c, then we do the io
> permission check. But it may be a bit complex for hypervisor to read guest
> process’s TSS and find and check its io bitmap.
> 
> BTW: Why the existence code doesn't check the LOCK prefix (which should cause
> #UD injected to guest)
>       
>       Welcome for your comment, thanks!
> 
> Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>
> 
> Best Regards,
> --Dongxiao
> 

RE: [PATCH]Fix the bug of guest os installation failure and win2k boot failure

[Xu, Dongxiao]

Hi, Keir, 

    Thanks for your response.

    Realmode emulation code path for I/O is fine. I just think that realmode I/O doesn't need that line of check code.

    For vmexit code path: When I debug this issue, I found that for some I/O vmexits which happened when /sbin/loader is executed, the cpl is 3, the iopl is 0, so when "generate_exception_if(!mode_iopl(), EXC_GP)" checks cpl and iopl relationship, it push out a GP fault and makes the guest installation fail. Actually before the code enters vmx_vmexit_handler(), the processor has already checked the I/O permission. So here I think that line of check code is not needed.

    Also we haven't found any bug caused by the 4-instruction emulation till now. Adding the change in the shadow code path is because: There may be I/O instructions among the 4 instructions in theory. In this case, I think a full check of cpl, iopl, and the I/O bitmap is needed. So we may either add the I/O permission check in software, or break the 4-instruction emulate and let processor do the I/O permission check, then emulate it by vmx_vmexit_handler()->handle_mmio() code path. Here the patch uses the second way. 

    There may be some points that I haven't considered. Thanks for comments!

Best Regards,
-- Dongxiao
    



-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2008年3月17日 16:32
To: Xu, Dongxiao; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installation failure and win2k boot failure

Hi Dongxiao,

Thanks for tracking this one down this far! I'm not fully convinced by your
analysis however, for the following reasons:

 1. In real mode we must surely have CPL==SS.DPL==0, otherwise the write of
CR0.PE would have been disallowed. Hence all I/O port operations must surely
succeed in our emulated real mode.

 2. I don't understand your changes to the shadow emulation path. Firstly,
the shadow emulator does not provide callback functions for I/O-port
operations, so they can never be emulated. Secondly, even if we do fall into
one of the generate_exception_if(!mode_iopl(), EXC_GP) statements, the
shadow emulator does not provide an inject_hw_exception() callback and hence
no exception will be generated and instead the emulation will (correctly)
fail.

This makes sense, since the nasty guest-installation problems have only
appeared (as far as I know) since the old I/O vmexit path was changed to use
x86_emulate(). Before that the 4-instruction shadow emulator and also the
real-mode emulator were working pretty much fine!

So... with regard to what is wrong with the I/O vmexit path. I agree that
the CPL-IOPL check is redundant, but it *should* work! Are we simply falling
down because we are not also checking the TSS bitmap?

Removing the IOPL checks from x86_emulate() may be the right thing to do,
but I would like to really understand the underlying root-cause problem
first.

 -- Keir

On 17/3/08 08:08, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir,
> 
>       This patch is to fix the problem of Linux guest installation failure and
> Windows 2000 boot failure.
> 
> ????? In the early code, we use vmx_vmexit_handler() -> vmx_io_instruction()
> function to emulate I/O instructions. But now, we use vmx_vmexit_handler() ->
> handle_mmio -> hvm_emulate_one() -> x86_emulate() to emulate I/O instructions.
> Also nowadays, the realmode emulation code walks through the path:
> vmx_realmode() -> realmode_emulate_one() -> hvm_emulate_one() ->
> x86_emulate().
> 
> ????? The I/O handle code in x86_emulate() checks the cpl and iopl value, and
> if cpl > iopl, it will generate a GP fault. This causes Linux guest
> installation failure and Windows 2000 boot failure. I think this check code
> may be not reasonable for two aspects:
> 
> ????? 1. If x86_emulate() is called from vmexit or from realmode emulation, I
> think this line of code is not needed, because:
> 
> ??????????? a). In I/O emulation, the cpu has already checked the cpl, iopl,
> and also the I/O bitmap before vmx_vmexit_handler() is called,
> ??????????? b). For realmode, we shouldn't check the cpl and iopl, because any
> I/O operation is permitted in realmode.
> 
> ????? 2. If x86_emulate() is called from multi.c, which emulates up to four
> instructions when dealing with PAE guest page tables. In this condition, the
> check is needed, but it is not correct, it should follow the code as follows,
> which is stated in the Intel SDM:
> 
> 　If (cpl <= iopl)
> 　??? Do I/O operation;
> 　Else {
> 　??? If (I/O permission bit for the port == 0)
> 　??????? Do I/O operation;
> 　??? Else 
> 　??????? Generate GP fault;
> 　} 
> 
> ????? Now this patch remove the cpl and iopl check in I/O handler code in
> x86_emulate() function. And it checks the four instructions which would be
> emulated by multi.c, if any of them is IN/INS/OUT/OUTS, or REP
> IN/INS/OUT/OUTS, we will break that four-instruction emulation, and let the
> I/O instruction walk through the path of vmx_vmexit_handler() -> handle_mmio
> -> hvm_emulate_one() -> x86_emulate().
> 
>       Another way to solve this issue could be that, we put the entire io
> permission check in x86_emulate(), and use a flag to indicate whether we
> should do the check. If x86_emulate() is called by vmexit or realmode
> emulation, we skip this check; if it is called by multi.c, then we do the io
> permission check. But it may be a bit complex for hypervisor to read guest
> process’s TSS and find and check its io bitmap.
> 
> BTW: Why the existence code doesn't check the LOCK prefix (which should cause
> #UD injected to guest)
>       
>       Welcome for your comment, thanks!
> 
> Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>
> 
> Best Regards,
> --Dongxiao
> 

Re: [PATCH]Fix the bug of guest os installation failure and win2k boot failure

[Keir Fraser]

On 17/3/08 10:06, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

>     Realmode emulation code path for I/O is fine. I just think that realmode
> I/O doesn't need that line of check code.

Okay, understood and I agree.

>     For vmexit code path: When I debug this issue, I found that for some I/O
> vmexits which happened when /sbin/loader is executed, the cpl is 3, the iopl
> is 0, so when "generate_exception_if(!mode_iopl(), EXC_GP)" checks cpl and
> iopl relationship, it push out a GP fault and makes the guest installation
> fail. Actually before the code enters vmx_vmexit_handler(), the processor has
> already checked the I/O permission. So here I think that line of check code is
> not needed.

Ah, and so we assume that the I/O access is actually permitted by the TSS
I/O bitmap? Seems reasonable, and so we could add that check or just remove
the CPL-IOPL check. I agree again.

>     Also we haven't found any bug caused by the 4-instruction emulation till
> now. Adding the change in the shadow code path is because: There may be I/O
> instructions among the 4 instructions in theory. In this case, I think a full
> check of cpl, iopl, and the I/O bitmap is needed. So we may either add the I/O
> permission check in software, or break the 4-instruction emulate and let
> processor do the I/O permission check, then emulate it by
> vmx_vmexit_handler()->handle_mmio() code path. Here the patch uses the second
> way. 

I think you misunderstand. The shadow emulator *never* emulates I/O port
accesses or exception deliveries. Those callback functions are simply not
implemented and are left as NULL. Hence we will fail the IOPL check, but we
will also fail to deliver EXC_GP, and so we will simply return
X86EMUL_UNHANDLEABLE, which is the right thing to do. So, no bug here and
your changes to the shadow emulator are not required.

I will try out a version of your patch which is basically just your
x86_emulate.c changes, and see how that works out for me.

 Thanks!
 Keir

RE: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Cui, Dexuan]

Keir Fraser wrote:
>>     For vmexit code path: When I debug this issue, I found that for
>> some I/O vmexits which happened when /sbin/loader is executed, the
>> cpl is 3, the iopl is 0, so when
>> "generate_exception_if(!mode_iopl(), EXC_GP)" checks cpl and iopl
>> relationship, it push out a GP fault and makes the guest
>> installation fail. Actually before the code enters
>> vmx_vmexit_handler(), the processor has already checked the I/O
>> permission. So here I think that line of check code is not needed. 
> 
> Ah, and so we assume that the I/O access is actually permitted by the
> TSS I/O bitmap? 

When a VMexit with reason IO_INSTRUCTION occurs, VMM can tell that: in
guest OS, 
the execution has enough privilege to access the port ( (an actually
virtual port, from VMM's 
perspective), because if ( vCPL > vIOPL || vTR.io_bitmap[port]==1), CPU
will raise #GP to guest directly without VM-exit.

> Seems reasonable, and so we could add that check or
> just remove the CPL-IOPL check. I agree again.
> 
>>     Also we haven't found any bug caused by the 4-instruction
>> emulation till now. Adding the change in the shadow code path is
>> because: There may be I/O instructions among the 4 instructions in
>> theory. In this case, I think a full check of cpl, iopl, and the I/O
>> bitmap is needed. So we may either add the I/O permission check in
>> software, or break the 4-instruction emulate and let processor do
>> the I/O permission check, then emulate it by
>> vmx_vmexit_handler()->handle_mmio() code path. Here the patch uses
>> the second way. 
> 
> I think you misunderstand. The shadow emulator *never* emulates I/O
> port accesses or exception deliveries. Those callback functions are
> simply not implemented and are left as NULL. 
Those callback functions -- what are they? -- do you mean the following?
static struct x86_emulate_ops hvm_emulate_ops = {
    ....
    .read_io       = hvmemul_read_io,
    .write_io      = hvmemul_write_io,
...
};


>Hence we will fail the
> IOPL check, but we will also fail to deliver EXC_GP, and so we will
> simply return X86EMUL_UNHANDLEABLE, which is the right thing to do.
> So, no bug here and your changes to the shadow emulator are not
> required. 
> 
> I will try out a version of your patch which is basically just your
> x86_emulate.c changes, and see how that works out for me.
> 
>  Keir


Thanks
-- Dexuan

Re: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Keir Fraser]

On 17/3/08 11:16, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

>> I think you misunderstand. The shadow emulator *never* emulates I/O
>> port accesses or exception deliveries. Those callback functions are
>> simply not implemented and are left as NULL.
> Those callback functions -- what are they? -- do you mean the following?
> static struct x86_emulate_ops hvm_emulate_ops = {
>     ....
>     .read_io       = hvmemul_read_io,
>     .write_io      = hvmemul_write_io,
> ...
> };

Yes indeed. Also, crucially, .inject_hw_exception. Without that
x86_emulate() is unable to inject any exception into the guest, and will
instead return X86EMUL_UNHANDLEABLE.

 -- Keir

RE: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Xu, Dongxiao]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi, Keir, 
    Now I understand what you mean. read_io, write_io, inject_hw_exception callbacks are not defined within the multi.c. So I/O instructions will not be emulated by it. Thanks for your comments. And the new patch is attached.

Best regards,
-- Dongxiao

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2008年3月17日 19:21
To: Cui, Dexuan; Xu, Dongxiao; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

On 17/3/08 11:16, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

>> I think you misunderstand. The shadow emulator *never* emulates I/O
>> port accesses or exception deliveries. Those callback functions are
>> simply not implemented and are left as NULL.
> Those callback functions -- what are they? -- do you mean the following?
> static struct x86_emulate_ops hvm_emulate_ops = {
>     ....
>     .read_io       = hvmemul_read_io,
>     .write_io      = hvmemul_write_io,
> ...
> };

Yes indeed. Also, crucially, .inject_hw_exception. Without that
x86_emulate() is unable to inject any exception into the guest, and will
instead return X86EMUL_UNHANDLEABLE.

 -- Keir



[-- Attachment #2: io_check.patch --]
[-- Type: application/octet-stream, Size: 1692 bytes --]

Remove the CPL and IOPL check in the I/O handling code in x86_emulate().
  - For Realmode I/O emulation, this check is not needed because any I/O
    operation should be permitted in this mode.
    For I/O emulation in vmx_vmexit_handler() code path, this check is also
    not needed because processor has already checked the I/O permission before
    vmx_vmexit_handler() is called.

Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>

diff -r 59b8768d0d0d xen/arch/x86/x86_emulate.c
--- a/xen/arch/x86/x86_emulate.c	Wed Mar 05 11:18:25 2008 +0000
+++ b/xen/arch/x86/x86_emulate.c	Tue Mar 18 09:42:20 2008 +0800
@@ -2255,7 +2255,6 @@ x86_emulate(
 
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
         unsigned long nr_reps = get_rep_prefix();
-        generate_exception_if(!mode_iopl(), EXC_GP, 0);
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
@@ -2285,7 +2284,6 @@ x86_emulate(
 
     case 0x6e ... 0x6f: /* outs %esi,%dx */ {
         unsigned long nr_reps = get_rep_prefix();
-        generate_exception_if(!mode_iopl(), EXC_GP, 0);
         dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         if ( (nr_reps > 1) && (ops->rep_outs != NULL) &&
              ((rc = ops->rep_outs(ea.mem.seg, truncate_ea(_regs.esi),
@@ -2818,7 +2816,6 @@ x86_emulate(
         unsigned int port = ((b < 0xe8)
                              ? insn_fetch_type(uint8_t)
                              : (uint16_t)_regs.edx);
-        generate_exception_if(!mode_iopl(), EXC_GP, 0);
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         if ( b & 2 )
         {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Keir Fraser]

We're on the same page now, except that I realised there is a TOCTTOU race
introduced by relying on the processor's permission check while re-fetching
the instruction from scratch in the hypervisor. This allows, in theory, a
multi-threaded process to rewrite the I/O-port accessing instruction after
the processor has fetched the instruction, and validated the access, but
before Xen re-fetches the instruction for emulation. Possibly we do not care
too much about this, since the process must already have some
I/O-port-access permissions, but equally I don't expect we fall into the
TSS-bitmap check all that often, it's not that hard to implement, and then
we are definitely safe.

 -- Keir

On 18/3/08 01:49, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir, 
>     Now I understand what you mean. read_io, write_io, inject_hw_exception
> callbacks are not defined within the multi.c. So I/O instructions will not be
> emulated by it. Thanks for your comments. And the new patch is attached.
> 
> Best regards,
> -- Dongxiao
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: 2008年3月17日 19:21
> To: Cui, Dexuan; Xu, Dongxiao; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure
> and win2k boot failure
> 
> On 17/3/08 11:16, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> 
>>> I think you misunderstand. The shadow emulator *never* emulates I/O
>>> port accesses or exception deliveries. Those callback functions are
>>> simply not implemented and are left as NULL.
>> Those callback functions -- what are they? -- do you mean the following?
>> static struct x86_emulate_ops hvm_emulate_ops = {
>>     ....
>>     .read_io       = hvmemul_read_io,
>>     .write_io      = hvmemul_write_io,
>> ...
>> };
> 
> Yes indeed. Also, crucially, .inject_hw_exception. Without that
> x86_emulate() is unable to inject any exception into the guest, and will
> instead return X86EMUL_UNHANDLEABLE.
> 
>  -- Keir
> 
> 

RE: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Xu, Dongxiao]

Hi, Keir, 
    Do you mean that in a multi-thread process, one thread issues an I/O operation, and in the time slot that just after the processor has fetched the instruction, validated the access, but before Xen re-fetches the instruction for emulation, another thread steals that I/O instruction and replace it with a new one? Maybe we can regard it as a kind of attack...
    This could be happen in theory, but I think other instruction emulation may also have this problem. In your last sentence, do you mean that we still need to do an entire I/O permission check (including CPL, IOPL, and TSS I/O bitmap) in x86_emulate() for safety consideration? Thanks! :-)

Best regards,
-- Dongxiao


-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2008年3月18日 16:46
To: Xu, Dongxiao; Cui, Dexuan; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

We're on the same page now, except that I realised there is a TOCTTOU race
introduced by relying on the processor's permission check while re-fetching
the instruction from scratch in the hypervisor. This allows, in theory, a
multi-threaded process to rewrite the I/O-port accessing instruction after
the processor has fetched the instruction, and validated the access, but
before Xen re-fetches the instruction for emulation. Possibly we do not care
too much about this, since the process must already have some
I/O-port-access permissions, but equally I don't expect we fall into the
TSS-bitmap check all that often, it's not that hard to implement, and then
we are definitely safe.

 -- Keir

On 18/3/08 01:49, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir, 
>     Now I understand what you mean. read_io, write_io, inject_hw_exception
> callbacks are not defined within the multi.c. So I/O instructions will not be
> emulated by it. Thanks for your comments. And the new patch is attached.
> 
> Best regards,
> -- Dongxiao
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: 2008年3月17日 19:21
> To: Cui, Dexuan; Xu, Dongxiao; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure
> and win2k boot failure
> 
> On 17/3/08 11:16, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> 
>>> I think you misunderstand. The shadow emulator *never* emulates I/O
>>> port accesses or exception deliveries. Those callback functions are
>>> simply not implemented and are left as NULL.
>> Those callback functions -- what are they? -- do you mean the following?
>> static struct x86_emulate_ops hvm_emulate_ops = {
>>     ....
>>     .read_io       = hvmemul_read_io,
>>     .write_io      = hvmemul_write_io,
>> ...
>> };
> 
> Yes indeed. Also, crucially, .inject_hw_exception. Without that
> x86_emulate() is unable to inject any exception into the guest, and will
> instead return X86EMUL_UNHANDLEABLE.
> 
>  -- Keir
> 
> 

Re: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Keir Fraser]

On 18/3/08 09:35, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

>     Do you mean that in a multi-thread process, one thread issues an I/O
> operation, and in the time slot that just after the processor has fetched the
> instruction, validated the access, but before Xen re-fetches the instruction
> for emulation, another thread steals that I/O instruction and replace it with
> a new one? Maybe we can regard it as a kind of attack...

We could regard it as that, since that is what it would be. :-)

>     This could be happen in theory, but I think other instruction emulation
> may also have this problem.

Which other instruction emulations? Can you give an example?

> In your last sentence, do you mean that we still
> need to do an entire I/O permission check (including CPL, IOPL, and TSS I/O
> bitmap) in x86_emulate() for safety consideration? Thanks! :-)

Yes. Like I said: the CPL-IOPL check is very cheap, the TSS bitmap check is
a little more expensive but probably relatively rare. And in any case the
I/O port access latency is largely dominated by the VMEXIT/VMENTRY times.
Also the devices we emulate are mostly managed by mmio.

 -- Keir

RE: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Xu, Dongxiao]

Hi, Keir, 
    I think it is a common problem because there is always a time slot for all instruction emulation. In the time slot, an attacker could replace the old instruction with a new one. Just for example, if one thread issues a "push reg" operation, and during the time slot, another thread can replace it with a "pop reg" operation. Because there is no mechanism for us to check whether the instruction has been changed during that time slot. This may cause the guest OS doesn't work well. So, I think this kind of issue may not only happen with I/O emulations. :-)
    
Best regards,
-- Dongxiao

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2008年3月18日 18:03
To: Xu, Dongxiao; Cui, Dexuan; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

On 18/3/08 09:35, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

>     Do you mean that in a multi-thread process, one thread issues an I/O
> operation, and in the time slot that just after the processor has fetched the
> instruction, validated the access, but before Xen re-fetches the instruction
> for emulation, another thread steals that I/O instruction and replace it with
> a new one? Maybe we can regard it as a kind of attack...

We could regard it as that, since that is what it would be. :-)

>     This could be happen in theory, but I think other instruction emulation
> may also have this problem.

Which other instruction emulations? Can you give an example?

> In your last sentence, do you mean that we still
> need to do an entire I/O permission check (including CPL, IOPL, and TSS I/O
> bitmap) in x86_emulate() for safety consideration? Thanks! :-)

Yes. Like I said: the CPL-IOPL check is very cheap, the TSS bitmap check is
a little more expensive but probably relatively rare. And in any case the
I/O port access latency is largely dominated by the VMEXIT/VMENTRY times.
Also the devices we emulate are mostly managed by mmio.

 -- Keir

Re: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Keir Fraser]

That example is not a security hole, though. The worst the process can do is
shoot itself in the foot. The time slot exists for execution by the real
processor too, does it not?

 -- Keir

On 18/3/08 10:31, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir, 
>     I think it is a common problem because there is always a time slot for all
> instruction emulation. In the time slot, an attacker could replace the old
> instruction with a new one. Just for example, if one thread issues a "push
> reg" operation, and during the time slot, another thread can replace it with a
> "pop reg" operation. Because there is no mechanism for us to check whether the
> instruction has been changed during that time slot. This may cause the guest
> OS doesn't work well. So, I think this kind of issue may not only happen with
> I/O emulations. :-)
>     
> Best regards,
> -- Dongxiao
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: 2008年3月18日 18:03
> To: Xu, Dongxiao; Cui, Dexuan; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH]Fix the bug of guest os installationfailure
> and win2k boot failure
> 
> On 18/3/08 09:35, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:
> 
>>     Do you mean that in a multi-thread process, one thread issues an I/O
>> operation, and in the time slot that just after the processor has fetched the
>> instruction, validated the access, but before Xen re-fetches the instruction
>> for emulation, another thread steals that I/O instruction and replace it with
>> a new one? Maybe we can regard it as a kind of attack...
> 
> We could regard it as that, since that is what it would be. :-)
> 
>>     This could be happen in theory, but I think other instruction emulation
>> may also have this problem.
> 
> Which other instruction emulations? Can you give an example?
> 
>> In your last sentence, do you mean that we still
>> need to do an entire I/O permission check (including CPL, IOPL, and TSS I/O
>> bitmap) in x86_emulate() for safety consideration? Thanks! :-)
> 
> Yes. Like I said: the CPL-IOPL check is very cheap, the TSS bitmap check is
> a little more expensive but probably relatively rare. And in any case the
> I/O port access latency is largely dominated by the VMEXIT/VMENTRY times.
> Also the devices we emulate are mostly managed by mmio.
> 
>  -- Keir
> 
> 

Re: [PATCH]Fix the bug of guest os installationfailure and win2k boot failure

[Keir Fraser]

On 18/3/08 10:02, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> In your last sentence, do you mean that we still
>> need to do an entire I/O permission check (including CPL, IOPL, and TSS I/O
>> bitmap) in x86_emulate() for safety consideration? Thanks! :-)
> 
> Yes. Like I said: the CPL-IOPL check is very cheap, the TSS bitmap check is a
> little more expensive but probably relatively rare. And in any case the I/O
> port access latency is largely dominated by the VMEXIT/VMENTRY times. Also the
> devices we emulate are mostly managed by mmio.

I'll cook up a patch for this myself, by the way.

 -- Keir