diff mbox series

arm: fix memcpy and memmove for negative len [BZ #25620]

Message ID 20200603175510.GA16145@arch-home-a35
State Committed
Commit 79a4fa341b8a89cb03f84564fd72abaa1a2db394
Headers
Series arm: fix memcpy and memmove for negative len [BZ #25620] |

Commit Message

Evgeny Eremin June 3, 2020, 5:55 p.m. UTC 
  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(-)

Comments

Florian Weimer June 4, 2020, 9:16 a.m. UTC | #1 
* 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
Evgeny Eremin June 4, 2020, 5:30 p.m. UTC | #2 
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 June 7, 2020, 12:44 p.m. UTC | #3 
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.
Evgeny Eremin June 26, 2020, 1:56 p.m. UTC | #4 
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.
Florian Weimer June 29, 2020, 9:49 a.m. UTC | #5 
* 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
Dmitry V. Levin June 29, 2020, 10:14 a.m. UTC | #6 
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
Florian Weimer June 29, 2020, 10:28 a.m. UTC | #7 
* 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
Florian Weimer June 30, 2020, 2:29 p.m. UTC | #8 
* 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
Evgeny Eremin June 30, 2020, 2:39 p.m. UTC | #9 
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?
diff mbox series

Patch

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)