diff mbox series

memcpy: use bhs/bls instead of bge/blt (CVE-2020-6096) [BZ #25620]

Message ID d6117f7c4c194a3da1482c379b28fddc@huawei.com
State New
Headers show
Series memcpy: use bhs/bls instead of bge/blt (CVE-2020-6096) [BZ #25620] | expand

Commit Message

zhuyan (M) April 9, 2020, 2:05 p.m. UTC
An exploitable signed comparison vulnerability exists in the ARMv7
memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
targets that utilize the GNU glibc implementation) with a negative
value for the 'num' parameter results in a signed comparison
vulnerability.

If an attacker underflows the 'num' parameter to memcpy(), this
vulnerability could lead to undefined behavior such as writing to
out-of-bounds memory and potentially remote code execution.
Furthermore, this memcpy() implementation allows for program
execution to continue in scenarios where a segmentation fault or
crash should have occurred. The dangers occur in that subsequent
execution and iterations of this code will be executed with this
corrupted data.

Reference URL: https://sourceware.org/bugzilla/attachment.cgi?id=12334&action=edit

Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
---
sysdeps/arm/armv7/multiarch/memcpy_impl.S | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

       subs tmp2, count, #64     /* Use tmp2 for count.  */
-        blt    .Ltail63aligned
+       bls    .Ltail63aligned
        cmp tmp2, #512
-        bge  .Lcpy_body_long
+       bhs   .Lcpy_body_long
 .Lcpy_body_medium:                       /* Count in tmp2.  */
#ifdef USE_VFP
@@ -378,7 +378,7 @@ ENTRY(memcpy)
       add  src, src, #64
       vstr  d1, [dst, #56]
       add  dst, dst, #64
-        bge  1b
+       bhs   1b
       tst    tmp2, #0x3f
       beq  .Ldone
@@ -412,7 +412,7 @@ ENTRY(memcpy)
       ldrd  A_l, A_h, [src, #64]!
       strd  A_l, A_h, [dst, #64]!
       subs tmp2, tmp2, #64
-        bge  1b
+       bhs   1b
       tst    tmp2, #0x3f
       bne  1f
       ldr    tmp2,[sp], #FRAME_SIZE
@@ -482,7 +482,7 @@ ENTRY(memcpy)
       add  src, src, #32
        subs tmp2, tmp2, #prefetch_lines * 64 * 2
-        blt    2f
+       bls    2f
1:
       cpy_line_vfp     d3, 0
       cpy_line_vfp     d4, 64
@@ -494,7 +494,7 @@ ENTRY(memcpy)
       add  dst, dst, #2 * 64
       add  src, src, #2 * 64
       subs tmp2, tmp2, #prefetch_lines * 64
-        bge  1b
+       bhs   1b
 2:
       cpy_tail_vfp     d3, 0
--
2.12.3

Comments

Joseph Myers April 9, 2020, 4:25 p.m. UTC | #1
On Thu, 9 Apr 2020, zhuyan (M) wrote:

> An exploitable signed comparison vulnerability exists in the ARMv7
> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
> targets that utilize the GNU glibc implementation) with a negative
> value for the 'num' parameter results in a signed comparison
> vulnerability.
> 
> If an attacker underflows the 'num' parameter to memcpy(), this
> vulnerability could lead to undefined behavior such as writing to
> out-of-bounds memory and potentially remote code execution.
> Furthermore, this memcpy() implementation allows for program
> execution to continue in scenarios where a segmentation fault or
> crash should have occurred. The dangers occur in that subsequent
> execution and iterations of this code will be executed with this
> corrupted data.

I don't object to changing to use unsigned comparisons, since unsigned 
comparisons are what's logically appropriate here, but the commit message 
needs to discuss how it's very questionable whether this actually counts 
as a vulnerability, given that interfaces such as malloc cannot construct 
objects larger than PTRDIFF_MAX bytes and it's not clear what exactly is 
permitted to be done with a larger memory region allocated with mmap, so 
it's not clear if there are any cases where these functions can validly be 
called with a size argument exceeding PTRDIFF_MAX.

Any case where a size argument is passed that is larger than the allocated 
memory region cannot be a security vulnerability in glibc, only in the 
application passing the bogus size argument (though of course we sometimes 
do choose to improve hardening against buggy applications).

Furthermore, there was a comment in the bug saying a new test should be 
added to the testsuite.  So either the patch should add a test (that fails 
before and passes after the memcpy change is applied) or the commit 
message should justify why this is hard to test in the testsuite.
Robin Miyagi April 9, 2020, 4:28 p.m. UTC | #2
unsubscribe


On 9/4/20 9:25 am, Joseph Myers wrote:
> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>
>> An exploitable signed comparison vulnerability exists in the ARMv7
>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>> targets that utilize the GNU glibc implementation) with a negative
>> value for the 'num' parameter results in a signed comparison
>> vulnerability.
>>
>> If an attacker underflows the 'num' parameter to memcpy(), this
>> vulnerability could lead to undefined behavior such as writing to
>> out-of-bounds memory and potentially remote code execution.
>> Furthermore, this memcpy() implementation allows for program
>> execution to continue in scenarios where a segmentation fault or
>> crash should have occurred. The dangers occur in that subsequent
>> execution and iterations of this code will be executed with this
>> corrupted data.
> I don't object to changing to use unsigned comparisons, since unsigned
> comparisons are what's logically appropriate here, but the commit message
> needs to discuss how it's very questionable whether this actually counts
> as a vulnerability, given that interfaces such as malloc cannot construct
> objects larger than PTRDIFF_MAX bytes and it's not clear what exactly is
> permitted to be done with a larger memory region allocated with mmap, so
> it's not clear if there are any cases where these functions can validly be
> called with a size argument exceeding PTRDIFF_MAX.
>
> Any case where a size argument is passed that is larger than the allocated
> memory region cannot be a security vulnerability in glibc, only in the
> application passing the bogus size argument (though of course we sometimes
> do choose to improve hardening against buggy applications).
>
> Furthermore, there was a comment in the bug saying a new test should be
> added to the testsuite.  So either the patch should add a test (that fails
> before and passes after the memcpy change is applied) or the commit
> message should justify why this is hard to test in the testsuite.
>
Carlos O'Donell April 9, 2020, 6:27 p.m. UTC | #3
On 4/9/20 12:25 PM, Joseph Myers wrote:
> On Thu, 9 Apr 2020, zhuyan (M) wrote:
> 
>> An exploitable signed comparison vulnerability exists in the ARMv7
>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>> targets that utilize the GNU glibc implementation) with a negative
>> value for the 'num' parameter results in a signed comparison
>> vulnerability.
>>
>> If an attacker underflows the 'num' parameter to memcpy(), this
>> vulnerability could lead to undefined behavior such as writing to
>> out-of-bounds memory and potentially remote code execution.
>> Furthermore, this memcpy() implementation allows for program
>> execution to continue in scenarios where a segmentation fault or
>> crash should have occurred. The dangers occur in that subsequent
>> execution and iterations of this code will be executed with this
>> corrupted data.
> 
> I don't object to changing to use unsigned comparisons, since unsigned 
> comparisons are what's logically appropriate here, but the commit message 
> needs to discuss how it's very questionable whether this actually counts 
> as a vulnerability, given that interfaces such as malloc cannot construct 
> objects larger than PTRDIFF_MAX bytes and it's not clear what exactly is 
> permitted to be done with a larger memory region allocated with mmap, so 
> it's not clear if there are any cases where these functions can validly be 
> called with a size argument exceeding PTRDIFF_MAX.
> 
> Any case where a size argument is passed that is larger than the allocated 
> memory region cannot be a security vulnerability in glibc, only in the 
> application passing the bogus size argument (though of course we sometimes 
> do choose to improve hardening against buggy applications).
> 
> Furthermore, there was a comment in the bug saying a new test should be 
> added to the testsuite.  So either the patch should add a test (that fails 
> before and passes after the memcpy change is applied) or the commit 
> message should justify why this is hard to test in the testsuite.

This change should *absolutely* have a test case so we can see if there
are other similar problems with memcpy on other architectures. A synthetic
test case should be possible to construct.
Robin Miyagi April 9, 2020, 6:28 p.m. UTC | #4
unsubsribe

On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
> On 4/9/20 12:25 PM, Joseph Myers wrote:
>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>
>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>> targets that utilize the GNU glibc implementation) with a negative
>>> value for the 'num' parameter results in a signed comparison
>>> vulnerability.
>>>
>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>> vulnerability could lead to undefined behavior such as writing to
>>> out-of-bounds memory and potentially remote code execution.
>>> Furthermore, this memcpy() implementation allows for program
>>> execution to continue in scenarios where a segmentation fault or
>>> crash should have occurred. The dangers occur in that subsequent
>>> execution and iterations of this code will be executed with this
>>> corrupted data.
>> I don't object to changing to use unsigned comparisons, since unsigned
>> comparisons are what's logically appropriate here, but the commit message
>> needs to discuss how it's very questionable whether this actually counts
>> as a vulnerability, given that interfaces such as malloc cannot construct
>> objects larger than PTRDIFF_MAX bytes and it's not clear what exactly is
>> permitted to be done with a larger memory region allocated with mmap, so
>> it's not clear if there are any cases where these functions can validly be
>> called with a size argument exceeding PTRDIFF_MAX.
>>
>> Any case where a size argument is passed that is larger than the allocated
>> memory region cannot be a security vulnerability in glibc, only in the
>> application passing the bogus size argument (though of course we sometimes
>> do choose to improve hardening against buggy applications).
>>
>> Furthermore, there was a comment in the bug saying a new test should be
>> added to the testsuite.  So either the patch should add a test (that fails
>> before and passes after the memcpy change is applied) or the commit
>> message should justify why this is hard to test in the testsuite.
> This change should *absolutely* have a test case so we can see if there
> are other similar problems with memcpy on other architectures. A synthetic
> test case should be possible to construct.
>
Robin Miyagi April 9, 2020, 6:50 p.m. UTC | #5
unsubscribe

On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
> unsubsribe
>
> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>
>>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>> targets that utilize the GNU glibc implementation) with a negative
>>>> value for the 'num' parameter results in a signed comparison
>>>> vulnerability.
>>>>
>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>> vulnerability could lead to undefined behavior such as writing to
>>>> out-of-bounds memory and potentially remote code execution.
>>>> Furthermore, this memcpy() implementation allows for program
>>>> execution to continue in scenarios where a segmentation fault or
>>>> crash should have occurred. The dangers occur in that subsequent
>>>> execution and iterations of this code will be executed with this
>>>> corrupted data.
>>> I don't object to changing to use unsigned comparisons, since unsigned
>>> comparisons are what's logically appropriate here, but the commit 
>>> message
>>> needs to discuss how it's very questionable whether this actually 
>>> counts
>>> as a vulnerability, given that interfaces such as malloc cannot 
>>> construct
>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>> exactly is
>>> permitted to be done with a larger memory region allocated with 
>>> mmap, so
>>> it's not clear if there are any cases where these functions can 
>>> validly be
>>> called with a size argument exceeding PTRDIFF_MAX.
>>>
>>> Any case where a size argument is passed that is larger than the 
>>> allocated
>>> memory region cannot be a security vulnerability in glibc, only in the
>>> application passing the bogus size argument (though of course we 
>>> sometimes
>>> do choose to improve hardening against buggy applications).
>>>
>>> Furthermore, there was a comment in the bug saying a new test should be
>>> added to the testsuite.  So either the patch should add a test (that 
>>> fails
>>> before and passes after the memcpy change is applied) or the commit
>>> message should justify why this is hard to test in the testsuite.
>> This change should *absolutely* have a test case so we can see if there
>> are other similar problems with memcpy on other architectures. A 
>> synthetic
>> test case should be possible to construct.
>>
Robin Miyagi April 9, 2020, 6:50 p.m. UTC | #6
unsubscribe

On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
> unsubscribe
>
> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>> unsubsribe
>>
>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>
>>>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>> targets that utilize the GNU glibc implementation) with a negative
>>>>> value for the 'num' parameter results in a signed comparison
>>>>> vulnerability.
>>>>>
>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>> vulnerability could lead to undefined behavior such as writing to
>>>>> out-of-bounds memory and potentially remote code execution.
>>>>> Furthermore, this memcpy() implementation allows for program
>>>>> execution to continue in scenarios where a segmentation fault or
>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>> execution and iterations of this code will be executed with this
>>>>> corrupted data.
>>>> I don't object to changing to use unsigned comparisons, since unsigned
>>>> comparisons are what's logically appropriate here, but the commit 
>>>> message
>>>> needs to discuss how it's very questionable whether this actually 
>>>> counts
>>>> as a vulnerability, given that interfaces such as malloc cannot 
>>>> construct
>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>> exactly is
>>>> permitted to be done with a larger memory region allocated with 
>>>> mmap, so
>>>> it's not clear if there are any cases where these functions can 
>>>> validly be
>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>
>>>> Any case where a size argument is passed that is larger than the 
>>>> allocated
>>>> memory region cannot be a security vulnerability in glibc, only in the
>>>> application passing the bogus size argument (though of course we 
>>>> sometimes
>>>> do choose to improve hardening against buggy applications).
>>>>
>>>> Furthermore, there was a comment in the bug saying a new test 
>>>> should be
>>>> added to the testsuite.  So either the patch should add a test 
>>>> (that fails
>>>> before and passes after the memcpy change is applied) or the commit
>>>> message should justify why this is hard to test in the testsuite.
>>> This change should *absolutely* have a test case so we can see if there
>>> are other similar problems with memcpy on other architectures. A 
>>> synthetic
>>> test case should be possible to construct.
>>>
Robin Miyagi April 9, 2020, 6:50 p.m. UTC | #7
unsubscribe

On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
> unsubscribe
>
> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>> unsubscribe
>>
>> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>>> unsubsribe
>>>
>>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>>
>>>>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>>> targets that utilize the GNU glibc implementation) with a negative
>>>>>> value for the 'num' parameter results in a signed comparison
>>>>>> vulnerability.
>>>>>>
>>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>>> vulnerability could lead to undefined behavior such as writing to
>>>>>> out-of-bounds memory and potentially remote code execution.
>>>>>> Furthermore, this memcpy() implementation allows for program
>>>>>> execution to continue in scenarios where a segmentation fault or
>>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>>> execution and iterations of this code will be executed with this
>>>>>> corrupted data.
>>>>> I don't object to changing to use unsigned comparisons, since 
>>>>> unsigned
>>>>> comparisons are what's logically appropriate here, but the commit 
>>>>> message
>>>>> needs to discuss how it's very questionable whether this actually 
>>>>> counts
>>>>> as a vulnerability, given that interfaces such as malloc cannot 
>>>>> construct
>>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>>> exactly is
>>>>> permitted to be done with a larger memory region allocated with 
>>>>> mmap, so
>>>>> it's not clear if there are any cases where these functions can 
>>>>> validly be
>>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>>
>>>>> Any case where a size argument is passed that is larger than the 
>>>>> allocated
>>>>> memory region cannot be a security vulnerability in glibc, only in 
>>>>> the
>>>>> application passing the bogus size argument (though of course we 
>>>>> sometimes
>>>>> do choose to improve hardening against buggy applications).
>>>>>
>>>>> Furthermore, there was a comment in the bug saying a new test 
>>>>> should be
>>>>> added to the testsuite.  So either the patch should add a test 
>>>>> (that fails
>>>>> before and passes after the memcpy change is applied) or the commit
>>>>> message should justify why this is hard to test in the testsuite.
>>>> This change should *absolutely* have a test case so we can see if 
>>>> there
>>>> are other similar problems with memcpy on other architectures. A 
>>>> synthetic
>>>> test case should be possible to construct.
>>>>
Robin Miyagi April 9, 2020, 6:51 p.m. UTC | #8
unsubsribe

