Re: [PATCH v2 1/2] domain: fix misaligned unmap address in {,un}map_guest_area()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 16, 2023 at 02:30:12PM +0200, Jan Beulich wrote:
> On 06.10.2023 15:00, Roger Pau Monne wrote:> --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, 
> > unsigned int size,
> >   unmap:
> >      if ( pg )
> >      {
> > -        unmap_domain_page_global(map);
> > +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
> >          put_page_and_type(pg);
> >      }
> >  
> > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct 
> > guest_area *area)
> >  
> >      if ( pg )
> >      {
> > -        unmap_domain_page_global(map);
> > +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
> >          put_page_and_type(pg);
> >      }
> >  }
> 
> On v1 in a reply to Julien you talk of "limiting misuse" by not relaxing
> expecations in Arm's backing code, but I wonder what kind of misuse you
> think about. Aiui there's no strong need to insist on page aligned input,
> and relaxing things there may simplify code elsewhere as well.

destroy_xen_mappings() both on Arm and x86 will trigger asserts if the
passed address is not page aligned.  I do think it makes sense to call
unmap_domain_page_global() with page-aligned addresses, as that could
help detect bogus callers or corrupted data passed as input.

IMO an assert for page aligned input address should be placed at
vunmap() in order to not get differing expectations on input address
being page aligned or not whether destroy_xen_mappings() or
map_pages_to_xen() is used.  map_pages_to_xen() doesn't require
page-aligned virtual addresses as input.

Roger.