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

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

Commit Message

Julien Grall Oct. 15, 2019, 6:18 p.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>

---

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>
Cc: jgross@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(-)

Comments

Stefano Stabellini Oct. 15, 2019, 7:28 p.m. | #1
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@arm.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>
> Cc: jgross@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)

?
Julien Grall Oct. 15, 2019, 7:47 p.m. | #2
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@arm.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>

>> Cc: jgross@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?

Cheers,

-- 
Julien Grall
Stefano Stabellini Oct. 15, 2019, 8:38 p.m. | #3
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@arm.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>
> >> Cc: jgross@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.

[1] https://marc.info/?l=xen-devel&m=154904722227312
Julien Grall Oct. 15, 2019, 9:08 p.m. | #4
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@arm.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>

>>>> Cc: jgross@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
Stefano Stabellini Oct. 16, 2019, 4:31 a.m. | #5
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@arm.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>
> >>>> Cc: jgross@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?

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 :-)
Jürgen Groß Oct. 16, 2019, 4:56 a.m. | #6
On 15.10.19 20:18, 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>

With or without the cast suggested by Stefano:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen
Ian Jackson Oct. 16, 2019, 10:18 a.m. | #7
Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()"):
> 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 :-)

FTAOD I think you are suggesting this:
 - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
 + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))

virt_to_maddr(va) is a macro which expands to
   __virt_to_maddr((vaddr_t)(va))

So what is happening here is that the cast to an integer type is being
done before the subtraction.

Without the cast, you are calculating the pointer value _end-1 from
the value _end, which is UB.  With the cast you are calculating an
integer value.  vaddr_t is unsigned, so all arithmetic operations are
defined.  Nothing casts the result back to the "forbidden" (with this
provenance) pointer value, so all is well.

(With the macro expansion the cast happens twice.  This is probably
better than using __virt_to_maddr here.)

Ie, in this case I agree with Stefano.  The cast is both necessary and
sufficient.

Ian.
George Dunlap Oct. 16, 2019, 10:22 a.m. | #8
On 10/16/19 11:18 AM, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()"):
>> 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 :-)
> 
> FTAOD I think you are suggesting this:
>  - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>  + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
> 
> virt_to_maddr(va) is a macro which expands to
>    __virt_to_maddr((vaddr_t)(va))
> 
> So what is happening here is that the cast to an integer type is being
> done before the subtraction.
> 
> Without the cast, you are calculating the pointer value _end-1 from
> the value _end, which is UB.  With the cast you are calculating an
> integer value.  vaddr_t is unsigned, so all arithmetic operations are
> defined.  Nothing casts the result back to the "forbidden" (with this
> provenance) pointer value, so all is well.
> 
> (With the macro expansion the cast happens twice.  This is probably
> better than using __virt_to_maddr here.)
> 
> Ie, in this case I agree with Stefano.  The cast is both necessary and
> sufficient.

Maybe I missed something, but why are we using `<=` at all?  Why not use
`<`?

Or is this some weird C pointer comparison UB thing?

 -George
Julien Grall Oct. 16, 2019, 10:31 a.m. | #9
Hi George,

On 16/10/2019 11:22, George Dunlap wrote:
> On 10/16/19 11:18 AM, Ian Jackson wrote:
>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()"):
>>> 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 :-)
>>
>> FTAOD I think you are suggesting this:
>>   - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>   + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>
>> virt_to_maddr(va) is a macro which expands to
>>     __virt_to_maddr((vaddr_t)(va))
>>
>> So what is happening here is that the cast to an integer type is being
>> done before the subtraction.
>>
>> Without the cast, you are calculating the pointer value _end-1 from
>> the value _end, which is UB.  With the cast you are calculating an
>> integer value.  vaddr_t is unsigned, so all arithmetic operations are
>> defined.  Nothing casts the result back to the "forbidden" (with this
>> provenance) pointer value, so all is well.
>>
>> (With the macro expansion the cast happens twice.  This is probably
>> better than using __virt_to_maddr here.)
>>
>> Ie, in this case I agree with Stefano.  The cast is both necessary and
>> sufficient.
> 
> Maybe I missed something, but why are we using `<=` at all?  Why not use
> `<`?
> 
> Or is this some weird C pointer comparison UB thing?

_end may not be mapped in the virtual address space. This is the case when the 
size of Xen is page-aligned. So _end will point to the next page.

virt_to_maddr() cannot fail so it should only be called on valid virtual 
address. The behavior is undefined in all the other cases.

On x86, you might be able to get away because you translate the virtual address 
to physical address in software.

On Arm, we are using the hardware instruction to do the translation. As _end is 
not always mapped, then the translation may fail. In other word, Xen will crash.

Cheers,
George Dunlap Oct. 16, 2019, 10:38 a.m. | #10
On 10/16/19 11:31 AM, Julien Grall wrote:
> Hi George,
> 
> On 16/10/2019 11:22, George Dunlap wrote:
>> On 10/16/19 11:18 AM, Ian Jackson wrote:
>>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
>>> _end in is_xen_fixed_mfn()"):
>>>> 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 :-)
>>>
>>> FTAOD I think you are suggesting this:
>>>   - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>>   + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>>
>>> virt_to_maddr(va) is a macro which expands to
>>>     __virt_to_maddr((vaddr_t)(va))
>>>
>>> So what is happening here is that the cast to an integer type is being
>>> done before the subtraction.
>>>
>>> Without the cast, you are calculating the pointer value _end-1 from
>>> the value _end, which is UB.  With the cast you are calculating an
>>> integer value.  vaddr_t is unsigned, so all arithmetic operations are
>>> defined.  Nothing casts the result back to the "forbidden" (with this
>>> provenance) pointer value, so all is well.
>>>
>>> (With the macro expansion the cast happens twice.  This is probably
>>> better than using __virt_to_maddr here.)
>>>
>>> Ie, in this case I agree with Stefano.  The cast is both necessary and
>>> sufficient.
>>
>> Maybe I missed something, but why are we using `<=` at all?  Why not use
>> `<`?
>>
>> Or is this some weird C pointer comparison UB thing?
> 
> _end may not be mapped in the virtual address space. This is the case
> when the size of Xen is page-aligned. So _end will point to the next page.
> 
> virt_to_maddr() cannot fail so it should only be called on valid virtual
> address. The behavior is undefined in all the other cases.
> 
> On x86, you might be able to get away because you translate the virtual
> address to physical address in software.
> 
> On Arm, we are using the hardware instruction to do the translation. As
> _end is not always mapped, then the translation may fail. In other word,
> Xen will crash.

None of this explains my question.

Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
`mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?

Under what conditions would they be different?

And if they're the same, then you can just use `<` instead of `<=`, and
not have to worry about casting before subtracting.

 -George
Jürgen Groß Oct. 16, 2019, 10:41 a.m. | #11
On 16.10.19 12:38, George Dunlap wrote:
> On 10/16/19 11:31 AM, Julien Grall wrote:
>> Hi George,
>>
>> On 16/10/2019 11:22, George Dunlap wrote:
>>> On 10/16/19 11:18 AM, Ian Jackson wrote:
>>>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
>>>> _end in is_xen_fixed_mfn()"):
>>>>> 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 :-)
>>>>
>>>> FTAOD I think you are suggesting this:
>>>>    - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>>>    + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>>>
>>>> virt_to_maddr(va) is a macro which expands to
>>>>      __virt_to_maddr((vaddr_t)(va))
>>>>
>>>> So what is happening here is that the cast to an integer type is being
>>>> done before the subtraction.
>>>>
>>>> Without the cast, you are calculating the pointer value _end-1 from
>>>> the value _end, which is UB.  With the cast you are calculating an
>>>> integer value.  vaddr_t is unsigned, so all arithmetic operations are
>>>> defined.  Nothing casts the result back to the "forbidden" (with this
>>>> provenance) pointer value, so all is well.
>>>>
>>>> (With the macro expansion the cast happens twice.  This is probably
>>>> better than using __virt_to_maddr here.)
>>>>
>>>> Ie, in this case I agree with Stefano.  The cast is both necessary and
>>>> sufficient.
>>>
>>> Maybe I missed something, but why are we using `<=` at all?  Why not use
>>> `<`?
>>>
>>> Or is this some weird C pointer comparison UB thing?
>>
>> _end may not be mapped in the virtual address space. This is the case
>> when the size of Xen is page-aligned. So _end will point to the next page.
>>
>> virt_to_maddr() cannot fail so it should only be called on valid virtual
>> address. The behavior is undefined in all the other cases.
>>
>> On x86, you might be able to get away because you translate the virtual
>> address to physical address in software.
>>
>> On Arm, we are using the hardware instruction to do the translation. As
>> _end is not always mapped, then the translation may fail. In other word,
>> Xen will crash.
> 
> None of this explains my question.
> 
> Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
> is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
> and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
> `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?
> 
> Under what conditions would they be different?

