On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote: > On 22.09.2021 11:26, Roger Pau Monné wrote: > > On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: > >> On 21.09.2021 10:32, Roger Pau Monné wrote: > >>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: > >>>> On 20.09.2021 12:20, Roger Pau Monné wrote: > >>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: > >>>>>> --- a/xen/include/asm-arm/grant_table.h > >>>>>> +++ b/xen/include/asm-arm/grant_table.h > >>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || > >>>>>> \ > >>>>> > >>>>> I'm slightly confused by this checks, don't you need to check for > >>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call > >>>>> guest_physmap_remove_page? > >>>> > >>>> Why? It's ogfn which gets passed to the function. And it indeed is the > >>>> prior GFN's mapping that we want to remove here. > >>>> > >>>>> Or assuming that ogfn is not invalid can be used to imply a removal? > >>>> > >>>> That implication can be (and on x86 is) used for the incoming argument, > >>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. > >>> > >>> I guess I'm confused due to the ogfn checks done on the Arm side that > >>> are not performed on x86. So on Arm you always need to explicitly > >>> unhook the previous GFN before attempting to setup a new mapping, > >>> while on x86 you only need to do this when it's a removal in order to > >>> clear the entry? > >> > >> The difference isn't with guest_physmap_add_entry() (both x86 and > >> Arm only insert a new mapping there), but with > >> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior > >> mappings. And gnttab_map_frame() gets called only from there. This > >> is effectively what the first paragraph of the description is about. > > > > OK, sorry, it wasn't clear to me from the description. Could you > > explicitly mention in the description that the removal is moved into > > gnttab_set_frame_gfn on Arm in order to cope with the fact that > > xenmem_add_to_physmap_one doesn't perform it. > > Well, it's not really "in order to cope" - that's true for the placement > prior to this change as well, so not a justification for the change. > Nevertheless I've tried to make this more clear by changing the 1st > paragraph to: > > "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(); it cannot be dropped there since > xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). 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."
Thanks, this is indeed much clearer IMO: Acked-by: Roger Pau Monné <roger....@citrix.com> Albeit I still think we need to fix Arm side to do the removal in xenmem_add_to_physmap_one (or the x86 side to not do it). Thanks, Roger.
