Hi Oleksandr,
On 14/12/2021 17:26, Oleksandr wrote:
@@ -1487,7 +1489,23 @@ int xenmem_add_to_physmap_one(
}
/* Map at new location. */
- rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+ if ( !p2m_is_ram(t) || !is_xen_heap_mfn(mfn) )
+ rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+ else
+ {
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+ p2m_write_lock(p2m);
+ if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)),
INVALID_GFN) )
+ {
+ rc = p2m_set_entry(p2m, gfn, 1, mfn, t,
p2m->default_access);
+ if ( !rc )
+ page_set_xenheap_gfn(mfn_to_page(mfn), gfn);
+ }
+ else
+ rc = -EBUSY;
May I suggest to avoid failing here when
page_get_xenheap_gfn(mfn_to_page(mfn))
matches the passed in GFN?
Good question...
There was an explicit request to fail here if page_get_xenheap_gfn()
returns a valid GFN.
From the other side, if old GFN matches new GFN we do not remove the
mapping in gnttab_set_frame_gfn(),
so probably we could avoid failing here in that particular case.
@Julien, what do you think?
I will answer by a question :). Can this situation happen in normal
circumstances (e.g. there is no concurrent call)?
The recent XSAs in the grant table code made me more cautious and I
would prefer if we fail more often than risking potentially introducing
yet another security issue. It is easy to relax afterwards if there are
legitimate use cases.
Cheers,
--
Julien Grall