| Message ID | 1465577084-19849-1-git-send-email-lucian.cojocar@vu.nl |
|---|---|
| State | Accepted |
| Headers | show |
Hi, Any follow-up on this patch? Thanks, Lucian On 06/10/2016 06:44 PM, Lucian Cojocar wrote: > The 'BLT' instruction checks for *signed* values. So if a3, length > parameter of memset, is negative, then value added to the PC will be > large. > > memset(buf, 0xaa, 0xffff0000) triggers the bug. > > GDB session without the patch: > > """ > $ gdb ./main-buggy-memset.elf -q > Reading symbols from ./main-buggy-memset.elf...done. > (gdb) x/i memset > 0x8770 <memset>: mov r3, r0 > (gdb) r > Starting program: /root/memset/main-buggy-memset.elf > > Program received signal SIGSEGV, Segmentation fault. > 0x00048808 in ?? () > """ > > The $pc is outside of the memset function because: > > """ > (gdb) x/i $pc > => 0x87e4 <memset+116>: add pc, pc, r2, lsl #2 > (gdb) info reg $r2 > r2 0x10007 65543 > """ > > GDB session with the bug fixed (patch applied): > > """ > $ gdb ./main-fixed-memset.elf -q > Reading symbols from ./main-fixed-memset.elf...done. > (gdb) x/i memset > 0x8770 <memset>: mov r3, r0 > (gdb) r > Starting program: /root/memset/main-fixed-memset.elf > > Program received signal SIGSEGV, Segmentation fault. > memset () at libc/string/arm/memset.S:92 > 92 libc/string/arm/memset.S: No such file or directory. > (gdb) x/i $pc > => 0x87b0 <memset+64>: stmia r3!, {r1, r12} > (gdb) info reg $r3 > r3 0x15000 86016 > (gdb) info proc mappings > process 5822 > Mapped address spaces: > > Start Addr End Addr Size Offset objfile > 0x8000 0xb000 0x3000 0x0 > /root/memset/main-fixed-memset.elf > 0x12000 0x15000 0x3000 0x2000 > /root/memset/main-fixed-memset.elf > 0xb6fff000 0xb7000000 0x1000 0x0 [sigpage] > 0xbefdf000 0xbf000000 0x21000 0x0 > 0xffff0000 0xffff1000 0x1000 0x0 [vectors] > (gdb) info reg $sp > sp 0x14d78 0x14d78 > """ > > GDB crashes inside the memset function, on the store instruction. This > time the crash is (as expected) because of a memory access imediately > after the memory region that contains the stack -- the buffer that's > being memset'd is allocated on the stack. > > Signed-off-by: Lucian Cojocar <lucian.cojocar@vu.nl> > --- > libc/string/arm/memset.S | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S > index 2be4850..412270f 100644 > --- a/libc/string/arm/memset.S > +++ b/libc/string/arm/memset.S > @@ -67,7 +67,7 @@ memset: > memset: > mov a4, a1 > cmp a3, $8 @ at least 8 bytes to do? > - blt 2f > + blo 2f > orr a2, a2, a2, lsl $8 > orr a2, a2, a2, lsl $16 > 1: > @@ -84,27 +84,27 @@ memset: > mov ip, a2 > 1: > cmp a3, $8 @ 8 bytes still to do? > - blt 2f > + blo 2f > stmia a4!, {a2, ip} > sub a3, a3, $8 > cmp a3, $8 @ 8 bytes still to do? > - blt 2f > + blo 2f > stmia a4!, {a2, ip} > sub a3, a3, $8 > cmp a3, $8 @ 8 bytes still to do? > - blt 2f > + blo 2f > stmia a4!, {a2, ip} > sub a3, a3, $8 > cmp a3, $8 @ 8 bytes still to do? > #if defined(__thumb2__) > - itt ge > - stmiage a4!, {a2, ip} > - subge a3, a3, $8 > + itt hs > + stmiahs a4!, {a2, ip} > + subhs a3, a3, $8 > #else > - stmgeia a4!, {a2, ip} > - subge a3, a3, $8 > + stmhsia a4!, {a2, ip} > + subhs a3, a3, $8 > #endif > - bge 1b > + bhs 1b > 2: > movs a3, a3 @ anything left? > IT(t, eq) >
Hi Lucian, Lucian Cojocar wrote, > Hi, > > Any follow-up on this patch? Not yet. You are saying the second segmentation fault would be expected. What is then the exact benefit of the patch, if the result is a segfault? Or do you have a simple testcase showing breakage before your patch and non-breakage after? best regards Waldemar
Waldemar Brodkorb <wbx <at> ucibc-ng.org> writes: > > Hi Lucian, > Lucian Cojocar wrote, > > > Hi, > > > > Any follow-up on this patch? > > Not yet. You are saying the second segmentation fault would be > expected. What is then the exact benefit of the patch, if the result > is a segfault? 'memset' will work on a range >= 2GB. > Or do you have a simple testcase showing breakage before your patch > and non-breakage after? Yes, I have a test which works in an environment where you can use more than 2GB contiguous virtual memory (e.g. have enough swap to accommodate 2GB contiguous virtual memory). """ #include <string.h> #include <stdio.h> #include <sys/mman.h> #define GB (size_t)(1*(1 << 30ul)) #define TWO_GB (2*(size_t)(GB)) #define S (size_t)(TWO_GB+4096) int main(void) { char *p = NULL; p = mmap(NULL, S, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0); if (p == MAP_FAILED) { perror("mmap"); exit(-1); } printf("&p[0] = %p\n", &p[0]); printf("&p[S] = %p\n", &p[S]); printf("memsetting ..."); memset(p, 0xaa, S); printf("done\n"); printf("p[S-1]=%02x\n", (unsigned char)p[S-1]); exit(0); } """ ~# ./main-buggy-memset.elf &p[0] = 0x36f66000 &p[S] = 0xb6f67000 Segmentation fault ~# ./main-fixed-memset.elf &p[0] = 0x36f8a000 &p[S] = 0xb6f8b000 memsetting ...done p[S-1]=aa ~# free -h total used free shared buffers cached Mem: 247M 56M 191M 252K 4.3M 33M -/+ buffers/cache: 19M 228M Swap: 10G 16M 10G
Hi Lucian, Lucian Cojocar wrote, > Waldemar Brodkorb <wbx <at> ucibc-ng.org> writes: > > > > > Hi Lucian, > > Lucian Cojocar wrote, > > > > > Hi, > > > > > > Any follow-up on this patch? > > > > Not yet. You are saying the second segmentation fault would be > > expected. What is then the exact benefit of the patch, if the result > > is a segfault? > > 'memset' will work on a range >= 2GB. > > > Or do you have a simple testcase showing breakage before your patch > > and non-breakage after? > > Yes, I have a test which works in an environment where you can use more than > 2GB contiguous virtual memory (e.g. have enough swap to accommodate 2GB > contiguous virtual memory). > > """ > #include <string.h> > #include <stdio.h> > #include <sys/mman.h> > > #define GB (size_t)(1*(1 << 30ul)) > #define TWO_GB (2*(size_t)(GB)) > #define S (size_t)(TWO_GB+4096) > > int > main(void) > { > char *p = NULL; > p = mmap(NULL, S, PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, > -1, 0); > if (p == MAP_FAILED) { > perror("mmap"); > exit(-1); > } > printf("&p[0] = %p\n", &p[0]); > printf("&p[S] = %p\n", &p[S]); > > printf("memsetting ..."); > memset(p, 0xaa, S); > printf("done\n"); > > printf("p[S-1]=%02x\n", (unsigned char)p[S-1]); > exit(0); > } > """ > > ~# ./main-buggy-memset.elf > &p[0] = 0x36f66000 > &p[S] = 0xb6f67000 > Segmentation fault > ~# ./main-fixed-memset.elf > &p[0] = 0x36f8a000 > &p[S] = 0xb6f8b000 > memsetting ...done > p[S-1]=aa > ~# free -h > total used free shared buffers cached > Mem: 247M 56M 191M 252K 4.3M 33M > -/+ buffers/cache: 19M 228M > Swap: 10G 16M 10G Applied and pushed, Thanks you, Waldemar
diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 2be4850..412270f 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -67,7 +67,7 @@ memset: memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? - blt 2f + blo 2f orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: @@ -84,27 +84,27 @@ memset: mov ip, a2 1: cmp a3, $8 @ 8 bytes still to do? - blt 2f + blo 2f stmia a4!, {a2, ip} sub a3, a3, $8 cmp a3, $8 @ 8 bytes still to do? - blt 2f + blo 2f stmia a4!, {a2, ip} sub a3, a3, $8 cmp a3, $8 @ 8 bytes still to do? - blt 2f + blo 2f stmia a4!, {a2, ip} sub a3, a3, $8 cmp a3, $8 @ 8 bytes still to do? #if defined(__thumb2__) - itt ge - stmiage a4!, {a2, ip} - subge a3, a3, $8 + itt hs + stmiahs a4!, {a2, ip} + subhs a3, a3, $8 #else - stmgeia a4!, {a2, ip} - subge a3, a3, $8 + stmhsia a4!, {a2, ip} + subhs a3, a3, $8 #endif - bge 1b + bhs 1b 2: movs a3, a3 @ anything left? IT(t, eq)
The 'BLT' instruction checks for *signed* values. So if a3, length parameter of memset, is negative, then value added to the PC will be large. memset(buf, 0xaa, 0xffff0000) triggers the bug. GDB session without the patch: """ $ gdb ./main-buggy-memset.elf -q Reading symbols from ./main-buggy-memset.elf...done. (gdb) x/i memset 0x8770 <memset>: mov r3, r0 (gdb) r Starting program: /root/memset/main-buggy-memset.elf Program received signal SIGSEGV, Segmentation fault. 0x00048808 in ?? () """ The $pc is outside of the memset function because: """ (gdb) x/i $pc => 0x87e4 <memset+116>: add pc, pc, r2, lsl #2 (gdb) info reg $r2 r2 0x10007 65543 """ GDB session with the bug fixed (patch applied): """ $ gdb ./main-fixed-memset.elf -q Reading symbols from ./main-fixed-memset.elf...done. (gdb) x/i memset 0x8770 <memset>: mov r3, r0 (gdb) r Starting program: /root/memset/main-fixed-memset.elf Program received signal SIGSEGV, Segmentation fault. memset () at libc/string/arm/memset.S:92 92 libc/string/arm/memset.S: No such file or directory. (gdb) x/i $pc => 0x87b0 <memset+64>: stmia r3!, {r1, r12} (gdb) info reg $r3 r3 0x15000 86016 (gdb) info proc mappings process 5822 Mapped address spaces: Start Addr End Addr Size Offset objfile 0x8000 0xb000 0x3000 0x0 /root/memset/main-fixed-memset.elf 0x12000 0x15000 0x3000 0x2000 /root/memset/main-fixed-memset.elf 0xb6fff000 0xb7000000 0x1000 0x0 [sigpage] 0xbefdf000 0xbf000000 0x21000 0x0 0xffff0000 0xffff1000 0x1000 0x0 [vectors] (gdb) info reg $sp sp 0x14d78 0x14d78 """ GDB crashes inside the memset function, on the store instruction. This time the crash is (as expected) because of a memory access imediately after the memory region that contains the stack -- the buffer that's being memset'd is allocated on the stack. Signed-off-by: Lucian Cojocar <lucian.cojocar@vu.nl> --- libc/string/arm/memset.S | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)