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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan