* [PATCH for-4.10] string: fix memmove when size is 0
@ 2017-10-17 12:03 Roger Pau Monne
2017-10-17 12:26 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Roger Pau Monne @ 2017-10-17 12:03 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monne
ubsan in clang 5.0 complains with:
(XEN) UBSAN: Undefined behaviour in string.c:50:28
(XEN) pointer overflow:
(XEN) addition of unsigned offset to ffff830000100000 overflowed to ffff8300000fffff
[...]
(XEN) Xen call trace:
(XEN) [<ffff82d0802dce0d>] ubsan.c#ubsan_epilogue+0xd/0xc0
(XEN) [<ffff82d0802de145>] __ubsan_handle_pointer_overflow+0xa5/0xe0
(XEN) [<ffff82d0803bf627>] memmove+0x67/0x80
(XEN) [<ffff82d080679f87>] page_alloc.c#bootmem_region_add+0x157/0x220
(XEN) [<ffff82d080679c66>] init_boot_pages+0x56/0x220
(XEN) [<ffff82d0806bcb9d>] __start_xen+0x2c2d/0x4b50
(XEN) [<ffff82d0802000f3>] __high_start+0x53/0x60
This is due to memmove doing a n-1+addr when n is 0. This is harmless
because the loop is bounded to 0. Fix this by returning earlier when n
is 0.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
---
This is a harmless fix that shouldn't introduce any functional change,
and hence it should be committed to 4.10 IMHO.
---
xen/arch/x86/string.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
index cd85a38e6d..19dcfe88ed 100644
--- a/xen/arch/x86/string.c
+++ b/xen/arch/x86/string.c
@@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n)
{
long d0, d1, d2;
+ if ( !n )
+ return;
+
if ( dest < src )
return memcpy(dest, src, n);
--
2.13.5 (Apple Git-94)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 12:03 [PATCH for-4.10] string: fix memmove when size is 0 Roger Pau Monne
@ 2017-10-17 12:26 ` Jan Beulich
2017-10-17 12:41 ` Andrew Cooper
2017-10-20 7:17 ` Jan Beulich
2 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2017-10-17 12:26 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, Julien Grall, xen-devel
>>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n)
> {
> long d0, d1, d2;
>
> + if ( !n )
> + return;
memmove() hopefully isn't on any really hot path, so the extra
conditional shouldn't hurt much. Personally I think in cases like
this, where the compiler would need to step out of its way in
order to cause actually unexpected behavior, it is rather
pointless to try to please a checking tool.
Anyway,
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 12:03 [PATCH for-4.10] string: fix memmove when size is 0 Roger Pau Monne
2017-10-17 12:26 ` Jan Beulich
@ 2017-10-17 12:41 ` Andrew Cooper
2017-10-17 12:52 ` Roger Pau Monné
2017-10-20 7:17 ` Jan Beulich
2 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2017-10-17 12:41 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Julien Grall, Jan Beulich
On 17/10/17 13:03, Roger Pau Monne wrote:
> ubsan in clang 5.0 complains with:
>
> (XEN) UBSAN: Undefined behaviour in string.c:50:28
> (XEN) pointer overflow:
> (XEN) addition of unsigned offset to ffff830000100000 overflowed to ffff8300000fffff
> [...]
> (XEN) Xen call trace:
> (XEN) [<ffff82d0802dce0d>] ubsan.c#ubsan_epilogue+0xd/0xc0
> (XEN) [<ffff82d0802de145>] __ubsan_handle_pointer_overflow+0xa5/0xe0
> (XEN) [<ffff82d0803bf627>] memmove+0x67/0x80
> (XEN) [<ffff82d080679f87>] page_alloc.c#bootmem_region_add+0x157/0x220
> (XEN) [<ffff82d080679c66>] init_boot_pages+0x56/0x220
> (XEN) [<ffff82d0806bcb9d>] __start_xen+0x2c2d/0x4b50
> (XEN) [<ffff82d0802000f3>] __high_start+0x53/0x60
>
> This is due to memmove doing a n-1+addr when n is 0. This is harmless
> because the loop is bounded to 0. Fix this by returning earlier when n
> is 0.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
There are many passed values which could trigger this warning. Does
diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
index cd85a38..4f55856 100644
--- a/xen/arch/x86/string.c
+++ b/xen/arch/x86/string.c
@@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
" rep movsb ; "
" cld "
: "=&c" (d0), "=&S" (d1), "=&D" (d2)
- : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
+ : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
: "memory");
return dest;
work any better?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 12:41 ` Andrew Cooper
@ 2017-10-17 12:52 ` Roger Pau Monné
2017-10-17 13:00 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Roger Pau Monné @ 2017-10-17 12:52 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall, Jan Beulich
On Tue, Oct 17, 2017 at 01:41:35PM +0100, Andrew Cooper wrote:
> On 17/10/17 13:03, Roger Pau Monne wrote:
> > ubsan in clang 5.0 complains with:
> >
> > (XEN) UBSAN: Undefined behaviour in string.c:50:28
> > (XEN) pointer overflow:
> > (XEN) addition of unsigned offset to ffff830000100000 overflowed to ffff8300000fffff
> > [...]
> > (XEN) Xen call trace:
> > (XEN) [<ffff82d0802dce0d>] ubsan.c#ubsan_epilogue+0xd/0xc0
> > (XEN) [<ffff82d0802de145>] __ubsan_handle_pointer_overflow+0xa5/0xe0
> > (XEN) [<ffff82d0803bf627>] memmove+0x67/0x80
> > (XEN) [<ffff82d080679f87>] page_alloc.c#bootmem_region_add+0x157/0x220
> > (XEN) [<ffff82d080679c66>] init_boot_pages+0x56/0x220
> > (XEN) [<ffff82d0806bcb9d>] __start_xen+0x2c2d/0x4b50
> > (XEN) [<ffff82d0802000f3>] __high_start+0x53/0x60
> >
> > This is due to memmove doing a n-1+addr when n is 0. This is harmless
> > because the loop is bounded to 0. Fix this by returning earlier when n
> > is 0.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> There are many passed values which could trigger this warning. Does
>
> diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
> index cd85a38..4f55856 100644
> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
> " rep movsb ; "
> " cld "
> : "=&c" (d0), "=&S" (d1), "=&D" (d2)
> - : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
> + : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
> : "memory");
>
> return dest;
>
> work any better?
That does indeed work, but I'm not sure if it would mask legitimate
pointer overflows by casting them into integers. In any case, as said
on IRC the only problematic case ATM is when n == 0.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 12:52 ` Roger Pau Monné
@ 2017-10-17 13:00 ` Jan Beulich
2017-10-18 10:44 ` Roger Pau Monné
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2017-10-17 13:00 UTC (permalink / raw)
To: Andrew Cooper, Roger Pau Monné; +Cc: xen-devel, JulienGrall
>>> On 17.10.17 at 14:52, <roger.pau@citrix.com> wrote:
> On Tue, Oct 17, 2017 at 01:41:35PM +0100, Andrew Cooper wrote:
>> There are many passed values which could trigger this warning. Does
>>
>> diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
>> index cd85a38..4f55856 100644
>> --- a/xen/arch/x86/string.c
>> +++ b/xen/arch/x86/string.c
>> @@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
>> " rep movsb ; "
>> " cld "
>> : "=&c" (d0), "=&S" (d1), "=&D" (d2)
>> - : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
>> + : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
>> : "memory");
>>
>> return dest;
>>
>> work any better?
>
> That does indeed work, but I'm not sure if it would mask legitimate
> pointer overflows by casting them into integers.
It certainly would, as the tool can't possibly know that the asm()
itself then effectively casts the integers back to pointers (i.e. it
has no basis to try to "look through" the cast and continue analysis).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 13:00 ` Jan Beulich
@ 2017-10-18 10:44 ` Roger Pau Monné
2017-10-18 13:17 ` Jan Beulich
2017-10-18 13:18 ` Julien Grall
0 siblings, 2 replies; 10+ messages in thread
From: Roger Pau Monné @ 2017-10-18 10:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, JulienGrall, xen-devel
On Tue, Oct 17, 2017 at 07:00:25AM -0600, Jan Beulich wrote:
> >>> On 17.10.17 at 14:52, <roger.pau@citrix.com> wrote:
> > On Tue, Oct 17, 2017 at 01:41:35PM +0100, Andrew Cooper wrote:
> >> There are many passed values which could trigger this warning. Does
> >>
> >> diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
> >> index cd85a38..4f55856 100644
> >> --- a/xen/arch/x86/string.c
> >> +++ b/xen/arch/x86/string.c
> >> @@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
> >> " rep movsb ; "
> >> " cld "
> >> : "=&c" (d0), "=&S" (d1), "=&D" (d2)
> >> - : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
> >> + : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
> >> : "memory");
> >>
> >> return dest;
> >>
> >> work any better?
> >
> > That does indeed work, but I'm not sure if it would mask legitimate
> > pointer overflows by casting them into integers.
>
> It certainly would, as the tool can't possibly know that the asm()
> itself then effectively casts the integers back to pointers (i.e. it
> has no basis to try to "look through" the cast and continue analysis).
I assume there are no further steps for me, just wait for Julien's
release Ack.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-18 10:44 ` Roger Pau Monné
@ 2017-10-18 13:17 ` Jan Beulich
2017-10-18 13:18 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2017-10-18 13:17 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, JulienGrall, xen-devel
>>> On 18.10.17 at 12:44, <roger.pau@citrix.com> wrote:
> On Tue, Oct 17, 2017 at 07:00:25AM -0600, Jan Beulich wrote:
>> >>> On 17.10.17 at 14:52, <roger.pau@citrix.com> wrote:
>> > On Tue, Oct 17, 2017 at 01:41:35PM +0100, Andrew Cooper wrote:
>> >> There are many passed values which could trigger this warning. Does
>> >>
>> >> diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
>> >> index cd85a38..4f55856 100644
>> >> --- a/xen/arch/x86/string.c
>> >> +++ b/xen/arch/x86/string.c
>> >> @@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
>> >> " rep movsb ; "
>> >> " cld "
>> >> : "=&c" (d0), "=&S" (d1), "=&D" (d2)
>> >> - : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
>> >> + : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
>> >> : "memory");
>> >>
>> >> return dest;
>> >>
>> >> work any better?
>> >
>> > That does indeed work, but I'm not sure if it would mask legitimate
>> > pointer overflows by casting them into integers.
>>
>> It certainly would, as the tool can't possibly know that the asm()
>> itself then effectively casts the integers back to pointers (i.e. it
>> has no basis to try to "look through" the cast and continue analysis).
>
> I assume there are no further steps for me, just wait for Julien's
> release Ack.
Plus, considering his earlier comments, preferably a "not-a-nack"
clarification by Andrew.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-18 10:44 ` Roger Pau Monné
2017-10-18 13:17 ` Jan Beulich
@ 2017-10-18 13:18 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Julien Grall @ 2017-10-18 13:18 UTC (permalink / raw)
To: Roger Pau Monné, Jan Beulich; +Cc: Andrew Cooper, JulienGrall, xen-devel
Hi Roger,
On 10/18/2017 11:44 AM, Roger Pau Monné wrote:
> On Tue, Oct 17, 2017 at 07:00:25AM -0600, Jan Beulich wrote:
>>>>> On 17.10.17 at 14:52, <roger.pau@citrix.com> wrote:
>>> On Tue, Oct 17, 2017 at 01:41:35PM +0100, Andrew Cooper wrote:
>>>> There are many passed values which could trigger this warning. Does
>>>>
>>>> diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
>>>> index cd85a38..4f55856 100644
>>>> --- a/xen/arch/x86/string.c
>>>> +++ b/xen/arch/x86/string.c
>>>> @@ -47,7 +47,7 @@ void *(memmove)(void *dest, const void *src, size_t n)
>>>> " rep movsb ; "
>>>> " cld "
>>>> : "=&c" (d0), "=&S" (d1), "=&D" (d2)
>>>> - : "0" (n), "1" (n-1+(const char *)src), "2" (n-1+(char *)dest)
>>>> + : "0" (n), "1" ((uintptr_t)src + n - 1), "2" ((uintptr_t)dest + n - 1)
>>>> : "memory");
>>>>
>>>> return dest;
>>>>
>>>> work any better?
>>>
>>> That does indeed work, but I'm not sure if it would mask legitimate
>>> pointer overflows by casting them into integers.
>>
>> It certainly would, as the tool can't possibly know that the asm()
>> itself then effectively casts the integers back to pointers (i.e. it
>> has no basis to try to "look through" the cast and continue analysis).
>
> I assume there are no further steps for me, just wait for Julien's
> release Ack.
Release-acked-by: Julien Grall <julien.grall@linaro.org>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-17 12:03 [PATCH for-4.10] string: fix memmove when size is 0 Roger Pau Monne
2017-10-17 12:26 ` Jan Beulich
2017-10-17 12:41 ` Andrew Cooper
@ 2017-10-20 7:17 ` Jan Beulich
2017-10-20 10:13 ` Roger Pau Monné
2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2017-10-20 7:17 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, Julien Grall, xen-devel
>>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n)
> {
> long d0, d1, d2;
>
> + if ( !n )
> + return;
Actually - I can't see how this would build successfully: The function
returns void *, not void. I'm taking the liberty to fix this (and also
add unlikely()) while committing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH for-4.10] string: fix memmove when size is 0
2017-10-20 7:17 ` Jan Beulich
@ 2017-10-20 10:13 ` Roger Pau Monné
0 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monné @ 2017-10-20 10:13 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Julien Grall, xen-devel
On Fri, Oct 20, 2017 at 01:17:40AM -0600, Jan Beulich wrote:
> >>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/string.c
> > +++ b/xen/arch/x86/string.c
> > @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n)
> > {
> > long d0, d1, d2;
> >
> > + if ( !n )
> > + return;
>
> Actually - I can't see how this would build successfully: The function
> returns void *, not void. I'm taking the liberty to fix this (and also
> add unlikely()) while committing.
Thanks and sorry. I tested this with clang 5.0 + ubsan enabled, but I
have no idea why/how that compiled.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-10-20 10:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-17 12:03 [PATCH for-4.10] string: fix memmove when size is 0 Roger Pau Monne
2017-10-17 12:26 ` Jan Beulich
2017-10-17 12:41 ` Andrew Cooper
2017-10-17 12:52 ` Roger Pau Monné
2017-10-17 13:00 ` Jan Beulich
2017-10-18 10:44 ` Roger Pau Monné
2017-10-18 13:17 ` Jan Beulich
2017-10-18 13:18 ` Julien Grall
2017-10-20 7:17 ` Jan Beulich
2017-10-20 10:13 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.