[PATCH] selftests/powerpc: Fix strncpy usage

[PATCH] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

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 <khandual@linux.vnet.ibm.com>
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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Segher Boessenkool]

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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

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:
>> -	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");

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Christophe LEROY]

Le 22/06/2018 à 16:43, Breno Leitao a écrit :
> 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:
>>> -	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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Segher Boessenkool]

On Fri, Jun 22, 2018 at 04:51:21PM +0200, Christophe LEROY wrote:
> Le 22/06/2018 à 16:43, Breno Leitao a écrit :
> >+               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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Paul Clarke]

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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Al Dunsmuir]

On Friday, June 22, 2018, 11:15:29 AM, Paul Clarke wrote:
> 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".

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]);

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Segher Boessenkool]

Hi!

On Fri, Jun 22, 2018 at 11:43:44AM -0300, Breno Leitao wrote:
> On 06/21/2018 08:18 PM, Segher Boessenkool wrote:
> > 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 ;-)
> 
> 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

Re: [PATCH] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

hi Segher,

On 06/22/2018 10:10 PM, Segher Boessenkool wrote:
>> -       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

[PATCH v2] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

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]);
+
 	return test_harness(dscr_inherit_exec, "dscr_inherit_exec_test");
 }
-- 
2.16.3

Re: [PATCH v2] selftests/powerpc: Fix strncpy usage

[Michael Ellerman]

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

Re: [PATCH v2] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

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!

[PATCH v3 1/2] selftests/powerpc: Fix strncpy usage

[Breno Leitao]

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 <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 | 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

[PATCH v3 2/2] selftests/powerpc: Fix typos

[Breno Leitao]

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 <gromero@linux.vnet.ibm.com>
---
 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

Re: [v3,1/2] selftests/powerpc: Fix strncpy usage

[Michael Ellerman]

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 <segher@kernel.crashing.org>
> CC: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>

Series applied to powerpc next, thanks.

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

cheers