Bug 15674 - __memcmp_ssse3 tries to read past the array boundary
Summary: __memcmp_ssse3 tries to read past the array boundary
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.17
: P2 normal
Target Milestone: ---
Assignee: Liubov Dmitrieva
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-25 00:20 UTC by Roni Simonian
Modified: 2014-06-16 16:46 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security+


Attachments
patch for glibc/string/test-memcmp.c (439 bytes, patch)
2013-06-25 00:20 UTC, Roni Simonian
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roni Simonian 2013-06-25 00:20:31 UTC
Created attachment 7093 [details]
patch for glibc/string/test-memcmp.c

Hello,

The bug occurrs when "memcmp(s1, s2, 72)" calls __memcmp_ssse3, and when (s1 + 72) coinsides with a page boundary.

The instruction 

mov	-9(%rdi), %eax

in __memcmp_ssse3 () at ../sysdeps/x86_64/multiarch/memcmp-ssse3.S:1467 causes a segmentation fault.

Example: the page is at the address range 0x2000 - 0x3000, s1=(0x3000 - 72), and the next page is mprotected with PROT_NONE. $rdi=0x3008, and the "mov" instruction tries to read 4 bytes starting at 0x3008-9, crossing the boundary.

The test case fails for the number of array sizes (72 is just one of them). Other sizes are 48, 65-75, etc. 


The reported glibc version is 2.18, but the same issue occurs in 2.15.


How to reproduce:

(1) Modify glibc/string/test-memcmp.c (the patch is attached)

(2) Run "env LANGUAGE=C LC_ALL=C make check"

test-memcmp-ifunc will fail with segfault. In string/test-memcmp-ifunc.out:

.....
check2: length=48, simple_memcmp
check2: length=48, __memcmp_sse4_1
check2: length=48, __memcmp_ssse3


Thank you.

Roni.
Comment 1 Paul Pluzhnikov 2013-06-25 01:24:36 UTC
Google ref b/9530601
Comment 2 Liubov Dmitrieva 2013-06-25 12:51:02 UTC
This patch fixes the issue. Tested on my machines.

diff --git a/sysdeps/x86_64/multiarch/memcmp-ssse3.S b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
index bdd2ed2..e319df9 100644
--- a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
+++ b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
@@ -1463,10 +1463,8 @@ L(next_24_bytes):
        test    $0x40, %dh
        jnz     L(Byte22)

-       mov     -9(%rdi), %eax
-       and     $0xff, %eax
-       mov     -9(%rsi), %edx
-       and     $0xff, %edx
+       movzbl  -9(%rdi), %eax
+       movzbl  -9(%rsi), %edx
        sub     %edx, %eax
        ret
 # else
Comment 3 Ondrej Bilka 2013-06-25 13:18:10 UTC
On Tue, Jun 25, 2013 at 12:51:02PM +0000, liubov.dmitrieva at gmail dot com wrote:
> http://sourceware.org/bugzilla/show_bug.cgi?id=15674
> 
> --- Comment #2 from Liubov Dmitrieva <liubov.dmitrieva at gmail dot com> ---
> This patch fixes the issue. Tested on my machines.
> 
> diff --git a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> index bdd2ed2..e319df9 100644
> --- a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> +++ b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> @@ -1463,10 +1463,8 @@ L(next_24_bytes):
>         test    $0x40, %dh
>         jnz     L(Byte22)
> 
> -       mov     -9(%rdi), %eax
> -       and     $0xff, %eax
> -       mov     -9(%rsi), %edx
> -       and     $0xff, %edx
> +       movzbl  -9(%rdi), %eax
> +       movzbl  -9(%rsi), %edx
>         sub     %edx, %eax
>         ret
>  # else
> 
Could you send this to libc-alpha. Where was problem? This code looks at first
glance equivalent.
Comment 4 Liubov Dmitrieva 2013-06-25 13:24:56 UTC
(In reply to Ondrej Bilka from comment #3)
> On Tue, Jun 25, 2013 at 12:51:02PM +0000, liubov.dmitrieva at gmail dot com
> wrote:
> > http://sourceware.org/bugzilla/show_bug.cgi?id=15674
> > 
> > --- Comment #2 from Liubov Dmitrieva <liubov.dmitrieva at gmail dot com> ---
> > This patch fixes the issue. Tested on my machines.
> > 
> > diff --git a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> > b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> > index bdd2ed2..e319df9 100644
> > --- a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> > +++ b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> > @@ -1463,10 +1463,8 @@ L(next_24_bytes):
> >         test    $0x40, %dh
> >         jnz     L(Byte22)
> > 
> > -       mov     -9(%rdi), %eax
> > -       and     $0xff, %eax
> > -       mov     -9(%rsi), %edx
> > -       and     $0xff, %edx
> > +       movzbl  -9(%rdi), %eax
> > +       movzbl  -9(%rsi), %edx
> >         sub     %edx, %eax
> >         ret
> >  # else
> > 
> Could you send this to libc-alpha. Where was problem? This code looks at
> first
> glance equivalent.

Ok. I will send.

Movzbl reads only one byte and doesn't cross the boundary. 
Mov reads 4 bytes.
For MOVZBL, the low 8 bits of the destination are replaced by the source operand. the top 24 bits are set to 0. The source operand is unaffected.
Comment 5 Andreas Jaeger 2013-06-25 13:26:03 UTC
I suggest you add a comment that you only do single byte reads to not cross page boundaries.
Comment 7 Joseph Myers 2013-06-27 01:01:56 UTC
Fixed.
Comment 8 Florian Weimer 2014-06-16 16:46:03 UTC
Introduced in glibc 2.15, fixed in glibc 2.18.