On 15/10/2019 21:38, Stefano Stabellini wrote: > On Tue, 15 Oct 2019, Julien Grall wrote: >> 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.gr...@arm.com> >>>> >>>> --- >>>> >>>> Cc: Andrew Cooper <andrew.coop...@citrix.com> >>>> Cc: George Dunlap <george.dun...@eu.citrix.com> >>>> Cc: Ian Jackson <ian.jack...@eu.citrix.com> >>>> Cc: Jan Beulich <jbeul...@suse.com> >>>> Cc: Julien Grall <jul...@xen.org> >>>> Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> >>>> Cc: Stefano Stabellini <sstabell...@kernel.org> >>>> Cc: Tim Deegan <t...@xen.org> >>>> Cc: Wei Liu <w...@xen.org> >>>> Cc: jgr...@suse.com >>>> >>>> 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? > > Yes, there was a long discussion at the beginning of the year; it was > about how to define _start and _end so that we could avoid compilers > undefined behavior. The main underlying issue is that comparisons > between pointers to different objects are undefined [1] (_start and _end > can be interpreted as pointers to different objects). > > This case is a bit different, and easier. The issue is that, because the > result of "_end - 1" is not within the boundaries of the _end array, > then the operation is "undefined" by the C specification (C99 6.5.6). > Undefined is not good. > > So, I am not really asking for any complex fix, or hypervisor-wide > rework. I am only asking to avoid introducing new undefined behavior. > Casting to vaddr_t should solve it I think.
I agree that we should not add more undefined behavior in Xen. However, I don't like cast if they can't be justified. In this particular case, you seem to be unsure this is going to remove an undefined behavior. IIRC, I pointed out in the past that compiler can see through cast. So can we have some certainty that your suggestion is going to work? Cheers, > > [1] https://marc.info/?l=xen-devel&m=154904722227312 > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
