Re: [Xen-devel] [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 15 Oct 2019, Julien Grall wrote:
> virt_to_maddr() is using the hardware page-table walk instructions to
> translate a virtual address to physical address. The function should
> only be called on virtual address mapped.
> 
> _end points past the end of Xen binary and may not be mapped when the
> binary size is page-aligned. This means virt_to_maddr() will not be able
> to do the translation and therefore crash Xen.
> 
> Note there is also an off-by-one issue in this code, but the panic will
> trump that.
> 
> Both issues can be fixed by using _end - 1 in the check.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: jgross@xxxxxxxx
> 
> x86 seems to be affected by the off-by-one issue. Jan, Andrew?
> 
> This could be reached by a domain via XEN_SYSCTL_page_offline_op.
> However, the operation is not security supported (see XSA-77). So we are
> fine here.
> ---
>  xen/include/asm-arm/mm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 262d92f18d..174acd8859 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -153,7 +153,7 @@ extern unsigned long xenheap_base_pdx;
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
>      ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
> -     (mfn_to_maddr(mfn) <= virt_to_maddr(&_end)))
> +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))

Thank you for sending the patch and I think that "_end - 1" is the right
fix. I am just wondering whether we want/need an explicit cast of some
sort here, because technically _end is a char[] and 1 is a integer.
Maybe:

  ((vaddr_t)_end - 1)

?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel