|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()
Hi, On 15/10/2019 20:28, Stefano Stabellini wrote: > 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) > > ? I vaguely remember a lengthy discussion about it last year. But I don't think there was any conclusion in it. In this case, what are you trying to prevent with the cast? Is it underflow of an array? If so, how the cast is actually going to prevent the compiler to do the wrong thing? Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |