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

[Julien Grall <julien@xxxxxxx>]

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.

> ---
>   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?
Depending on the answer, the call in map_guest_area() may also need some 
adjustment.
Cheers,

--
Julien Grall