In case virt_to_maddr(_end) is undefined due to no translation being
available?


Juergen
Julien Grall Oct. 16, 2019, 10:43 a.m. | #12
Hi George,

On 16/10/2019 11:38, George Dunlap wrote:
> On 10/16/19 11:31 AM, Julien Grall wrote:
>> On 16/10/2019 11:22, George Dunlap wrote:
>>> On 10/16/19 11:18 AM, Ian Jackson wrote:
>>>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
>>>> _end in is_xen_fixed_mfn()"):
>>>>> 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 :-)
>>>>
>>>> FTAOD I think you are suggesting this:
>>>>    - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>>>    + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>>>
>>>> virt_to_maddr(va) is a macro which expands to
>>>>      __virt_to_maddr((vaddr_t)(va))
>>>>
>>>> So what is happening here is that the cast to an integer type is being
>>>> done before the subtraction.
>>>>
>>>> Without the cast, you are calculating the pointer value _end-1 from
>>>> the value _end, which is UB.  With the cast you are calculating an
>>>> integer value.  vaddr_t is unsigned, so all arithmetic operations are
>>>> defined.  Nothing casts the result back to the "forbidden" (with this
>>>> provenance) pointer value, so all is well.
>>>>
>>>> (With the macro expansion the cast happens twice.  This is probably
>>>> better than using __virt_to_maddr here.)
>>>>
>>>> Ie, in this case I agree with Stefano.  The cast is both necessary and
>>>> sufficient.
>>>
>>> Maybe I missed something, but why are we using `<=` at all?  Why not use
>>> `<`?
>>>
>>> Or is this some weird C pointer comparison UB thing?
>>
>> _end may not be mapped in the virtual address space. This is the case
>> when the size of Xen is page-aligned. So _end will point to the next page.
>>
>> virt_to_maddr() cannot fail so it should only be called on valid virtual
>> address. The behavior is undefined in all the other cases.
>>
>> On x86, you might be able to get away because you translate the virtual
>> address to physical address in software.
>>
>> On Arm, we are using the hardware instruction to do the translation. As
>> _end is not always mapped, then the translation may fail. In other word,
>> Xen will crash.
> 
> None of this explains my question.
> 
> Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
> is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
> and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
> `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?
> 
> Under what conditions would they be different?
> 
> And if they're the same, then you can just use `<` instead of `<=`, and
> not have to worry about casting before subtracting.

_end is not part of the binary but points one past it. So there is no guarantee 
this address will be mapped in the virtual address space.

As I pointed out in my previous e-mail the result of virt_to_maddr() will be 
undefined in this case. On Arm, this will actually crash Xen.

So you can't use '<' here.

Cheers,
George Dunlap Oct. 16, 2019, 10:44 a.m. | #13
On 10/16/19 11:41 AM, Jürgen Groß wrote:
> On 16.10.19 12:38, George Dunlap wrote:
>> On 10/16/19 11:31 AM, Julien Grall wrote:
>>> Hi George,
>>>
>>> On 16/10/2019 11:22, George Dunlap wrote:
>>>> On 10/16/19 11:18 AM, Ian Jackson wrote:
>>>>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
>>>>> _end in is_xen_fixed_mfn()"):
>>>>>> 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 :-)
>>>>>
>>>>> FTAOD I think you are suggesting this:
>>>>>    - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>>>>    + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>>>>
>>>>> virt_to_maddr(va) is a macro which expands to
>>>>>      __virt_to_maddr((vaddr_t)(va))
>>>>>
>>>>> So what is happening here is that the cast to an integer type is being
>>>>> done before the subtraction.
>>>>>
>>>>> Without the cast, you are calculating the pointer value _end-1 from
>>>>> the value _end, which is UB.  With the cast you are calculating an
>>>>> integer value.  vaddr_t is unsigned, so all arithmetic operations are
>>>>> defined.  Nothing casts the result back to the "forbidden" (with this
>>>>> provenance) pointer value, so all is well.
>>>>>
>>>>> (With the macro expansion the cast happens twice.  This is probably
>>>>> better than using __virt_to_maddr here.)
>>>>>
>>>>> Ie, in this case I agree with Stefano.  The cast is both necessary and
>>>>> sufficient.
>>>>
>>>> Maybe I missed something, but why are we using `<=` at all?  Why not
>>>> use
>>>> `<`?
>>>>
>>>> Or is this some weird C pointer comparison UB thing?
>>>
>>> _end may not be mapped in the virtual address space. This is the case
>>> when the size of Xen is page-aligned. So _end will point to the next
>>> page.
>>>
>>> virt_to_maddr() cannot fail so it should only be called on valid virtual
>>> address. The behavior is undefined in all the other cases.
>>>
>>> On x86, you might be able to get away because you translate the virtual
>>> address to physical address in software.
>>>
>>> On Arm, we are using the hardware instruction to do the translation. As
>>> _end is not always mapped, then the translation may fail. In other word,
>>> Xen will crash.
>>
>> None of this explains my question.
>>
>> Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
>> is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
>> and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
>> `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?
>>
>> Under what conditions would they be different?
> 
> In case virt_to_maddr(_end) is undefined due to no translation being
> available?

