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

Message ID 20200603175510.GA16145@arch-home-a35
State New
Headers show
Series
  • arm: fix memcpy and memmove for negative len [BZ #25620]
Related show

Commit Message

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

-- 
2.17.1


-- 
Evgeny Eremin
Senior Software Engineer
Open Mobile Platform
https://omprussia.ru

Comments

Pali Rohár via Libc-alpha June 4, 2020, 9:16 a.m. | #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. | #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
Senior Software Engineer
Open Mobile Platform
https://omprussia.ru
Evgeny Eremin June 7, 2020, 12:44 p.m. | #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.

-- 
Thanks,
Evgeny
Evgeny Eremin June 26, 2020, 1:56 p.m. | #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.

-- 
Thanks,
Evgeny
Pali Rohár via Libc-alpha June 29, 2020, 9:49 a.m. | #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. | #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


-- 
ldv
Pali Rohár via Libc-alpha June 29, 2020, 10:28 a.m. | #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
Pali Rohár via Libc-alpha June 30, 2020, 2:29 p.m. | #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. | #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?

-- 
Thanks,
Evgeny

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)