public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* Re: [PATCH v2] memcpy: use bhs/bls instead of bge/blt [BZ #25620]
@ 2020-04-15 11:59 Wilco Dijkstra
  0 siblings, 0 replies; 10+ messages in thread
From: Wilco Dijkstra @ 2020-04-15 11:59 UTC (permalink / raw)
  To: libc-alpha, zhuyan34

Hi Zhuyan,

As Joseph mentioned the test should not be target specific. If you're thinking of
just adding a test for memcpy for now, then adding it to string/test-memcpy.c would
be fine given it should be part of the basic tests. Existing tests like test-memchr.c already
have special checks for huge inputs. Once we have a working implementation, it would
be easy to copy it to test-memmove.c, test-memset.c etc.

In particular do_test1 shows how to do a mmap with an inaccessible page to avoid
corrupting any memory which could cause the test to pass or fail incorrectly.
The test should run all ifuncs as well, not just one.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] memcpy: use bhs/bls instead of bge/blt [BZ #25620]
@ 2020-05-06 12:40 zhuyan (M)
  0 siblings, 0 replies; 10+ messages in thread
From: zhuyan (M) @ 2020-05-06 12:40 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

* Florian Weimer:

> > -	blt	.Ltail63aligned
> > +	bls	.Ltail63aligned
> 
> Is bls really the unsigned variant of blt?  Why not blo?

Yes, it is more appropriate to use blo instead of blt.



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] memcpy: use bhs/bls instead of bge/blt [BZ #25620]
@ 2020-05-01 12:58 Wilco Dijkstra
  0 siblings, 0 replies; 10+ messages in thread
From: Wilco Dijkstra @ 2020-05-01 12:58 UTC (permalink / raw)
  To: Florian Weimer, zhuyan34; +Cc: 'GNU C Library'

Hi Florian,

>> -	blt	.Ltail63aligned
>> +	bls	.Ltail63aligned
>
> Is bls really the unsigned variant of blt?  Why not blo?

Well spotted! Indeed bls is incorrect here since the tail code cannot handle 64 bytes.

Note there are several uses of PL/MI that need to be changed too:

	subs	count, count, #64
	ldrmi	tmp2, [sp], #FRAME_SIZE
	bmi	.Ltail63unaligned

	subs	count, count, #64
	bpl	1b

Basically the initial test protects all the others, but when that one is changed to
unsigned, all the other signed checks fail, and we're basically left with the exact
same problem. Hence the need to change all signed condition codes and add
tests that catch all possible cases.

Note a very quick grep shows memcpy.S, armv7/multiarch/memcpy_impl.S,
memmove.S, strlen.S and armv6t2.memchr.S using signed conditions.

Cheers,
Wilco

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2] memcpy: use bhs/bls instead of bge/blt [BZ #25620]
@ 2020-04-13 14:16 zhuyan (M)
  2020-04-14 22:26 ` Joseph Myers
  2020-04-30 20:22 ` Florian Weimer
  0 siblings, 2 replies; 10+ messages in thread
From: zhuyan (M) @ 2020-04-13 14:16 UTC (permalink / raw)
  To: libc-alpha; +Cc: Carlos O'Donell, Joseph Myers

In ARMv7, the memcpy() implementation allows for program execution to continue in scenarios where a segmentation fault or crash should have occurred. The dangers occur in that subsequent execution and iterations of this code will be executed with this corrupted data.

Such as, we use 'memcpy' copy 0x80000000 byte to buffer(The buffer size is 100 bytes), it didn't crash.

Reference link: https://sourceware.org/bugzilla/attachment.cgi?id=12334&action=edit

Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
---
 sysdeps/arm/Makefile                      |  6 +++-
 sysdeps/arm/armv7/multiarch/memcpy_impl.S | 14 ++++----
 sysdeps/arm/tst-armv7memcpybign.c         | 54 +++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 8 deletions(-)  create mode 100644 sysdeps/arm/tst-armv7memcpybign.c

diff --git a/sysdeps/arm/Makefile b/sysdeps/arm/Makefile index ad2042b93a..7c1f19e225 100644
--- a/sysdeps/arm/Makefile
+++ b/sysdeps/arm/Makefile
@@ -19,21 +19,25 @@ lib-noranlib: $(objpfx)libgcc-stubs.a
 
 ifeq ($(build-shared),yes)
 ifeq ($(have-arm-tls-desc),yes)
-tests += tst-armtlsdescloc tst-armtlsdescextnow tst-armtlsdescextlazy
+tests += tst-armtlsdescloc tst-armtlsdescextnow tst-armtlsdescextlazy 
+tst-armv7memcpybign
 modules-names += tst-armtlsdesclocmod
 modules-names += tst-armtlsdescextlazymod tst-armtlsdescextnowmod
+modeles-names += tst-armv7memcpybignmod
 CPPFLAGS-tst-armtlsdescextnowmod.c += -Dstatic=  CPPFLAGS-tst-armtlsdescextlazymod.c += -Dstatic=  CFLAGS-tst-armtlsdesclocmod.c += -mtls-dialect=gnu2  CFLAGS-tst-armtlsdescextnowmod.c += -mtls-dialect=gnu2  CFLAGS-tst-armtlsdescextlazymod.c += -mtls-dialect=gnu2
+CFLAGS-tst-armv7memcpybign.c += -mtls-dialect=gnu2
 LDFLAGS-tst-armtlsdescextnowmod.so += -Wl,-z,now  tst-armtlsdescloc-ENV = LD_BIND_NOW=1  tst-armtlsdescextnow-ENV = LD_BIND_NOW=1  tst-armtlsdescextlazy-ENV = LD_BIND_NOW=1
+tst-armv7memcpybign-ENV = LD_BIND_NOW=1
 $(objpfx)tst-armtlsdescloc: $(objpfx)tst-armtlsdesclocmod.so
 $(objpfx)tst-armtlsdescextnow: $(objpfx)tst-armtlsdescextnowmod.so
 $(objpfx)tst-armtlsdescextlazy: $(objpfx)tst-armtlsdescextlazymod.so
+$(objpfx)tst-armv7memcpybign: $(objpfx)tst-armv7memcpybignmod.so
 endif
 endif
 endif
diff --git a/sysdeps/arm/armv7/multiarch/memcpy_impl.S b/sysdeps/arm/armv7/multiarch/memcpy_impl.S
index bf4ac7077f..7455bdc6c7 100644
--- a/sysdeps/arm/armv7/multiarch/memcpy_impl.S
+++ b/sysdeps/arm/armv7/multiarch/memcpy_impl.S
@@ -268,7 +268,7 @@ ENTRY(memcpy)
 
 	mov	dst, dstin	/* Preserve dstin, we need to return it.  */
 	cmp	count, #64
-	bge	.Lcpy_not_short
+	bhs	.Lcpy_not_short
 	/* Deal with small copies quickly by dropping straight into the
 	   exit block.  */
 
@@ -351,10 +351,10 @@ ENTRY(memcpy)
 
 1:
 	subs	tmp2, count, #64	/* Use tmp2 for count.  */
-	blt	.Ltail63aligned
+	bls	.Ltail63aligned
 
 	cmp	tmp2, #512
-	bge	.Lcpy_body_long
+	bhs	.Lcpy_body_long
 
 .Lcpy_body_medium:			/* Count in tmp2.  */
 #ifdef USE_VFP
@@ -378,7 +378,7 @@ ENTRY(memcpy)
 	add	src, src, #64
 	vstr	d1, [dst, #56]
 	add	dst, dst, #64
-	bge	1b
+	bhs	1b
 	tst	tmp2, #0x3f
 	beq	.Ldone
 
@@ -412,7 +412,7 @@ ENTRY(memcpy)
 	ldrd	A_l, A_h, [src, #64]!
 	strd	A_l, A_h, [dst, #64]!
 	subs	tmp2, tmp2, #64
-	bge	1b
+	bhs	1b
 	tst	tmp2, #0x3f
 	bne	1f
 	ldr	tmp2,[sp], #FRAME_SIZE
@@ -482,7 +482,7 @@ ENTRY(memcpy)
 	add	src, src, #32
 
 	subs	tmp2, tmp2, #prefetch_lines * 64 * 2
-	blt	2f
+	bls	2f
 1:
 	cpy_line_vfp	d3, 0
 	cpy_line_vfp	d4, 64
@@ -494,7 +494,7 @@ ENTRY(memcpy)
 	add	dst, dst, #2 * 64
 	add	src, src, #2 * 64
 	subs	tmp2, tmp2, #prefetch_lines * 64
-	bge	1b
+	bhs	1b
 
 2:
 	cpy_tail_vfp	d3, 0
diff --git a/sysdeps/arm/tst-armv7memcpybign.c b/sysdeps/arm/tst-armv7memcpybign.c
new file mode 100644
index 0000000000..c9f0873c90
--- /dev/null
+++ b/sysdeps/arm/tst-armv7memcpybign.c
@@ -0,0 +1,54 @@
+/* Test scenes where ARMv7 memcpy parameter num is very large.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <string.h>
+
+int g_ret = 0;
+
+void
+sigsegv_handle(int signum)
+{
+  printf("Enter sigsegv_handle function\n");
+  g_ret = signum;
+  exit(0);
+}
+
+int
+memcpy_big_n (void)
+{
+  char buf[100] = {0};
+  memcpy(buf, "abcd", 0x80000000);
+}
+
+int
+do_test (void)
+{
+  signal(SIGSEGV, sigsegv_handle);
+  memcpy_big_n();
+  if (g_ret == 0)
+    return 1;
+  else
+    return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
--
2.12.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-06 12:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15 11:59 [PATCH v2] memcpy: use bhs/bls instead of bge/blt [BZ #25620] Wilco Dijkstra
  -- strict thread matches above, loose matches on Subject: below --
2020-05-06 12:40 zhuyan (M)
2020-05-01 12:58 Wilco Dijkstra
2020-04-13 14:16 zhuyan (M)
2020-04-14 22:26 ` Joseph Myers
2020-04-21 14:36   ` Florian Weimer
2020-04-21 21:27     ` Joseph Myers
2020-04-28 21:14       ` Florian Weimer
2020-04-30 20:33       ` Florian Weimer
2020-04-30 20:22 ` Florian Weimer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).