| 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() | expand |
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) ?
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
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
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
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 :-)
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
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.
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
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,
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
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
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,
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
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,
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!
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))
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(-)