On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
> unsubscribe
>
> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>> unsubscribe
>>
>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>> unsubscribe
>>>
>>> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>>>> unsubsribe
>>>>
>>>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>>>
>>>>>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>>>> targets that utilize the GNU glibc implementation) with a negative
>>>>>>> value for the 'num' parameter results in a signed comparison
>>>>>>> vulnerability.
>>>>>>>
>>>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>>>> vulnerability could lead to undefined behavior such as writing to
>>>>>>> out-of-bounds memory and potentially remote code execution.
>>>>>>> Furthermore, this memcpy() implementation allows for program
>>>>>>> execution to continue in scenarios where a segmentation fault or
>>>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>>>> execution and iterations of this code will be executed with this
>>>>>>> corrupted data.
>>>>>> I don't object to changing to use unsigned comparisons, since 
>>>>>> unsigned
>>>>>> comparisons are what's logically appropriate here, but the commit 
>>>>>> message
>>>>>> needs to discuss how it's very questionable whether this actually 
>>>>>> counts
>>>>>> as a vulnerability, given that interfaces such as malloc cannot 
>>>>>> construct
>>>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>>>> exactly is
>>>>>> permitted to be done with a larger memory region allocated with 
>>>>>> mmap, so
>>>>>> it's not clear if there are any cases where these functions can 
>>>>>> validly be
>>>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>>>
>>>>>> Any case where a size argument is passed that is larger than the 
>>>>>> allocated
>>>>>> memory region cannot be a security vulnerability in glibc, only 
>>>>>> in the
>>>>>> application passing the bogus size argument (though of course we 
>>>>>> sometimes
>>>>>> do choose to improve hardening against buggy applications).
>>>>>>
>>>>>> Furthermore, there was a comment in the bug saying a new test 
>>>>>> should be
>>>>>> added to the testsuite.  So either the patch should add a test 
>>>>>> (that fails
>>>>>> before and passes after the memcpy change is applied) or the commit
>>>>>> message should justify why this is hard to test in the testsuite.
>>>>> This change should *absolutely* have a test case so we can see if 
>>>>> there
>>>>> are other similar problems with memcpy on other architectures. A 
>>>>> synthetic
>>>>> test case should be possible to construct.
>>>>>
Robin Miyagi April 9, 2020, 6:51 p.m. UTC | #9
unsubsribe

On 9/4/20 11:51 am, Robin Miyagi via Libc-alpha wrote:
> unsubsribe
>
> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>> unsubscribe
>>
>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>> unsubscribe
>>>
>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>> unsubscribe
>>>>
>>>> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>>>>> unsubsribe
>>>>>
>>>>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>>>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>>>>
>>>>>>>> An exploitable signed comparison vulnerability exists in the ARMv7
>>>>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>>>>> targets that utilize the GNU glibc implementation) with a negative
>>>>>>>> value for the 'num' parameter results in a signed comparison
>>>>>>>> vulnerability.
>>>>>>>>
>>>>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>>>>> vulnerability could lead to undefined behavior such as writing to
>>>>>>>> out-of-bounds memory and potentially remote code execution.
>>>>>>>> Furthermore, this memcpy() implementation allows for program
>>>>>>>> execution to continue in scenarios where a segmentation fault or
>>>>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>>>>> execution and iterations of this code will be executed with this
>>>>>>>> corrupted data.
>>>>>>> I don't object to changing to use unsigned comparisons, since 
>>>>>>> unsigned
>>>>>>> comparisons are what's logically appropriate here, but the 
>>>>>>> commit message
>>>>>>> needs to discuss how it's very questionable whether this 
>>>>>>> actually counts
>>>>>>> as a vulnerability, given that interfaces such as malloc cannot 
>>>>>>> construct
>>>>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>>>>> exactly is
>>>>>>> permitted to be done with a larger memory region allocated with 
>>>>>>> mmap, so
>>>>>>> it's not clear if there are any cases where these functions can 
>>>>>>> validly be
>>>>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>>>>
>>>>>>> Any case where a size argument is passed that is larger than the 
>>>>>>> allocated
>>>>>>> memory region cannot be a security vulnerability in glibc, only 
>>>>>>> in the
>>>>>>> application passing the bogus size argument (though of course we 
>>>>>>> sometimes
>>>>>>> do choose to improve hardening against buggy applications).
>>>>>>>
>>>>>>> Furthermore, there was a comment in the bug saying a new test 
>>>>>>> should be
>>>>>>> added to the testsuite.  So either the patch should add a test 
>>>>>>> (that fails
>>>>>>> before and passes after the memcpy change is applied) or the commit
>>>>>>> message should justify why this is hard to test in the testsuite.
>>>>>> This change should *absolutely* have a test case so we can see if 
>>>>>> there
>>>>>> are other similar problems with memcpy on other architectures. A 
>>>>>> synthetic
>>>>>> test case should be possible to construct.
>>>>>>
Robin Miyagi April 9, 2020, 6:52 p.m. UTC | #10
unsubsribe

