| Message ID | 1529962249-24774-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] selftests/powerpc: Fix strncpy usage | expand |
Breno Leitao <leitao@debian.org> writes: > There is a buffer overflow in dscr_inherit_test.c test. In main(), strncpy()'s > third argument is the length of the source, not the size of the destination > buffer, which makes strncpy() behaves like strcpy(), causing a buffer overflow > if argv[0] is bigger than LEN_MAX (100). > > This patch allocates 'prog' according to the argv[0] length, avoiding LEN_MAX > restriction. > > CC: Segher Boessenkool <segher@kernel.crashing.org> > CC: Anshuman Khandual <khandual@linux.vnet.ibm.com> > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c > index 08a8b95e3bc1..ecac4900c7dd 100644 > --- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c > +++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c > @@ -19,7 +19,7 @@ > */ > #include "dscr.h" > > -static char prog[LEN_MAX]; > +static char *prog; > > static void do_exec(unsigned long parent_dscr) > { > @@ -104,6 +104,13 @@ int main(int argc, char *argv[]) > exit(1); > } > > - strncpy(prog, argv[0], strlen(argv[0])); > + prog = malloc(strlen(argv[0]) + 1); > + if (prog == NULL) { > + fprintf(stderr, "Unable to allocate enough memory\n"); > + exit(1); > + } > + > + strcpy(prog, argv[0]); Why do we need to copy it at all? Can't we just save a pointer it? ie, prog = argv[0]; What am I missing? cheers
On 06/26/2018 02:24 AM, Michael Ellerman wrote: > Breno Leitao <leitao@debian.org> writes: > >> There is a buffer overflow in dscr_inherit_test.c test. In main(), strncpy()'s >> third argument is the length of the source, not the size of the destination >> buffer, which makes strncpy() behaves like strcpy(), causing a buffer overflow >> if argv[0] is bigger than LEN_MAX (100). >> >> This patch allocates 'prog' according to the argv[0] length, avoiding LEN_MAX >> restriction. >> >> CC: Segher Boessenkool <segher@kernel.crashing.org> >> CC: Anshuman Khandual <khandual@linux.vnet.ibm.com> >> Signed-off-by: Breno Leitao <leitao@debian.org> >> --- >> tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c >> index 08a8b95e3bc1..ecac4900c7dd 100644 >> --- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c >> +++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c >> @@ -19,7 +19,7 @@ >> */ >> #include "dscr.h" >> >> -static char prog[LEN_MAX]; >> +static char *prog; >> >> static void do_exec(unsigned long parent_dscr) >> { >> @@ -104,6 +104,13 @@ int main(int argc, char *argv[]) >> exit(1); >> } >> >> - strncpy(prog, argv[0], strlen(argv[0])); >> + prog = malloc(strlen(argv[0]) + 1); >> + if (prog == NULL) { >> + fprintf(stderr, "Unable to allocate enough memory\n"); >> + exit(1); >> + } >> + >> + strcpy(prog, argv[0]); > > Why do we need to copy it at all? We do not. Pointing proj to argv[0], as you proposed, should be the best solution for this problem. Thanks!
diff --git a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c index 08a8b95e3bc1..ecac4900c7dd 100644 --- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c +++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c @@ -19,7 +19,7 @@ */ #include "dscr.h" -static char prog[LEN_MAX]; +static char *prog; static void do_exec(unsigned long parent_dscr) { @@ -104,6 +104,13 @@ int main(int argc, char *argv[]) exit(1); } - strncpy(prog, argv[0], strlen(argv[0])); + prog = malloc(strlen(argv[0]) + 1); + if (prog == NULL) { + fprintf(stderr, "Unable to allocate enough memory\n"); + exit(1); + } + + strcpy(prog, argv[0]); + return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test"); }
There is a buffer overflow in dscr_inherit_test.c test. In main(), strncpy()'s third argument is the length of the source, not the size of the destination buffer, which makes strncpy() behaves like strcpy(), causing a buffer overflow if argv[0] is bigger than LEN_MAX (100). This patch allocates 'prog' according to the argv[0] length, avoiding LEN_MAX restriction. CC: Segher Boessenkool <segher@kernel.crashing.org> CC: Anshuman Khandual <khandual@linux.vnet.ibm.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)