Re: [Xen-devel] [PATCH v14 07/11] x86/mm: add an extra command to HYPERVISOR_mmu_update...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

 -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 12 December 2017 14:39
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v14 07/11] x86/mm: add an extra command to
> HYPERVISOR_mmu_update...
> 
> >>> On 12.12.17 at 14:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 12/12/17 13:25, Jan Beulich wrote:
> >>>>> On 28.11.17 at 16:08, <paul.durrant@xxxxxxxxxx> wrote:
> >>> @@ -1905,7 +1906,8 @@ static int mod_l1_entry(l1_pgentry_t *pl1e,
> > l1_pgentry_t nl1e,
> >>>          }
> >>>
> >>>          /* Translate foreign guest address. */
> >>> -        if ( paging_mode_translate(pg_dom) )
> >>> +        if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
> >>> +             paging_mode_translate(pg_dom) )
> >>>          {
> >>>              p2m_type_t p2mt;
> >>>              p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
> >> Now that they're public - it was this change which led to the
> >> recognition of the issue XSA-248 describes (which in turn led to the
> >> other three). Without the fix for XSA-248 you'd have introduced a
> >> worse issue here, allowing writable mappings of page table pages
> >> rather than just r/o ones (leading to hypervisor crashes).
> >>
> >> Especially with the bypass of acquiring a writable page ref in
> >> get_page_from_l1e() for domains controlling shadow-external
> >> domains we need to be extremely careful with assigning page
> >> ownership. Before this series goes in I'd therefor like to ask you and
> >> others (especially people on the Cc list) to double check that the
> >> bypass introduced above doesn't allow for other (security) badness.
> >> I think I've sufficiently convinced myself that it doesn't, but this
> >> clearly wants double checking.
> >
> > Perhaps it is worth stepping back and considering the usecase from first
> > principles.
> 
> First of all, from your reply as a whole I can't judge whether you
> mean to say "all is fine, let's not think about it anymore" or "this
> needs to be redone from scratch" or anything in between.
> 
> > We are deliberately trying to introducing a mechanism whereby a
> > toolstack/device-mode/other semi-privileged entity can map resources
> > belonging to a guest which are not part of the guests physmap.  This is
> > because we deliberately want to move things like emulator rings out of
> > the guest physmap for attack surface reduction purposes.
> 
> Correct. What I was trying to point out with my reply is that the
> bypass here removes a check which previously we've been
> relying on: By finding the page in the guest's physmap, we can
> at least be certain that access to the page from outside of Xen is
> expected. With it removed, the only other check is the
> ownership one; the bypass in get_page_from_l1e() then blindly
> allows writable mappings to pages owned by the guest, even if
> they were shared r/o.
> 
> So while the relaxation here is deliberate _for the purposes the
> series intends_, we still need to make sure we don't open a path
> for device models to gain access to memory which they aren't
> supposed to be able to write (or just read).
> 

So, a suggestion would be to use some form of flag on the page (probably a PGC_ 
flag?) to tag it as a mappable resource. We can then white-list grant frames 
and ioreq frames with the new flag and then make sure use of 
MMU_PT_UPDATE_NO_TRANSLATE checks that the mfn is either in the guest P2M 
anyway, or tagged as a mappable resource?

Does that sound reasonable?

  Paul

> As you certainly realize, this would have happened if, long after
> having reviewed the patch, it hadn't occurred to me that there's
> problem here. Hence I think it is quite reasonable to take a step
> back and think through another time whether there isn't any
> further issue being introduced here.
> 
> Jan
> 
> > On top of that, it would be far more simple if the mechanism by which
> > this is achieved was compatible with the existing mapping interfaces.
> > One way or another, a PV guest needs to be able to construct a PTE for
> > these frames, and HVM guests need to be able to add these frames to its
> > physmap, and this looks very similar to foreign mapping.
> >
> > Other thoughts/suggestions welcome.
> >
> > ~Andrew
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel