Re: [Xen-devel] [PATCH 5/7] x86/shadow: Use the pagewalk reserved bits helpers

[Tim Deegan <tim@xxxxxxx>]

At 12:26 +0000 on 02 Mar (1488457613), Andrew Cooper wrote:
> On 01/03/17 16:03, Jan Beulich wrote:
> >>>> On 27.02.17 at 15:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> The shadow logic should never create a shadow of a guest PTE which contains
> >> reserved bits from the guests point of view.  Such a shadowed entry might 
> >> not
> >> cause #PF[RSVD] when walked by hardware, thus won't behave architecturally
> >> from the guests point of view.
> > But are we already or-ing in the RSVD bit accordingly in such cases,
> > before handing the #PF to the guest? The patch here certainly
> > doesn't make any change towards that, afaics.
> 
> The purpose of this patch is to ensure we never create a shadow which
> risks causing hardware to generate #PF[RSVD] when running on the
> shadows, other than the one deliberate case (MMIO fastpath).

Confusion! AIUI:

 - Shadows installed on demand in the pagefault handler are already
   correct.  If the guest PTE contained invalid bits we'd have injected
   a fault instead of shadowing it.

 - There is no risk of accidentally installing a shadow with reserved
   bits in it even if the guest pte has reserved bits in it.
   _sh_propagate() sanity-checks the flags, and the address bits
   come from the MFN (IOW we'd need a buggy p2m entry).  If that were
   a risk, I don't think this patch would solve it.

 - The potential bug that this patch tries to fix is:
   1. Guest writes a PTE with reserved bits in it.
   2. That gets shadowed by a write-to-pagetable path or a prefetch.
   3. The shadow is a valid PTE, so the guest gets no #PF, instead
      of #PF(rsvd).

Now by the same logic I used above there's probably no path
where a reserved _address_ bit causes a problem, but I see no harm
in centralising the logic and using the same code for these
paths as for the pt walker.

In answering this, I've spotted that the calls to
l1e_propagate_from_guest() in sh_resync_l1() and sh_prefetch()
aren't updated in this patch and should be. 

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel