* [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
@ 2020-06-03 17:55 Evgeny Eremin
2020-06-04 9:16 ` Florian Weimer
0 siblings, 1 reply; 10+ messages in thread
From: Evgeny Eremin @ 2020-06-03 17:55 UTC (permalink / raw)
To: libc-alpha
Hi,
Unsigned branch instructions could be used for r2 to fix the wrong
behavior when a negative length is passed to memcpy and memmove
(sysdeps/arm).
An In-house testing hasn't reveal any functional regressions.
Performance measurement & comparison are yet be done but the patch
doesn't change the logic too much.
This partially fixes CVE-2020-6096 [1] "GNU glibc ARMv7 memcpy() memory
corruption vulnerability".
Signed-off-by: Konstantin Karasev <k.karasev@omprussia.ru>
Signed-off-by: Anton Rybakov <a.rybakov@omprussia.ru>
Signed-off-by: Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>
Signed-off-by: Alexander Anisimov <a.anisimov@omprussia.ru>
[1] https://nvd.nist.gov/vuln/detail/CVE-2020-6096
---
sysdeps/arm/memcpy.S | 24 ++++++++++--------------
sysdeps/arm/memmove.S | 24 ++++++++++--------------
2 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/sysdeps/arm/memcpy.S b/sysdeps/arm/memcpy.S
index 510e8adaf2..bcfbc51d99 100644
--- a/sysdeps/arm/memcpy.S
+++ b/sysdeps/arm/memcpy.S
@@ -68,7 +68,7 @@ ENTRY(memcpy)
cfi_remember_state
subs r2, r2, #4
- blt 8f
+ blo 8f
ands ip, r0, #3
PLD( pld [r1, #0] )
bne 9f
@@ -82,7 +82,7 @@ ENTRY(memcpy)
cfi_rel_offset (r6, 4)
cfi_rel_offset (r7, 8)
cfi_rel_offset (r8, 12)
- blt 5f
+ blo 5f
CALGN( ands ip, r1, #31 )
CALGN( rsb r3, ip, #32 )
@@ -98,9 +98,9 @@ ENTRY(memcpy)
#endif
PLD( pld [r1, #0] )
-2: PLD( subs r2, r2, #96 )
+2: PLD( cmp r2, #96 )
PLD( pld [r1, #28] )
- PLD( blt 4f )
+ PLD( blo 4f )
PLD( pld [r1, #60] )
PLD( pld [r1, #92] )
@@ -108,9 +108,7 @@ ENTRY(memcpy)
4: ldmia r1!, {r3, r4, r5, r6, r7, r8, ip, lr}
subs r2, r2, #32
stmia r0!, {r3, r4, r5, r6, r7, r8, ip, lr}
- bge 3b
- PLD( cmn r2, #96 )
- PLD( bge 4b )
+ bhs 3b
5: ands ip, r2, #28
rsb ip, ip, #32
@@ -222,7 +220,7 @@ ENTRY(memcpy)
strbge r4, [r0], #1
subs r2, r2, ip
strb lr, [r0], #1
- blt 8b
+ blo 8b
ands ip, r1, #3
beq 1b
@@ -236,7 +234,7 @@ ENTRY(memcpy)
.macro forward_copy_shift pull push
subs r2, r2, #28
- blt 14f
+ blo 14f
CALGN( ands ip, r1, #31 )
CALGN( rsb ip, ip, #32 )
@@ -253,9 +251,9 @@ ENTRY(memcpy)
cfi_rel_offset (r10, 16)
PLD( pld [r1, #0] )
- PLD( subs r2, r2, #96 )
+ PLD( cmp r2, #96 )
PLD( pld [r1, #28] )
- PLD( blt 13f )
+ PLD( blo 13f )
PLD( pld [r1, #60] )
PLD( pld [r1, #92] )
@@ -280,9 +278,7 @@ ENTRY(memcpy)
mov ip, ip, PULL #\pull
orr ip, ip, lr, PUSH #\push
stmia r0!, {r3, r4, r5, r6, r7, r8, r10, ip}
- bge 12b
- PLD( cmn r2, #96 )
- PLD( bge 13b )
+ bhs 12b
pop {r5 - r8, r10}
cfi_adjust_cfa_offset (-20)
diff --git a/sysdeps/arm/memmove.S b/sysdeps/arm/memmove.S
index 954037ef3a..0d07b76ee6 100644
--- a/sysdeps/arm/memmove.S
+++ b/sysdeps/arm/memmove.S
@@ -85,7 +85,7 @@ ENTRY(memmove)
add r1, r1, r2
add r0, r0, r2
subs r2, r2, #4
- blt 8f
+ blo 8f
ands ip, r0, #3
PLD( pld [r1, #-4] )
bne 9f
@@ -99,7 +99,7 @@ ENTRY(memmove)
cfi_rel_offset (r6, 4)
cfi_rel_offset (r7, 8)
cfi_rel_offset (r8, 12)
- blt 5f
+ blo 5f
CALGN( ands ip, r1, #31 )
CALGN( sbcsne r4, ip, r2 ) @ C is always set here
@@ -114,9 +114,9 @@ ENTRY(memmove)
#endif
PLD( pld [r1, #-4] )
-2: PLD( subs r2, r2, #96 )
+2: PLD( cmp r2, #96 )
PLD( pld [r1, #-32] )
- PLD( blt 4f )
+ PLD( blo 4f )
PLD( pld [r1, #-64] )
PLD( pld [r1, #-96] )
@@ -124,9 +124,7 @@ ENTRY(memmove)
4: ldmdb r1!, {r3, r4, r5, r6, r7, r8, ip, lr}
subs r2, r2, #32
stmdb r0!, {r3, r4, r5, r6, r7, r8, ip, lr}
- bge 3b
- PLD( cmn r2, #96 )
- PLD( bge 4b )
+ bhs 3b
5: ands ip, r2, #28
rsb ip, ip, #32
@@ -237,7 +235,7 @@ ENTRY(memmove)
strbge r4, [r0, #-1]!
subs r2, r2, ip
strb lr, [r0, #-1]!
- blt 8b
+ blo 8b
ands ip, r1, #3
beq 1b
@@ -251,7 +249,7 @@ ENTRY(memmove)
.macro backward_copy_shift push pull
subs r2, r2, #28
- blt 14f
+ blo 14f
CALGN( ands ip, r1, #31 )
CALGN( rsb ip, ip, #32 )
@@ -268,9 +266,9 @@ ENTRY(memmove)
cfi_rel_offset (r10, 16)
PLD( pld [r1, #-4] )
- PLD( subs r2, r2, #96 )
+ PLD( cmp r2, #96 )
PLD( pld [r1, #-32] )
- PLD( blt 13f )
+ PLD( blo 13f )
PLD( pld [r1, #-64] )
PLD( pld [r1, #-96] )
@@ -295,9 +293,7 @@ ENTRY(memmove)
mov r4, r4, PUSH #\push
orr r4, r4, r3, PULL #\pull
stmdb r0!, {r4 - r8, r10, ip, lr}
- bge 12b
- PLD( cmn r2, #96 )
- PLD( bge 13b )
+ bhs 12b
pop {r5 - r8, r10}
cfi_adjust_cfa_offset (-20)
--
2.17.1
--
Evgeny Eremin
Senior Software Engineer
Open Mobile Platform
https://omprussia.ru
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-03 17:55 [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620] Evgeny Eremin
@ 2020-06-04 9:16 ` Florian Weimer
2020-06-04 17:30 ` Evgeny Eremin
2020-06-26 13:56 ` Evgeny Eremin
0 siblings, 2 replies; 10+ messages in thread
From: Florian Weimer @ 2020-06-04 9:16 UTC (permalink / raw)
To: Evgeny Eremin
Cc: libc-alpha, Konstantin Karasev, Anton Rybakov,
Ildar Kamaletdinov, Alexander Anisimov
* Evgeny Eremin:
> Hi,
>
> Unsigned branch instructions could be used for r2 to fix the wrong
> behavior when a negative length is passed to memcpy and memmove
> (sysdeps/arm).
>
> An In-house testing hasn't reveal any functional regressions.
> Performance measurement & comparison are yet be done but the patch
> doesn't change the logic too much.
>
> This partially fixes CVE-2020-6096 [1] "GNU glibc ARMv7 memcpy() memory
> corruption vulnerability".
>
> Signed-off-by: Konstantin Karasev <k.karasev@omprussia.ru>
> Signed-off-by: Anton Rybakov <a.rybakov@omprussia.ru>
> Signed-off-by: Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>
> Signed-off-by: Alexander Anisimov <a.anisimov@omprussia.ru>
>
> [1] https://nvd.nist.gov/vuln/detail/CVE-2020-6096
Thanks for working on this. Is this contribution covered by a copyright
assignment to the FSF? If not, would you be willing to file the
required paperwork with the FSF?
<https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
Do your changes fix string/tst-memmove-overflow for the baseline arm
implementation?
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-04 9:16 ` Florian Weimer
@ 2020-06-04 17:30 ` Evgeny Eremin
2020-06-07 12:44 ` Evgeny Eremin
2020-06-26 13:56 ` Evgeny Eremin
1 sibling, 1 reply; 10+ messages in thread
From: Evgeny Eremin @ 2020-06-04 17:30 UTC (permalink / raw)
To: libc-alpha
On Thu, Jun 04, 2020 at 11:16:16AM +0200, Florian Weimer wrote:
> Thanks for working on this. Is this contribution covered by a copyright
> assignment to the FSF? If not, would you be willing to file the
> required paperwork with the FSF?
>
> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
Got OK from our legal department and started the assignment process
using [1]. (Please guide me if I need to fill some other form.)
> Do your changes fix string/tst-memmove-overflow for the baseline arm
> implementation?
Yes, sure. We've payed additional attention to this particular case and
it XPASS-ed successfuly. Performance tests we've performed also look good,
showing very similar timings comparing to unpatched code for different
buffer lengths.
[1] http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
--
Evgeny Eremin
Senior Software Engineer
Open Mobile Platform
https://omprussia.ru
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-04 17:30 ` Evgeny Eremin
@ 2020-06-07 12:44 ` Evgeny Eremin
0 siblings, 0 replies; 10+ messages in thread
From: Evgeny Eremin @ 2020-06-07 12:44 UTC (permalink / raw)
To: libc-alpha
On Thu, Jun 04, 2020 at 08:30:31PM +0300, Evgeny Eremin wrote:
> On Thu, Jun 04, 2020 at 11:16:16AM +0200, Florian Weimer wrote:
>
> > Thanks for working on this. Is this contribution covered by a copyright
> > assignment to the FSF? If not, would you be willing to file the
> > required paperwork with the FSF?
> >
> > <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
>
> Got OK from our legal department and started the assignment process
> using [1]. (Please guide me if I need to fill some other form.)
BTW, while it takes some time to complete, I could start fixing issues if the
patch has any.
--
Thanks,
Evgeny
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-04 9:16 ` Florian Weimer
2020-06-04 17:30 ` Evgeny Eremin
@ 2020-06-26 13:56 ` Evgeny Eremin
2020-06-29 9:49 ` Florian Weimer
1 sibling, 1 reply; 10+ messages in thread
From: Evgeny Eremin @ 2020-06-26 13:56 UTC (permalink / raw)
To: libc-alpha; +Cc: Florian Weimer, Carlos O'Donell, Alexander Anisimov
On Thu, Jun 04, 2020 at 11:16:16AM +0200, Florian Weimer wrote:
> ...
>
> Thanks for working on this. Is this contribution covered by a copyright
> assignment to the FSF? If not, would you be willing to file the
> required paperwork with the FSF?
>
> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
The corporate assignment process with the FSF has been completed, so any
Open Mobile Platform employee can contribute to glibc.
--
Thanks,
Evgeny
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-26 13:56 ` Evgeny Eremin
@ 2020-06-29 9:49 ` Florian Weimer
2020-06-29 10:14 ` Dmitry V. Levin
0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2020-06-29 9:49 UTC (permalink / raw)
To: libc-alpha
* Evgeny Eremin:
> On Thu, Jun 04, 2020 at 11:16:16AM +0200, Florian Weimer wrote:
>> ...
>>
>> Thanks for working on this. Is this contribution covered by a copyright
>> assignment to the FSF? If not, would you be willing to file the
>> required paperwork with the FSF?
>>
>> <https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment>
>
> The corporate assignment process with the FSF has been completed, so any
> Open Mobile Platform employee can contribute to glibc.
It's still confusing because we can't easily match a contribution from
omprussia.ru to the copyright assignment on file for Open Mobile
Platform LLС. I've been told that the recent copyright assignment
record does not reference omprussia.ru. 8-(
Thanks,
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-29 9:49 ` Florian Weimer
@ 2020-06-29 10:14 ` Dmitry V. Levin
2020-06-29 10:28 ` Florian Weimer
0 siblings, 1 reply; 10+ messages in thread
From: Dmitry V. Levin @ 2020-06-29 10:14 UTC (permalink / raw)
To: libc-alpha
On Mon, Jun 29, 2020 at 11:49:53AM +0200, Florian Weimer via Libc-alpha wrote:
[...]
> It's still confusing because we can't easily match a contribution from
> omprussia.ru to the copyright assignment on file for Open Mobile
> Platform LLС. I've been told that the recent copyright assignment
> record does not reference omprussia.ru. 8-(
FWIW, there is a strong connection between them:
$ whois omprussia.ru |grep ^org:
org: Open Mobile Platform, LLC
--
ldv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-29 10:14 ` Dmitry V. Levin
@ 2020-06-29 10:28 ` Florian Weimer
2020-06-30 14:39 ` Evgeny Eremin
0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2020-06-29 10:28 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: libc-alpha
* Dmitry V. Levin:
> On Mon, Jun 29, 2020 at 11:49:53AM +0200, Florian Weimer via Libc-alpha wrote:
> [...]
>> It's still confusing because we can't easily match a contribution from
>> omprussia.ru to the copyright assignment on file for Open Mobile
>> Platform LLС. I've been told that the recent copyright assignment
>> record does not reference omprussia.ru. 8-(
>
> FWIW, there is a strong connection between them:
>
> $ whois omprussia.ru |grep ^org:
> org: Open Mobile Platform, LLC
Thanks, good point. I had already planned to commit these patches after
further testing anyway. 8->
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-30 14:39 ` Evgeny Eremin
@ 2020-06-30 14:29 ` Florian Weimer
0 siblings, 0 replies; 10+ messages in thread
From: Florian Weimer @ 2020-06-30 14:29 UTC (permalink / raw)
To: libc-alpha
* Evgeny Eremin:
> On Mon, Jun 29, 2020 at 12:28:22PM +0200, Florian Weimer via Libc-alpha wrote:
>> * Dmitry V. Levin:
>>
>> > On Mon, Jun 29, 2020 at 11:49:53AM +0200, Florian Weimer via Libc-alpha wrote:
>> > [...]
>> >> It's still confusing because we can't easily match a contribution from
>> >> omprussia.ru to the copyright assignment on file for Open Mobile
>> >> Platform LLС. I've been told that the recent copyright assignment
>> >> record does not reference omprussia.ru. 8-(
>> >
>> > FWIW, there is a strong connection between them:
>> >
>> > $ whois omprussia.ru |grep ^org:
>> > org: Open Mobile Platform, LLC
>>
>> Thanks, good point. I had already planned to commit these patches after
>> further testing anyway. 8->
>
> Maybe I should add a copyright notice like "Copyright (c) 2020 Open
> Mobile Platform LLС" to make it explicit? Or a whois request will
> be enough?
The copyright is supposed to reside with the FSF, due to the code
submission and the copyright assignment.
Thanks,
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620]
2020-06-29 10:28 ` Florian Weimer
@ 2020-06-30 14:39 ` Evgeny Eremin
2020-06-30 14:29 ` Florian Weimer
0 siblings, 1 reply; 10+ messages in thread
From: Evgeny Eremin @ 2020-06-30 14:39 UTC (permalink / raw)
To: libc-alpha
On Mon, Jun 29, 2020 at 12:28:22PM +0200, Florian Weimer via Libc-alpha wrote:
> * Dmitry V. Levin:
>
> > On Mon, Jun 29, 2020 at 11:49:53AM +0200, Florian Weimer via Libc-alpha wrote:
> > [...]
> >> It's still confusing because we can't easily match a contribution from
> >> omprussia.ru to the copyright assignment on file for Open Mobile
> >> Platform LLС. I've been told that the recent copyright assignment
> >> record does not reference omprussia.ru. 8-(
> >
> > FWIW, there is a strong connection between them:
> >
> > $ whois omprussia.ru |grep ^org:
> > org: Open Mobile Platform, LLC
>
> Thanks, good point. I had already planned to commit these patches after
> further testing anyway. 8->
Maybe I should add a copyright notice like "Copyright (c) 2020 Open
Mobile Platform LLС" to make it explicit? Or a whois request will
be enough?
--
Thanks,
Evgeny
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-06-30 14:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 17:55 [PATCH] arm: fix memcpy and memmove for negative len [BZ #25620] Evgeny Eremin
2020-06-04 9:16 ` Florian Weimer
2020-06-04 17:30 ` Evgeny Eremin
2020-06-07 12:44 ` Evgeny Eremin
2020-06-26 13:56 ` Evgeny Eremin
2020-06-29 9:49 ` Florian Weimer
2020-06-29 10:14 ` Dmitry V. Levin
2020-06-29 10:28 ` Florian Weimer
2020-06-30 14:39 ` Evgeny Eremin
2020-06-30 14:29 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).