On 9/4/20 11:51 am, Robin Miyagi via Libc-alpha wrote:
> unsubsribe
>
> On 9/4/20 11:51 am, Robin Miyagi via Libc-alpha wrote:
>> unsubsribe
>>
>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>> unsubscribe
>>>
>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>> unsubscribe
>>>>
>>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>>> unsubscribe
>>>>>
>>>>> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>>>>>> unsubsribe
>>>>>>
>>>>>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>>>>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>>>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>>>>>
>>>>>>>>> An exploitable signed comparison vulnerability exists in the 
>>>>>>>>> ARMv7
>>>>>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>>>>>> targets that utilize the GNU glibc implementation) with a 
>>>>>>>>> negative
>>>>>>>>> value for the 'num' parameter results in a signed comparison
>>>>>>>>> vulnerability.
>>>>>>>>>
>>>>>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>>>>>> vulnerability could lead to undefined behavior such as writing to
>>>>>>>>> out-of-bounds memory and potentially remote code execution.
>>>>>>>>> Furthermore, this memcpy() implementation allows for program
>>>>>>>>> execution to continue in scenarios where a segmentation fault or
>>>>>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>>>>>> execution and iterations of this code will be executed with this
>>>>>>>>> corrupted data.
>>>>>>>> I don't object to changing to use unsigned comparisons, since 
>>>>>>>> unsigned
>>>>>>>> comparisons are what's logically appropriate here, but the 
>>>>>>>> commit message
>>>>>>>> needs to discuss how it's very questionable whether this 
>>>>>>>> actually counts
>>>>>>>> as a vulnerability, given that interfaces such as malloc cannot 
>>>>>>>> construct
>>>>>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>>>>>> exactly is
>>>>>>>> permitted to be done with a larger memory region allocated with 
>>>>>>>> mmap, so
>>>>>>>> it's not clear if there are any cases where these functions can 
>>>>>>>> validly be
>>>>>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>>>>>
>>>>>>>> Any case where a size argument is passed that is larger than 
>>>>>>>> the allocated
>>>>>>>> memory region cannot be a security vulnerability in glibc, only 
>>>>>>>> in the
>>>>>>>> application passing the bogus size argument (though of course 
>>>>>>>> we sometimes
>>>>>>>> do choose to improve hardening against buggy applications).
>>>>>>>>
>>>>>>>> Furthermore, there was a comment in the bug saying a new test 
>>>>>>>> should be
>>>>>>>> added to the testsuite.  So either the patch should add a test 
>>>>>>>> (that fails
>>>>>>>> before and passes after the memcpy change is applied) or the 
>>>>>>>> commit
>>>>>>>> message should justify why this is hard to test in the testsuite.
>>>>>>> This change should *absolutely* have a test case so we can see 
>>>>>>> if there
>>>>>>> are other similar problems with memcpy on other architectures. A 
>>>>>>> synthetic
>>>>>>> test case should be possible to construct.
>>>>>>>
Robin Miyagi April 9, 2020, 6:52 p.m. UTC | #11
unsubsribe

On 9/4/20 11:52 am, Robin Miyagi via Libc-alpha wrote:
> unsubsribe
>
> On 9/4/20 11:51 am, Robin Miyagi via Libc-alpha wrote:
>> unsubsribe
>>
>> On 9/4/20 11:51 am, Robin Miyagi via Libc-alpha wrote:
>>> unsubsribe
>>>
>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>> unsubscribe
>>>>
>>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>>> unsubscribe
>>>>>
>>>>> On 9/4/20 11:50 am, Robin Miyagi via Libc-alpha wrote:
>>>>>> unsubscribe
>>>>>>
>>>>>> On 9/4/20 11:28 am, Robin Miyagi via Libc-alpha wrote:
>>>>>>> unsubsribe
>>>>>>>
>>>>>>> On 9/4/20 11:27 am, Carlos O'Donell via Libc-alpha wrote:
>>>>>>>> On 4/9/20 12:25 PM, Joseph Myers wrote:
>>>>>>>>> On Thu, 9 Apr 2020, zhuyan (M) wrote:
>>>>>>>>>
>>>>>>>>>> An exploitable signed comparison vulnerability exists in the 
>>>>>>>>>> ARMv7
>>>>>>>>>> memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7
>>>>>>>>>> targets that utilize the GNU glibc implementation) with a 
>>>>>>>>>> negative
>>>>>>>>>> value for the 'num' parameter results in a signed comparison
>>>>>>>>>> vulnerability.
>>>>>>>>>>
>>>>>>>>>> If an attacker underflows the 'num' parameter to memcpy(), this
>>>>>>>>>> vulnerability could lead to undefined behavior such as 
>>>>>>>>>> writing to
>>>>>>>>>> out-of-bounds memory and potentially remote code execution.
>>>>>>>>>> Furthermore, this memcpy() implementation allows for program
>>>>>>>>>> execution to continue in scenarios where a segmentation fault or
>>>>>>>>>> crash should have occurred. The dangers occur in that subsequent
>>>>>>>>>> execution and iterations of this code will be executed with this
>>>>>>>>>> corrupted data.
>>>>>>>>> I don't object to changing to use unsigned comparisons, since 
>>>>>>>>> unsigned
>>>>>>>>> comparisons are what's logically appropriate here, but the 
>>>>>>>>> commit message
>>>>>>>>> needs to discuss how it's very questionable whether this 
>>>>>>>>> actually counts
>>>>>>>>> as a vulnerability, given that interfaces such as malloc 
>>>>>>>>> cannot construct
>>>>>>>>> objects larger than PTRDIFF_MAX bytes and it's not clear what 
>>>>>>>>> exactly is
>>>>>>>>> permitted to be done with a larger memory region allocated 
>>>>>>>>> with mmap, so
>>>>>>>>> it's not clear if there are any cases where these functions 
>>>>>>>>> can validly be
>>>>>>>>> called with a size argument exceeding PTRDIFF_MAX.
>>>>>>>>>
>>>>>>>>> Any case where a size argument is passed that is larger than 
>>>>>>>>> the allocated
>>>>>>>>> memory region cannot be a security vulnerability in glibc, 
>>>>>>>>> only in the
>>>>>>>>> application passing the bogus size argument (though of course 
>>>>>>>>> we sometimes
>>>>>>>>> do choose to improve hardening against buggy applications).
>>>>>>>>>
>>>>>>>>> Furthermore, there was a comment in the bug saying a new test 
>>>>>>>>> should be
>>>>>>>>> added to the testsuite.  So either the patch should add a test 
>>>>>>>>> (that fails
>>>>>>>>> before and passes after the memcpy change is applied) or the 
>>>>>>>>> commit
>>>>>>>>> message should justify why this is hard to test in the testsuite.
>>>>>>>> This change should *absolutely* have a test case so we can see 
>>>>>>>> if there
>>>>>>>> are other similar problems with memcpy on other architectures. 
>>>>>>>> A synthetic
>>>>>>>> test case should be possible to construct.
>>>>>>>>
Carlos O'Donell April 9, 2020, 7:05 p.m. UTC | #12
On 4/9/20 2:52 PM, Robin Miyagi via Libc-alpha wrote:
> unsubsribe

I have manually handled your request.

The mailing list details including steps to unsubscribe are listed here:
https://sourceware.org/mailman/listinfo/libc-alpha
diff mbox series

Patch

diff --git a/sysdeps/arm/armv7/multiarch/memcpy_impl.S b/sysdeps/arm/armv7/multiarch/memcpy_impl.S
index bf4ac7077f..7455bdc6c7 100644
--- a/sysdeps/arm/armv7/multiarch/memcpy_impl.S
+++ b/sysdeps/arm/armv7/multiarch/memcpy_impl.S
@@ -268,7 +268,7 @@  ENTRY(memcpy)
        mov dst, dstin /* Preserve dstin, we need to return it.  */
       cmp count, #64
-        bge  .Lcpy_not_short
+       bhs   .Lcpy_not_short
       /* Deal with small copies quickly by dropping straight into the
          exit block.  */
@@ -351,10 +351,10 @@  ENTRY(memcpy)
 1: