Re: [PATCH V4] xen/gnttab: Store frame GFN in struct page_info on Arm

[Julien Grall <julien@xxxxxxx>]

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