diff mbox series

power8: fix read past array bounds in strncasecmp (bug 34683)

Message ID mvmbj9g6twj.fsf@suse.de (mailing list archive)
State Superseded
Headers
Series power8: fix read past array bounds in strncasecmp (bug 34683) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Andreas Schwab Sept. 29, 2026, 1:58 p.m. UTC 
  Move the load of the characters to the top of the loops, so that they
occur after the bounds check for the strncasecmp implementation.  Amend
the page test for strncasecmp to also test with non-null-terminated arrays
that trigger this bug.
---
 string/test-strncasecmp.c                     | 16 ++++++++++++++++
 sysdeps/powerpc/powerpc64/power8/strcasecmp.S | 14 ++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

Comments

Adhemerval Zanella Sept. 29, 2026, 2:05 p.m. UTC | #1 
On 29/09/26 10:58, Andreas Schwab wrote:
> Move the load of the characters to the top of the loops, so that they
> occur after the bounds check for the strncasecmp implementation.  Amend
> the page test for strncasecmp to also test with non-null-terminated arrays
> that trigger this bug.

We have two slight different patches that fix the same issue [1]. Which should
we use?

[1] https://patchwork.sourceware.org/project/glibc/patch/20260929112822.2099581-1-smonga@linux.ibm.com/

> ---
>  string/test-strncasecmp.c                     | 16 ++++++++++++++++
>  sysdeps/powerpc/powerpc64/power8/strcasecmp.S | 14 ++++++--------
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6b00113e66..3fced4202b 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -199,6 +199,22 @@ do_page_tests (void)
>  			  exp_result);
>  	}
>      }
> +
> +  /* Like strncmp, the arrays passed to strncasecmp are not required to be
> +     null-terminated.  Check that the implementations do not read beyond
> +     the end of the arrays.  */
> +
> +  s1[maxoffset - 1] = 'a';
> +  s2[maxoffset - 1] = 'a';
> +  for (size_t off1 = 0; off1 < maxoffset; off1++)
> +    {
> +      for (size_t off2 = 0; off2 < maxoffset; off2++)
> +	{
> +	  size_t off_max = off1 < off2 ? off2 : off1;
> +	  FOR_EACH_IMPL (impl, 0)
> +	    check_result (impl, s1 + off1, s2 + off2, maxoffset - off_max, 0);
> +	}
> +    }
>  }
>  
>  static void
> diff --git a/sysdeps/powerpc/powerpc64/power8/strcasecmp.S b/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
> index 3c45e9ea50..22db297855 100644
> --- a/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
> +++ b/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
> @@ -355,8 +355,6 @@ L(bytebybyte):
>  	offset and string descriptors are only updated in the end
>  	of loop unrolling. */
>  	ld	rLOC, LOCALE_CTYPE_TOLOWER(rLOC)
> -	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
> -	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
>  #ifdef USE_AS_STRNCASECMP
>  	rldicl	rTMP, r5, 62, 2
>  	cmpdi	cr7, rTMP, 0
> @@ -364,6 +362,8 @@ L(bytebybyte):
>  	mtctr	rTMP
>  #endif
>  L(loop):
> +	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
> +	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
>  	cmpdi	rCHAR1, 0		/* *s1 == '\0' ? */
>  	sldi	rADDR1, rCHAR1, 2	/* Calculate address for tolower(*s1) */
>  	sldi	rADDR2, rCHAR2, 2	/* Calculate address for tolower(*s2) */
> @@ -404,13 +404,11 @@ L(loop):
>  	lwzx	rLWR2, rLOC, rADDR2
>  	cmpw	cr1, rLWR1, rLWR2
>  	crorc	4*cr1+eq,eq,4*cr1+eq
> -	beq     cr1, L(done)
> -	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
> -	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
>  #ifdef USE_AS_STRNCASECMP
> +	beq     cr1, L(done)
>  	bdnz	L(loop)
>  #else
> -	b	L(loop)
> +	bne     cr1, L(loop)
>  #endif
>  #ifdef USE_AS_STRNCASECMP
>  L(lessthan4):
> @@ -419,6 +417,8 @@ L(lessthan4):
>  	beq	cr7, L(retnull)
>  	mtctr	r5
>  L(loop1):
> +	lbz	rCHAR1, 0(rSTR1)
> +	lbz	rCHAR2, 0(rSTR2)
>  	cmpdi	rCHAR1, 0
>  	sldi	rADDR1, rCHAR1, 2
>  	sldi	rADDR2, rCHAR2, 2
> @@ -429,8 +429,6 @@ L(loop1):
>  	beq	cr1, L(done)
>  	addi	rSTR1, rSTR1, 1
>  	addi	rSTR2, rSTR2, 1
> -	lbz	rCHAR1, 0(rSTR1)
> -	lbz	rCHAR2, 0(rSTR2)
>  	bdnz	L(loop1)
>  #endif
>  L(done):
Andreas Schwab Sept. 29, 2026, 2:24 p.m. UTC | #2 
On Sep 29 2026, Adhemerval Zanella Netto wrote:

> On 29/09/26 10:58, Andreas Schwab wrote:
>> Move the load of the characters to the top of the loops, so that they
>> occur after the bounds check for the strncasecmp implementation.  Amend
>> the page test for strncasecmp to also test with non-null-terminated arrays
>> that trigger this bug.
>
> We have two slight different patches that fix the same issue [1]. Which should
> we use?
>
> [1] https://patchwork.sourceware.org/project/glibc/patch/20260929112822.2099581-1-smonga@linux.ibm.com/

I haven't seen that one, it looks better than mine.
Sachin Monga Sept. 30, 2026, 6:22 p.m. UTC | #3 
On 29/09/26 7:54 pm, Andreas Schwab wrote:
> I haven't seen that one, it looks better than mine.

Thanks Andreas — and sorry for the duplicated effort. Since you prefer 
this version, and the advisory (GLIBC-SA-2026-0024) is already public 
with no fix upstream yet, I am pushing it.

Regards:
Sachin.
diff mbox series

Patch

diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
index 6b00113e66..3fced4202b 100644
--- a/string/test-strncasecmp.c
+++ b/string/test-strncasecmp.c
@@ -199,6 +199,22 @@  do_page_tests (void)
 			  exp_result);
 	}
     }
+
+  /* Like strncmp, the arrays passed to strncasecmp are not required to be
+     null-terminated.  Check that the implementations do not read beyond
+     the end of the arrays.  */
+
+  s1[maxoffset - 1] = 'a';
+  s2[maxoffset - 1] = 'a';
+  for (size_t off1 = 0; off1 < maxoffset; off1++)
+    {
+      for (size_t off2 = 0; off2 < maxoffset; off2++)
+	{
+	  size_t off_max = off1 < off2 ? off2 : off1;
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1 + off1, s2 + off2, maxoffset - off_max, 0);
+	}
+    }
 }
 
 static void
diff --git a/sysdeps/powerpc/powerpc64/power8/strcasecmp.S b/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
index 3c45e9ea50..22db297855 100644
--- a/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
+++ b/sysdeps/powerpc/powerpc64/power8/strcasecmp.S
@@ -355,8 +355,6 @@  L(bytebybyte):
 	offset and string descriptors are only updated in the end
 	of loop unrolling. */
 	ld	rLOC, LOCALE_CTYPE_TOLOWER(rLOC)
-	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
-	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
 #ifdef USE_AS_STRNCASECMP
 	rldicl	rTMP, r5, 62, 2
 	cmpdi	cr7, rTMP, 0
@@ -364,6 +362,8 @@  L(bytebybyte):
 	mtctr	rTMP
 #endif
 L(loop):
+	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
+	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
 	cmpdi	rCHAR1, 0		/* *s1 == '\0' ? */
 	sldi	rADDR1, rCHAR1, 2	/* Calculate address for tolower(*s1) */
 	sldi	rADDR2, rCHAR2, 2	/* Calculate address for tolower(*s2) */
@@ -404,13 +404,11 @@  L(loop):
 	lwzx	rLWR2, rLOC, rADDR2
 	cmpw	cr1, rLWR1, rLWR2
 	crorc	4*cr1+eq,eq,4*cr1+eq
-	beq     cr1, L(done)
-	lbz	rCHAR1, 0(rSTR1)	/* Load char from s1 */
-	lbz	rCHAR2, 0(rSTR2)	/* Load char from s2 */
 #ifdef USE_AS_STRNCASECMP
+	beq     cr1, L(done)
 	bdnz	L(loop)
 #else
-	b	L(loop)
+	bne     cr1, L(loop)
 #endif
 #ifdef USE_AS_STRNCASECMP
 L(lessthan4):
@@ -419,6 +417,8 @@  L(lessthan4):
 	beq	cr7, L(retnull)
 	mtctr	r5
 L(loop1):
+	lbz	rCHAR1, 0(rSTR1)
+	lbz	rCHAR2, 0(rSTR2)
 	cmpdi	rCHAR1, 0
 	sldi	rADDR1, rCHAR1, 2
 	sldi	rADDR2, rCHAR2, 2
@@ -429,8 +429,6 @@  L(loop1):
 	beq	cr1, L(done)
 	addi	rSTR1, rSTR1, 1
 	addi	rSTR2, rSTR2, 1
-	lbz	rCHAR1, 0(rSTR1)
-	lbz	rCHAR2, 0(rSTR2)
 	bdnz	L(loop1)
 #endif
 L(done):