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

Message ID 20191016105303.21948-1-julien.grall@arm.com
State New
Headers show
Series
  • [Xen-devel,for-4.13,v2] xen/arm: Don't use _end in is_xen_fixed_mfn()
Related show

Commit Message

Julien Grall Oct. 16, 2019, 10:53 a.m.
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@arm.com>
Release-acked-by: Juergen Gross <jgross@suse.com>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>

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.

    Changes in v2:
        - Cast _end to vaddr_t to prevent UB.
        - Add Juergen's released-acked-by
---
 xen/include/asm-arm/mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefano Stabellini Oct. 16, 2019, 3:22 p.m. | #1
On Wed, 16 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@arm.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> 
> 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.
> 
>     Changes in v2:
>         - Cast _end to vaddr_t to prevent UB.
>         - Add Juergen's released-acked-by
> ---
>  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..333efd3a60 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((vaddr_t)_end - 1)))
>  
>  #define page_get_owner(_p)    (_p)->v.inuse.domain
>  #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> -- 
> 2.11.0
>
Jan Beulich Oct. 23, 2019, 2:33 p.m. | #2
On 16.10.2019 12:53, 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@arm.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>
> 
> ---
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> 
> x86 seems to be affected by the off-by-one issue. Jan, Andrew?

Indeed. I'm in the process of making a patch.

Jan

Patch

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 262d92f18d..333efd3a60 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((vaddr_t)_end - 1)))
 
 #define page_get_owner(_p)    (_p)->v.inuse.domain
 #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))