When we (Yunhong and I) looked into the shadow code recently, we happened to find a possible shadow bug when emulating move to cr3. Current policy of emulating mov_to_cr3 is: - If guest changes guest_cr3: Update_cr3(v); Update shadow pointer (GUEST_CR3) in VMCS - Or else, only: shadow_update_cr3(v); The 2nd direction has the assumption that shadow l4 pointer doesn''t change if so do guest cr3. However the logic within shadow_update_cr3() doesn''t ensure this, which will sh_put_ref old shadow. Normally l4 shadow page is pinned and thus with refcount as 2. So above sh_put_ref doesn''t free this shadow page, and the original one will be re-chosed immediately. This is the normal case, however it doesn''t hold true once shadow_prealloc is invoked before this update. Shadow_prealloc() will unpin top level shadow pages, which made refcount of these pages decremented to 1 before shadow_update_cr3. Under this special case, sh_put_ref in the latter will really free the original l4 shadow, and then later a new l4 shadow will be allocated. This definitely breaks the assumption made by mov_to_cr3 emulation. Shadow pointer in VMCS will keep stale. So we''d better abandon the assumption and always update shadow pointer in VMCS whatever guest sets to its cr3. Does it sound OK? Or are we missing anything important? :-) Thanks, Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, At 17:25 +0800 on 28 Nov (1164734703), Tian, Kevin wrote:> However the logic within shadow_update_cr3() doesn''t ensure this, > which will sh_put_ref old shadow. Normally l4 shadow page is pinned > and thus with refcount as 2. So above sh_put_ref doesn''t free this > shadow page, and the original one will be re-chosed immediately. > > This is the normal case, however it doesn''t hold true once > shadow_prealloc is invoked before this update.Yes; that''s a bug in sh_set_toplevel_shadow(), since we don''t want to accidentally unshadow an entire process. I''ve fixed it to take the ref on the new contents before putting the ref on the old. Thanks, Tim. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>From: Tim Deegan [mailto:Tim.Deegan@xensource.com] >Sent: 2006年11月28日 17:43 > >Hi, > >At 17:25 +0800 on 28 Nov (1164734703), Tian, Kevin wrote: >> However the logic within shadow_update_cr3() doesn''t ensure this, >> which will sh_put_ref old shadow. Normally l4 shadow page is pinned >> and thus with refcount as 2. So above sh_put_ref doesn''t free this >> shadow page, and the original one will be re-chosed immediately. >> >> This is the normal case, however it doesn''t hold true once >> shadow_prealloc is invoked before this update. > >Yes; that''s a bug in sh_set_toplevel_shadow(), since we don''t want to >accidentally unshadow an entire process. I''ve fixed it to take the ref >on the new contents before putting the ref on the old. > >Thanks, > >Tim.Yes, that''s a clean fix. Thanks for doing this. Thanks, Kevin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel