diff mbox

Fix powerpc64/power7 memchr for large input sizes

Message ID 1481802979-4302-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto Dec. 15, 2016, 11:56 a.m. UTC 
Current optimized powercp64/power7 memchr uses a strategy to check for
p versus align(p+n) (where 'p' is the input char pointer and n the
maximum size to check for the byte) without taking care for possible
overflow on the pointer addition in case of large 'n'.

It was triggered by 3038145ca23 where default rawmemchr (used to
created ppc64 rawmemchr in ifunc selection) now uses memchr (p, c, (size_t)-1)
on its implementation.

This patch fixes it by implement a satured addition where overflows
sets the maximum pointer size to UINTPTR_MAX.

Checked on powerpc64le-linux-gnu.

	[BZ# 20971]
	* sysdeps/powerpc/powerpc64/power7/memchr.S (__memchr): Avoid
	overflow in pointer addition.
---
 ChangeLog                                 |  6 ++++++
 sysdeps/powerpc/powerpc64/power7/memchr.S | 12 +++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Florian Weimer Dec. 15, 2016, 12:21 p.m. UTC | #1 
On 12/15/2016 12:56 PM, Adhemerval Zanella wrote:
> Current optimized powercp64/power7 memchr uses a strategy to check for
> p versus align(p+n) (where 'p' is the input char pointer and n the
> maximum size to check for the byte) without taking care for possible
> overflow on the pointer addition in case of large 'n'.
>
> It was triggered by 3038145ca23 where default rawmemchr (used to
> created ppc64 rawmemchr in ifunc selection) now uses memchr (p, c, (size_t)-1)
> on its implementation.

Do we already have a test case which covers this?

Thanks,
Florian
Adhemerval Zanella Netto Dec. 15, 2016, 4:59 p.m. UTC | #2 
On 15/12/2016 10:21, Florian Weimer wrote:
> On 12/15/2016 12:56 PM, Adhemerval Zanella wrote:
>> Current optimized powercp64/power7 memchr uses a strategy to check for
>> p versus align(p+n) (where 'p' is the input char pointer and n the
>> maximum size to check for the byte) without taking care for possible
>> overflow on the pointer addition in case of large 'n'.
>>
>> It was triggered by 3038145ca23 where default rawmemchr (used to
>> created ppc64 rawmemchr in ifunc selection) now uses memchr (p, c, (size_t)-1)
>> on its implementation.
> 
> Do we already have a test case which covers this?

No, but it is triggered indirectly by default powerpc64 rawmemchr
on ifunc build (__rawmemchr_ppc).  But now you noted, I think it
worth a testcase addition.
cseo Dec. 15, 2016, 6:11 p.m. UTC | #3 
On 12/15/16, 9:56 AM, "Adhemerval Zanella" <libc-alpha-owner@sourceware.org on behalf of adhemerval.zanella@linaro.org> wrote:

    This patch fixes it by implement a satured addition where overflows
    sets the maximum pointer size to UINTPTR_MAX.
    
    Checked on powerpc64le-linux-gnu.
    

LGTM.

--
Carlos Eduardo Seo
Software Engineer - Linux on Power Toolchain
cseo@linux.vnet.ibm.com
diff mbox

Patch

diff --git a/sysdeps/powerpc/powerpc64/power7/memchr.S b/sysdeps/powerpc/powerpc64/power7/memchr.S
index 03f0d7c..14059e2 100644
--- a/sysdeps/powerpc/powerpc64/power7/memchr.S
+++ b/sysdeps/powerpc/powerpc64/power7/memchr.S
@@ -26,7 +26,17 @@  ENTRY (__memchr)
 	dcbt	0,r3
 	clrrdi  r8,r3,3
 	insrdi	r4,r4,8,48
-	add	r7,r3,r5      /* Calculate the last acceptable address.  */
+
+	/* Calculate the last acceptable address and check for possible
+	   addition overflow by using satured math:
+	   r7 = r3 + r5
+	   r7 |= -(r7 < x)  */
+	add	r7,r3,r5
+	subfc	r6,r3,r7
+	subfe	r9,r9,r9
+	extsw	r6,r9
+	or	r7,r7,r6
+
 	insrdi	r4,r4,16,32
 	cmpldi	r5,32
 	li	r9, -1