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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

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