Re: [PATCH 1/2] domain: fix misaligned unmap address in unmap_guest_area()

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

On Fri, Oct 06, 2023 at 11:08:09AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 06/10/2023 10:13, Roger Pau Monne wrote:
> > unmap_domain_page_global() expects the provided address to be page aligned, 
> > or
> > else some of the called functions will trigger assertions, like
> > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm.
> > 
> > The following assert has been reported by osstest arm 32bit tests:
> > 
> > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243
> > (XEN) ----[ Xen-4.18-rc  arm32  debug=y  Not tainted ]----
> > (XEN) CPU:    0
> > (XEN) PC:     00271a38 destroy_xen_mappings+0x50/0x5c
> > [...]
> > (XEN) Xen call trace:
> > (XEN)    [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC)
> > (XEN)    [<00235aa8>] vunmap+0x30/0x1a0 (LR)
> > (XEN)    [<0026ad88>] unmap_domain_page_global+0x10/0x20
> > (XEN)    [<00208e38>] unmap_guest_area+0x90/0xec
> > (XEN)    [<00208f98>] domain_kill+0x104/0x180
> > (XEN)    [<00239e3c>] do_domctl+0x8ac/0x14fc
> > (XEN)    [<0027ae34>] do_trap_guest_sync+0x570/0x66c
> > (XEN)    [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4
> > 
> > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > unmap_domain_page_global() and vunmap() should likely have the same 
> > alignment
> > asserts, as not all paths lead to detecting the misalignment of the provided
> > linear address.  Will do a separate patch.
> 
> unmap_domain_page() seems to be able to deal with unaligned pointer. And
> supporting unaligned pointer in vunmap()/unmap_domain_page_global() would
> simplifyy call to those functions.
> 
> So I would consider to deal with the alignment rather than adding ASSERT()
> in the two functions. destroy_xen_mappings() and modify_xen_mappings()
> should stay as-is though.
> 
> Anyway, I don't think this is a 4.18 material.

Maybe, I don't really have a strong opinion.  It seems more sane to me
to expect a page aligned linear address if the function is unmapping a
page, leaves less room for misuse IMO.

> > ---
> >   xen/common/domain.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index b8281d7cff9d..2dcc64e659cc 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -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));
> 
> Looking at the code, I can't find where 'map' may have become unaligned. Do
> you have a pointer?

In map_guest_area(), there's:

if ( ~gaddr ) /* Map (i.e. not just unmap)? */
{
    [...]
    map = __map_domain_page_global(pg);
    if ( !map )
    {
        put_page_and_type(pg);
        return -ENOMEM;
    }
    map += PAGE_OFFSET(gaddr);
}

> Depending on the answer, the call in map_guest_area() may also need some
> adjustment.

Indeed, I've missed that one, let me spin v2.

Thanks, Roger.