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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 22.12.21 11:49, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien


> 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)?
I think no, it can't. Otherwise I would notice it while testing in 
normal circumstances.

> 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.
I got it, so your explicit request to fail here still stands. Thank you 
for the clarification.

> Cheers,
--
Regards,

Oleksandr Tyshchenko