Ah, gotcha.  Sorry for the noise.

 -George
Julien Grall Oct. 16, 2019, 10:52 a.m. | #14
Hi,

On 16/10/2019 11:18, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()"):
>> 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 :-)
> 
> FTAOD I think you are suggesting this:
>   - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>   + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
> 
> virt_to_maddr(va) is a macro which expands to
>     __virt_to_maddr((vaddr_t)(va))
> 
> So what is happening here is that the cast to an integer type is being
> done before the subtraction.
> 
> Without the cast, you are calculating the pointer value _end-1 from
> the value _end, which is UB.  With the cast you are calculating an
> integer value.  vaddr_t is unsigned, so all arithmetic operations are
> defined.  Nothing casts the result back to the "forbidden" (with this
> provenance) pointer value, so all is well.
> 
> (With the macro expansion the cast happens twice.  This is probably
> better than using __virt_to_maddr here.)
> 
> Ie, in this case I agree with Stefano.  The cast is both necessary and
> sufficient.

Thank you both for the explanation. I will update the patch and resend it.

Cheers,
Stefano Stabellini Oct. 16, 2019, 3:22 p.m. | #15
On Wed, 16 Oct 2019, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()"):
> > 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 :-)
> 
> FTAOD I think you are suggesting this:
>  - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>  + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
> 
> virt_to_maddr(va) is a macro which expands to
>    __virt_to_maddr((vaddr_t)(va))
> 
> So what is happening here is that the cast to an integer type is being
> done before the subtraction.
> 
> Without the cast, you are calculating the pointer value _end-1 from
> the value _end, which is UB.  With the cast you are calculating an
> integer value.  vaddr_t is unsigned, so all arithmetic operations are
> defined.  Nothing casts the result back to the "forbidden" (with this
> provenance) pointer value, so all is well.
> 
> (With the macro expansion the cast happens twice.  This is probably
> better than using __virt_to_maddr here.)
> 
> Ie, in this case I agree with Stefano.  The cast is both necessary and
> sufficient.

Thanks Ian for the second pair of eyes!

Patch

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)))
 
 #define page_get_owner(_p)    (_p)->v.inuse.domain
 #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))