Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 02/12/2021 10:12, Jan Beulich wrote:
> On 02.12.2021 10:09, Julien Grall wrote:
> > Hi Jan,
> >
> > On 13/09/2021 07:41, Jan Beulich wrote:
> > > Without holding appropriate locks, attempting to remove a prior mapping
> > > of the underlying page is pointless, as the same (or another) mapping
> > > could be re-established by a parallel request on another vCPU. Move the
> > > code to Arm's gnttab_set_frame_gfn(). Of course this new placement
> > > doesn't improve things in any way as far as the security of grant status
> > > frame mappings goes (see XSA-379). Proper locking would be needed here
> > > to allow status frames to be mapped securely.
> > >
> > > In turn this then requires replacing the other use in
> > > gnttab_unpopulate_status_frames(), which yet in turn requires adjusting
> > > x86's gnttab_set_frame_gfn(). Note that with proper locking inside
> > > guest_physmap_remove_page() combined with checking the GFN's mapping
> > > there against the passed in MFN, there then is no issue with the
> > > involved multiple gnttab_set_frame_gfn()-s potentially returning varying
> > > values (due to a racing XENMAPSPACE_grant_table request).
> > >
> > > This, as a side effect, does away with gnttab_map_frame() having a local
> > > variable "gfn" which shadows a function parameter of the same name.
> > >
> > > Together with XSA-379 this points out that XSA-255's addition to
> > > gnttab_map_frame() was really useless.
> > >
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > As discussed in the thread, I expect the Arm part to be simplified after
> > Oleksandr's series. So for the Arm part:
> >
> > Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> Thanks, but let me ask for a clarification: Explicitly just the Arm part,
> or also the common code change? I ask because from the x86 side I already
> have an ack by Roger, but strictly speaking that doesn't cover common
> code.
Ah, I thought Roger gave a reviewed-by and therefore from the check-in 
policy be sufficient. The common parts look also fine to me. So feel 
free to extend my acked-by to include the common code.
Cheers,

--
Julien Grall