[PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[George Dunlap]

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

When running in PAE mode, Windows 7 (apparently) will occasionally
switch cr3 with one of the L3 entries invalid, make it valid, and then
expect the hardware to load the new value.  (This behavior is
explicitly not promised in the hardware manuals.)  This leads to a
situation where on a shadow fault, the guest walk succeeds but the
shadow walk fails.  The code assumes this can only happen when the
domain is dying, and makes an ASSERT() to that effect.  So currently,
in debug mode, this will cause the host to crash; in non-debug mode,
this will cause a page-fault loop.

The attached patch solves the problem by calling update_cr3() in that
path when the guest is in PAE mode, and only ASSERT()ing when the
guest is not in PAE mode.  The guest will get one spurious page fault,
but subsequent accesses will succeed.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

[-- Attachment #2: 20091016-update-cr3-on-shadow-failure.diff --]
[-- Type: text/x-diff, Size: 935 bytes --]

diff -r 4b78d565b535 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Wed Oct 07 14:12:09 2009 +0100
+++ b/xen/arch/x86/mm/shadow/multi.c	Thu Oct 15 15:48:52 2009 +0100
@@ -3194,7 +3194,16 @@
          * are OK, this can only have been caused by a failed
          * shadow_set_l*e(), which will have crashed the guest.
          * Get out of the fault handler immediately. */
+        /* Windows 7 apparently relies on the hardware to do something
+         * it explicitly hasn't promised to do: load l3 values after
+         * the cr3 is loaded.
+         * In any case, in the PAE case, the ASSERT is not true; it can
+         * happen because of actions the guest is taking. */
+#if GUEST_PAGING_LEVELS == 3
+        v->arch.paging.mode->update_cr3(v, 0);
+#else
         ASSERT(d->is_shutting_down);
+#endif
         shadow_unlock(d);
         trace_shadow_gen(TRC_SHADOW_DOMF_DYING, va);
         return 0;

[-- 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] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[John Levon]

On Fri, Oct 16, 2009 at 02:09:20PM +0100, George Dunlap wrote:

> domain is dying, and makes an ASSERT() to that effect.  So currently,
> in debug mode, this will cause the host to crash; in non-debug mode,
> this will cause a page-fault loop.

Isn't this a security hole then?

regards
john

Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[Keir Fraser]

On 17/10/2009 14:51, "John Levon" <levon@movementarian.org> wrote:

> On Fri, Oct 16, 2009 at 02:09:20PM +0100, George Dunlap wrote:
> 
>> domain is dying, and makes an ASSERT() to that effect.  So currently,
>> in debug mode, this will cause the host to crash; in non-debug mode,
>> this will cause a page-fault loop.
> 
> Isn't this a security hole then?

Debug builds shouldn't be used in production environments. The page-fault
loop, in the non-debug case, is preemptible, so the offending guest can only
waste its own scheduling quantum.

 -- Keir

Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[John Levon]

On Sat, Oct 17, 2009 at 03:15:35PM +0100, Keir Fraser wrote:

> > On Fri, Oct 16, 2009 at 02:09:20PM +0100, George Dunlap wrote:
> > 
> >> domain is dying, and makes an ASSERT() to that effect.  So currently,
> >> in debug mode, this will cause the host to crash; in non-debug mode,
> >> this will cause a page-fault loop.
> > 
> > Isn't this a security hole then?
> 
> Debug builds shouldn't be used in production environments. The page-fault
> loop, in the non-debug case, is preemptible, so the offending guest can only
> waste its own scheduling quantum.

OK, makes sense.

regards
john

Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[Paolo Bonzini]

> The attached patch solves the problem by calling update_cr3() in that
> path when the guest is in PAE mode, and only ASSERT()ing when the
> guest is not in PAE mode.  The guest will get one spurious page fault,
> but subsequent accesses will succeed.

Hi George,

I suppose this patch is also needed when running 32-on-64.  In that 
case, shouldn't the test be

#if GUEST_PAGING_LEVELS >= 3

rather than "== 3"?

Thanks,

Paolo

Re: Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[Tim Deegan]

Hi,

At 10:46 +0100 on 21 Oct (1256121960), Paolo Bonzini wrote:
> I suppose this patch is also needed when running 32-on-64.  In that 
> case, shouldn't the test be
> 
> #if GUEST_PAGING_LEVELS >= 3
> 
> rather than "== 3"?

No, '==' is correct.  GUEST_PAGING_LEVELS refers to the layout of the
_guest's_ pagetables, not the shadows.  GUEST_PAGING_LEVELS == 4 means
the guest is using full 64-bit pagetables, which don't have this quirk.

Cheers, 

Tim,
 

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

Re: Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[Paolo Bonzini]

On 10/21/2009 11:50 AM, Tim Deegan wrote:
> Hi,
>
> At 10:46 +0100 on 21 Oct (1256121960), Paolo Bonzini wrote:
>> I suppose this patch is also needed when running 32-on-64.  In that
>> case, shouldn't the test be
>>
>> #if GUEST_PAGING_LEVELS>= 3
>>
>> rather than "== 3"?
>
> No, '==' is correct.  GUEST_PAGING_LEVELS refers to the layout of the
> _guest's_ pagetables, not the shadows.  GUEST_PAGING_LEVELS == 4 means
> the guest is using full 64-bit pagetables, which don't have this quirk.

Right, I should have read arch/x86/mm/shadow/Makefile.

Thanks,

Paolo

Re: Re: [PATCH] Update cr3 in PAE mode when guest walk succeed but shadow walk fails

[George Dunlap]

Paolo Bonzini wrote:
> Right, I should have read arch/x86/mm/shadow/Makefile.
>   
Yes, the shadow code is as sane as it can possibly be... but still 
barking mad by any standard. :-)

 -George