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:
> 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.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?
> > 
> > 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?

My suggestion is going to work: "the compiler sees through casts"
referred to comparisons between pointers, where we temporarily casted
both pointers to integers and back to pointers via a MACRO. That case
was iffy because the MACRO was clearly a workaround the spec.

Here the situation is different. For one, we are doing arithmetic. Also
virt_to_maddr already takes a vaddr_t as argument. So instead of doing
pointers arithmetic, then converting to vaddr_t, we are converting to
vaddr_t first, then doing arithmetics, which is fine both from a C99
point of view and even a MISRA C point of view. I can't see a problem
with that. I am sure as I reasonable can be :-)

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