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é <[email protected]>
> > ---
> > 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.
