[PATCH for-4.10] string: fix memmove when size is 0

[PATCH for-4.10] string: fix memmove when size is 0

[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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Jan Beulich]

>>> 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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Andrew Cooper]

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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Roger Pau Monné]

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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Jan Beulich]

>>> 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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Roger Pau Monné]

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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Jan Beulich]

>>> 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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Julien Grall]

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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Jan Beulich]

>>> 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

Re: [PATCH for-4.10] string: fix memmove when size is 0

[Roger Pau Monné]

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