Jan Beulich
2011-Mar-21 11:45 UTC
[Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
The PSE handling doesn''t check bits 1...8 (or 1...9 for non-PAE guests)
being zero, thus allowing bad (not 2Mb/4Mb aligned) large pages to be
handled (afaict potentially allowing the guest to access foreign memory).
Below is a possible fix, but unfortunately it doesn''t work for
GUEST_PAGING_LEVELS == 2, since _PAGE_INVALID_BITS is zero
there. Would defining _PAGE_INVALID_BITS to any bit mask between
0x80000000 and 0xfffff000 there be in conflict with anything?
Jan
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -231,9 +231,15 @@ guest_walk_tables(struct vcpu *v, struct
/* _PAGE_PSE_PAT not set: remove _PAGE_PAT from flags. */
flags &= ~_PAGE_PAT;
+#define GUEST_L2_GFN_ALIGN (1 << (GUEST_L2_PAGETABLE_SHIFT - \
+ GUEST_L1_PAGETABLE_SHIFT))
+ if ( gfn_x(start) & (GUEST_L2_GFN_ALIGN - 1) & ~0x1 )
+ rc |= _PAGE_INVALID_BITS;
+
/* Increment the pfn by the right number of 4k pages.
- * The ~0x1 is to mask out the PAT bit mentioned above. */
- start = _gfn((gfn_x(start) & ~0x1) + guest_l1_table_offset(va));
+ * Mask out PAT and invalid bits. */
+ start = _gfn((gfn_x(start) & ~(GUEST_L2_GFN_ALIGN - 1)) +
+ guest_l1_table_offset(va));
gw->l1e = guest_l1e_from_gfn(start, flags);
gw->l1mfn = _mfn(INVALID_MFN);
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Tim Deegan
2011-Mar-21 12:33 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
At 11:45 +0000 on 21 Mar (1300707922), Jan Beulich wrote:> The PSE handling doesn''t check bits 1...8 (or 1...9 for non-PAE guests) > being zero, thus allowing bad (not 2Mb/4Mb aligned) large pages to be > handled (afaict potentially allowing the guest to access foreign memory).I don''t think this allows access to foreign memory, since these pagetables are in GFN-space.> Below is a possible fix, but unfortunately it doesn''t work for > GUEST_PAGING_LEVELS == 2, since _PAGE_INVALID_BITS is zero > there.What does real hardware do in this case? IIRC 32-bit non-PAE doesn''t have an "invlid bit set" error code to use in pagefaults. /me supposes he had better check... Tim.> Would defining _PAGE_INVALID_BITS to any bit mask between > 0x80000000 and 0xfffff000 there be in conflict with anything? > > --- a/xen/arch/x86/mm/guest_walk.c > +++ b/xen/arch/x86/mm/guest_walk.c > @@ -231,9 +231,15 @@ guest_walk_tables(struct vcpu *v, struct > /* _PAGE_PSE_PAT not set: remove _PAGE_PAT from flags. */ > flags &= ~_PAGE_PAT; > > +#define GUEST_L2_GFN_ALIGN (1 << (GUEST_L2_PAGETABLE_SHIFT - \ > + GUEST_L1_PAGETABLE_SHIFT)) > + if ( gfn_x(start) & (GUEST_L2_GFN_ALIGN - 1) & ~0x1 ) > + rc |= _PAGE_INVALID_BITS; > + > /* Increment the pfn by the right number of 4k pages. > - * The ~0x1 is to mask out the PAT bit mentioned above. */ > - start = _gfn((gfn_x(start) & ~0x1) + guest_l1_table_offset(va)); > + * Mask out PAT and invalid bits. */ > + start = _gfn((gfn_x(start) & ~(GUEST_L2_GFN_ALIGN - 1)) + > + guest_l1_table_offset(va)); > gw->l1e = guest_l1e_from_gfn(start, flags); > gw->l1mfn = _mfn(INVALID_MFN); > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-21 13:10 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
>>> On 21.03.11 at 13:33, Tim Deegan <Tim.Deegan@citrix.com> wrote: > At 11:45 +0000 on 21 Mar (1300707922), Jan Beulich wrote: >> The PSE handling doesn''t check bits 1...8 (or 1...9 for non-PAE guests) >> being zero, thus allowing bad (not 2Mb/4Mb aligned) large pages to be >> handled (afaict potentially allowing the guest to access foreign memory). > > I don''t think this allows access to foreign memory, since these > pagetables are in GFN-space.Yes, if this is really only GFN space, then it would "just" result in bad translations getting installed, possibly conflicting with others (e.g. in cache attributes).>> Below is a possible fix, but unfortunately it doesn''t work for >> GUEST_PAGING_LEVELS == 2, since _PAGE_INVALID_BITS is zero >> there. > > What does real hardware do in this case? IIRC 32-bit non-PAE doesn''t > have an "invlid bit set" error code to use in pagefaults. > /me supposes he had better check...The bits are marked reserved (minus the PSE-36 feature), and the manual doesn''t make any distinction between the various paging modes when it comes to error code bits other than bit 4. I therefore think that on newer CPUs you would see bit 3 set in this case (minus errata). Hence I think forcing a page fault in this case would be correct. Leaving aside the non-PAE case, does the fix presented look reasonable? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-Mar-21 13:57 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
At 13:10 +0000 on 21 Mar (1300713029), Jan Beulich wrote:> Yes, if this is really only GFN space, then it would "just" result in > bad translations getting installed, possibly conflicting with others > (e.g. in cache attributes).Yep; that''s no worse that anything else a guest can do. I''m pretty sure no guest installs unaligned PAE entries and then tries to use them. (The only case I _know_ of where the OS relies on h/w to enforce invalid-bit detection is Xen itself).> Leaving aside the non-PAE case, does the fix presented look > reasonable?Yes, it looks correct to me. Give me a Signed-off-by and I''ll apply it. Thanks, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-21 14:19 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
>>> On 21.03.11 at 14:57, Tim Deegan <Tim.Deegan@citrix.com> wrote: > At 13:10 +0000 on 21 Mar (1300713029), Jan Beulich wrote: >> Yes, if this is really only GFN space, then it would "just" result in >> bad translations getting installed, possibly conflicting with others >> (e.g. in cache attributes). > > Yep; that''s no worse that anything else a guest can do. I''m pretty sure > no guest installs unaligned PAE entries and then tries to use them. (The > only case I _know_ of where the OS relies on h/w to enforce invalid-bit > detection is Xen itself).... and luckily Xen itself no longer runs on non-PAE (so this doesn''t become an issue with nested virtualization).>> Leaving aside the non-PAE case, does the fix presented look >> reasonable? > > Yes, it looks correct to me. Give me a Signed-off-by and I''ll apply it.Going with the page-fault-less case then for non-PAE? If we want that case to produce reserved bit faults too, I admit I''d prefer to submit a complete patch (which is half the reason why this was an RFC, non-signed-off one)... If we want no fault there, I''d like to comment that way in the code (so the or-ing with zero won''t prompt later readers to wonder whether this isn''t a bug/oversight). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2011-Mar-21 14:50 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
At 14:19 +0000 on 21 Mar (1300717156), Jan Beulich wrote:> >> Leaving aside the non-PAE case, does the fix presented look > >> reasonable? > > > > Yes, it looks correct to me. Give me a Signed-off-by and I''ll apply it. > > Going with the page-fault-less case then for non-PAE? If we want > that case to produce reserved bit faults too, I admit I''d prefer to > submit a complete patch (which is half the reason why this was an > RFC, non-signed-off one)... If we want no fault there, I''d like to > comment that way in the code (so the or-ing with zero won''t > prompt later readers to wonder whether this isn''t a bug/oversight).OK, I''ll wait until I can fix up the non-PAE case too. Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-28 07:55 UTC
Re: [Xen-devel] [RFC] x86: possible problem with guest_walk_tables()
>>> On 21.03.11 at 15:50, Tim Deegan <Tim.Deegan@citrix.com> wrote: > At 14:19 +0000 on 21 Mar (1300717156), Jan Beulich wrote: >> >> Leaving aside the non-PAE case, does the fix presented look >> >> reasonable? >> > >> > Yes, it looks correct to me. Give me a Signed-off-by and I''ll apply it. >> >> Going with the page-fault-less case then for non-PAE? If we want >> that case to produce reserved bit faults too, I admit I''d prefer to >> submit a complete patch (which is half the reason why this was an >> RFC, non-signed-off one)... If we want no fault there, I''d like to >> comment that way in the code (so the or-ing with zero won''t >> prompt later readers to wonder whether this isn''t a bug/oversight). > > OK, I''ll wait until I can fix up the non-PAE case too.Jun points out that SDM tables 5-8 and 5-9 are pretty complete. According to them, only bit 21 set would cause a reserved bit fault. Which fits pretty badly into the present code, hence I''m not certain it''s worth to uglify the code just for this case. I''ll therefore submit the patch with just a comment added for now - later adjustments can still be done if desired. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel