[PATCH] selftests/powerpc: Fix strncpy usage

There is a buffer overflow in dscr_inherit_test.c test. In main(), strncpy()'s
third argument is the lengh 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 simply limit the string copy to sizeof(prog) less 1 (space for \0).

CC: Anshuman Khandual <redacted>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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..638e0dc717d5 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c
+++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c

@@ -104,6 +104,6 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	strncpy(prog, argv[0], strlen(argv[0]));
+	strncpy(prog, argv[0], sizeof(prog) - 1);
 	return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");
 }

-- 
2.17.1

On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote:

-	strncpy(prog, argv[0], strlen(argv[0]));
+	strncpy(prog, argv[0], sizeof(prog) - 1);

	strncpy(prog, argv[0], sizeof prog);
	if (prog[sizeof prog - 1])
		scream_bloody_murder();

Silently using the wrong data is a worse habit than not checking for
overflows ;-)


Segher

Hi Segher,

On 06/21/2018 08:18 PM, Segher Boessenkool wrote:

On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote:

quoted

-	strncpy(prog, argv[0], strlen(argv[0]));
+	strncpy(prog, argv[0], sizeof(prog) - 1);

	strncpy(prog, argv[0], sizeof prog);
	if (prog[sizeof prog - 1])
		scream_bloody_murder();

Silently using the wrong data is a worse habit than not checking for
overflows ;-)

Completely agree! Thanks for bringing this up.

If you don't mind, I would solve this problem slightly different, as it seems
to be more readable.


-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);
+               return 1;
+       }
+
+       strncpy(prog, argv[0], sizeof(prog) - 1);
        return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");

Le 22/06/2018 à 16:43, Breno Leitao a écrit :

Hi Segher,

On 06/21/2018 08:18 PM, Segher Boessenkool wrote:

quoted

On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote:

quoted

-	strncpy(prog, argv[0], strlen(argv[0]));
+	strncpy(prog, argv[0], sizeof(prog) - 1);

	strncpy(prog, argv[0], sizeof prog);
	if (prog[sizeof prog - 1])
		scream_bloody_murder();

Silently using the wrong data is a worse habit than not checking for
overflows ;-)

Completely agree! Thanks for bringing this up.

If you don't mind, I would solve this problem slightly different, as it seems
to be more readable.


-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){

wouldn't it be better to use sizeof(prog) instead of LEN_MAX ?

+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);
+               return 1;
+       }
+
+       strncpy(prog, argv[0], sizeof(prog) - 1);

You have checked before that argv[0] is not too long, so you should not 
need to use strncpy(), strcpy() would do it.

         return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");

Christophe

On 06/22/2018 09:43 AM, Breno Leitao wrote:

If you don't mind, I would solve this problem slightly different, as it seems
to be more readable.

-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);

"Very big" is an observation.  "Too big" indicates a problem better.  Or, more explicitly "Executable name is too long".

PC

On Friday, June 22, 2018, 11:15:29 AM, Paul Clarke wrote:

On 06/22/2018 09:43 AM, Breno Leitao wrote:

quoted

If you don't mind, I would solve this problem slightly different, as it seems
to be more readable.

-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);

"Very big" is an observation.  "Too big" indicates a problem
better.  Or, more explicitly "Executable name is too long".

Or even better, display the limit that is being exceeded, in case that
value changes over time.  Something like.

-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+                fprintf(stderr, "Executable name exceeds limit (%d): %s\n",
+                        LEN_MAX,
+                        argv[0]);

On Fri, Jun 22, 2018 at 04:51:21PM +0200, Christophe LEROY wrote:

Le 22/06/2018 à 16:43, Breno Leitao a écrit :

quoted

+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);
+               return 1;
+       }
+
+       strncpy(prog, argv[0], sizeof(prog) - 1);

You have checked before that argv[0] is not too long, so you should not 
need to use strncpy(), strcpy() would do it.

If you don't care about the bytes of prog after the first zero byte, sure.


Segher

Hi!

On Fri, Jun 22, 2018 at 11:43:44AM -0300, Breno Leitao wrote:

On 06/21/2018 08:18 PM, Segher Boessenkool wrote:

quoted

On Wed, Jun 20, 2018 at 07:51:11PM -0300, Breno Leitao wrote:

quoted

-	strncpy(prog, argv[0], strlen(argv[0]));
+	strncpy(prog, argv[0], sizeof(prog) - 1);

	strncpy(prog, argv[0], sizeof prog);
	if (prog[sizeof prog - 1])
		scream_bloody_murder();

Silently using the wrong data is a worse habit than not checking for
overflows ;-)

Completely agree! Thanks for bringing this up.

If you don't mind, I would solve this problem slightly different, as it seems
to be more readable.

-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);
+               return 1;
+       }
+
+       strncpy(prog, argv[0], sizeof(prog) - 1);

The strlen reads all of argv[0], which can be very big in theory.  It won't
matter in this test file -- program arguments cannot be super long, for one
thing -- but it's not a good idea in general (that is one of the problems
of strlcpy, btw).

Best of course is to avoid string length restrictions completely, if you can.


Segher

hi Segher,

On 06/22/2018 10:10 PM, Segher Boessenkool wrote:

quoted

-       strncpy(prog, argv[0], strlen(argv[0]));
+       if (strlen(argv[0]) >= LEN_MAX){
+               fprintf(stderr, "Very big executable name: %s\n", argv[0]);
+               return 1;
+       }
+
+       strncpy(prog, argv[0], sizeof(prog) - 1);

The strlen reads all of argv[0], which can be very big in theory.  It won't
matter in this test file -- program arguments cannot be super long, for one
thing -- but it's not a good idea in general (that is one of the problems
of strlcpy, btw).

Best of course is to avoid string length restrictions completely, if you can.

Right, I was thinking about this problem and there is no motivation to have a
statically allocated and limited region.

I will send a v2 where 'prog' and avoid this restriction completely.

Thanks

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 <redacted>
CC: Anshuman Khandual <redacted>
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]);
+
 	return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");
 }

-- 
2.16.3

Breno Leitao [off-list ref] writes:

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 [off-list ref] writes:

quoted

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 <redacted>
CC: Anshuman Khandual <redacted>
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!

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 maps 'prog' to the argv[0] memory region, removing the static
allocation and the LEN_MAX size restriction.

CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Segher Boessenkool <redacted>
CC: Anshuman Khandual <redacted>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 4 ++--
 1 file changed, 2 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..55c55f39b6a6 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,6 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	strncpy(prog, argv[0], strlen(argv[0]));
+	prog = argv[0];
 	return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");
 }

-- 
2.16.3

Fix two typos in the file header. Replacing the word 'priviledged'
by 'privileged' and 'exuecuted' by 'executed'.

Signed-off-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Gustavo Romero <redacted>
---
 tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c | 4 ++--
 1 file changed, 2 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 55c55f39b6a6..c8c240accc0c 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c
+++ b/tools/testing/selftests/powerpc/dscr/dscr_inherit_exec_test.c

@@ -5,8 +5,8 @@
  * verifies that the child is using the changed DSCR using mfspr.
  *
  * When using the privilege state SPR, the instructions such as
- * mfspr or mtspr are priviledged and the kernel emulates them
- * for us. Instructions using problem state SPR can be exuecuted
+ * mfspr or mtspr are privileged and the kernel emulates them
+ * for us. Instructions using problem state SPR can be executed
  * directly without any emulation if the HW supports them. Else
  * they also get emulated by the kernel.
  *

-- 
2.16.3

On Tue, 2018-06-26 at 13:20:12 UTC, Breno Leitao wrote:

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 maps 'prog' to the argv[0] memory region, removing the static
allocation and the LEN_MAX size restriction.

CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Segher Boessenkool <redacted>
CC: Anshuman Khandual <redacted>
Signed-off-by: Breno Leitao <leitao@debian.org>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/09a61e894ac852fb063ee0b54fc513

cheers