[PATCH v2] kdb: Use strscpy with destination buffer size

[PATCH v2] kdb: Use strscpy with destination buffer size

[Prarit Bhargava]

gcc 8.1.0 warns with:

kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
     strncpy(prefix_name, name, strlen(name)+1);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/debug/kdb/kdb_support.c:239:31: note: length computed here

Use strscpy() with the destination buffer size, and use ellipses when
displaying truncated symbols.

v2: Use strscpy()

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Jonathan Toppins <jtoppins@redhat.com>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>
Cc: kgdb-bugreport@lists.sourceforge.net
---
 kernel/debug/kdb/kdb_io.c      | 15 +++++++++------
 kernel/debug/kdb/kdb_private.h |  2 +-
 kernel/debug/kdb/kdb_support.c | 10 +++++-----
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index ed5d34925ad0..6a4b41484afe 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
 	int count;
 	int i;
 	int diag, dtab_count;
-	int key;
+	int key, buf_size, ret;
 
 
 	diag = kdbgetintenv("DTABCOUNT", &dtab_count);
@@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
 		else
 			p_tmp = tmpbuffer;
 		len = strlen(p_tmp);
-		count = kallsyms_symbol_complete(p_tmp,
-						 sizeof(tmpbuffer) -
-						 (p_tmp - tmpbuffer));
+		buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
+		count = kallsyms_symbol_complete(p_tmp, buf_size);
 		if (tab == 2 && count > 0) {
 			kdb_printf("\n%d symbols are found.", count);
 			if (count > dtab_count) {
@@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
 			}
 			kdb_printf("\n");
 			for (i = 0; i < count; i++) {
-				if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
+				ret = kallsyms_symbol_next(p_tmp, i, buf_size);
+				if (WARN_ON(!ret))
 					break;
-				kdb_printf("%s ", p_tmp);
+				if (ret != -E2BIG)
+					kdb_printf("%s ", p_tmp);
+				else
+					kdb_printf("%s... ", p_tmp);
 				*(p_tmp + len) = '\0';
 			}
 			if (i >= dtab_count)
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 1e5a502ba4a7..2118d8258b7c 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -83,7 +83,7 @@ typedef struct __ksymtab {
 		unsigned long sym_start;
 		unsigned long sym_end;
 		} kdb_symtab_t;
-extern int kallsyms_symbol_next(char *prefix_name, int flag);
+extern int kallsyms_symbol_next(char *prefix_name, int flag, int buf_size);
 extern int kallsyms_symbol_complete(char *prefix_name, int max_len);
 
 /* Exported Symbols for kernel loadable modules to use. */
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 990b3cc526c8..61cd704a21c8 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -221,11 +221,13 @@ int kallsyms_symbol_complete(char *prefix_name, int max_len)
  * Parameters:
  *	prefix_name	prefix of a symbol name to lookup
  *	flag	0 means search from the head, 1 means continue search.
+ *	buf_size	maximum length that can be written to prefix_name
+ *			buffer
  * Returns:
  *	1 if a symbol matches the given prefix.
  *	0 if no string found
  */
-int kallsyms_symbol_next(char *prefix_name, int flag)
+int kallsyms_symbol_next(char *prefix_name, int flag, int buf_size)
 {
 	int prefix_len = strlen(prefix_name);
 	static loff_t pos;
@@ -235,10 +237,8 @@ int kallsyms_symbol_next(char *prefix_name, int flag)
 		pos = 0;
 
 	while ((name = kdb_walk_kallsyms(&pos))) {
-		if (strncmp(name, prefix_name, prefix_len) == 0) {
-			strncpy(prefix_name, name, strlen(name)+1);
-			return 1;
-		}
+		if (!strncmp(name, prefix_name, prefix_len))
+			return strscpy(prefix_name, name, buf_size);
 	}
 	return 0;
 }
-- 
2.14.4

Re: [PATCH v2] kdb: Use strscpy with destination buffer size

[Daniel Thompson]

On 20/09/2018 13:59, Prarit Bhargava wrote:
> gcc 8.1.0 warns with:
> 
> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
>       strncpy(prefix_name, name, strlen(name)+1);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
> 
> Use strscpy() with the destination buffer size, and use ellipses when
> displaying truncated symbols.
> 
> v2: Use strscpy()
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Jonathan Toppins <jtoppins@redhat.com>
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: kgdb-bugreport@lists.sourceforge.net
> ---
>   kernel/debug/kdb/kdb_io.c      | 15 +++++++++------
>   kernel/debug/kdb/kdb_private.h |  2 +-
>   kernel/debug/kdb/kdb_support.c | 10 +++++-----
>   3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index ed5d34925ad0..6a4b41484afe 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>   	int count;
>   	int i;
>   	int diag, dtab_count;
> -	int key;
> +	int key, buf_size, ret;
>   
>   
>   	diag = kdbgetintenv("DTABCOUNT", &dtab_count);
> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>   		else
>   			p_tmp = tmpbuffer;
>   		len = strlen(p_tmp);
> -		count = kallsyms_symbol_complete(p_tmp,
> -						 sizeof(tmpbuffer) -
> -						 (p_tmp - tmpbuffer));
> +		buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
> +		count = kallsyms_symbol_complete(p_tmp, buf_size);
>   		if (tab == 2 && count > 0) {
>   			kdb_printf("\n%d symbols are found.", count);
>   			if (count > dtab_count) {
> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
>   			}
>   			kdb_printf("\n");
>   			for (i = 0; i < count; i++) {
> -				if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
> +				ret = kallsyms_symbol_next(p_tmp, i, buf_size);
> +				if (WARN_ON(!ret))
>   					break;
I'm getting confused by having two different branches on ret.

Don't get a WARN_ON() when ret == -E2BIG?


Daniel.


> -				kdb_printf("%s ", p_tmp);
> +				if (ret != -E2BIG)
> +					kdb_printf("%s ", p_tmp);
> +				else
> +					kdb_printf("%s... ", p_tmp);
>   				*(p_tmp + len) = '\0';
>   			}
>   			if (i >= dtab_count)

Re: [PATCH v2] kdb: Use strscpy with destination buffer size

[Prarit Bhargava]

On 10/02/2018 11:53 AM, Daniel Thompson wrote:
> On 20/09/2018 13:59, Prarit Bhargava wrote:
>> gcc 8.1.0 warns with:
>>
>> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
>> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
>> depends on the length of the source argument [-Wstringop-overflow=]
>>       strncpy(prefix_name, name, strlen(name)+1);
>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>>
>> Use strscpy() with the destination buffer size, and use ellipses when
>> displaying truncated symbols.
>>
>> v2: Use strscpy()
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: Jonathan Toppins <jtoppins@redhat.com>
>> Cc: Jason Wessel <jason.wessel@windriver.com>
>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>> Cc: kgdb-bugreport@lists.sourceforge.net
>> ---
>>   kernel/debug/kdb/kdb_io.c      | 15 +++++++++------
>>   kernel/debug/kdb/kdb_private.h |  2 +-
>>   kernel/debug/kdb/kdb_support.c | 10 +++++-----
>>   3 files changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>> index ed5d34925ad0..6a4b41484afe 100644
>> --- a/kernel/debug/kdb/kdb_io.c
>> +++ b/kernel/debug/kdb/kdb_io.c
>> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>       int count;
>>       int i;
>>       int diag, dtab_count;
>> -    int key;
>> +    int key, buf_size, ret;
>>           diag = kdbgetintenv("DTABCOUNT", &dtab_count);
>> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>           else
>>               p_tmp = tmpbuffer;
>>           len = strlen(p_tmp);
>> -        count = kallsyms_symbol_complete(p_tmp,
>> -                         sizeof(tmpbuffer) -
>> -                         (p_tmp - tmpbuffer));
>> +        buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
>> +        count = kallsyms_symbol_complete(p_tmp, buf_size);
>>           if (tab == 2 && count > 0) {
>>               kdb_printf("\n%d symbols are found.", count);
>>               if (count > dtab_count) {
>> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>               }
>>               kdb_printf("\n");
>>               for (i = 0; i < count; i++) {
>> -                if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
>> +                ret = kallsyms_symbol_next(p_tmp, i, buf_size);
>> +                if (WARN_ON(!ret))
>>                       break;
> I'm getting confused by having two different branches on ret.
> 
> Don't get a WARN_ON() when ret == -E2BIG?
> 
> 

Should we WARN on a really long symbol?  I don't think we should as we're
handling that by truncating the output and adding ellipses below.

P.

> Daniel.
> 
> 
>> -                kdb_printf("%s ", p_tmp);
>> +                if (ret != -E2BIG)
>> +                    kdb_printf("%s ", p_tmp);
>> +                else
>> +                    kdb_printf("%s... ", p_tmp);
>>                   *(p_tmp + len) = '\0';
>>               }
>>               if (i >= dtab_count)

Re: [PATCH v2] kdb: Use strscpy with destination buffer size

[Daniel Thompson]

On Thu, Oct 04, 2018 at 08:25:30AM -0400, Prarit Bhargava wrote:
> On 10/02/2018 11:53 AM, Daniel Thompson wrote:
> > On 20/09/2018 13:59, Prarit Bhargava wrote:
> >> gcc 8.1.0 warns with:
> >>
> >> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
> >> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
> >> depends on the length of the source argument [-Wstringop-overflow=]
> >>       strncpy(prefix_name, name, strlen(name)+1);
> >>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
> >>
> >> Use strscpy() with the destination buffer size, and use ellipses when
> >> displaying truncated symbols.
> >>
> >> v2: Use strscpy()
> >>
> >> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> >> Cc: Jonathan Toppins <jtoppins@redhat.com>
> >> Cc: Jason Wessel <jason.wessel@windriver.com>
> >> Cc: Daniel Thompson <daniel.thompson@linaro.org>
> >> Cc: kgdb-bugreport@lists.sourceforge.net
> >> ---
> >>   kernel/debug/kdb/kdb_io.c      | 15 +++++++++------
> >>   kernel/debug/kdb/kdb_private.h |  2 +-
> >>   kernel/debug/kdb/kdb_support.c | 10 +++++-----
> >>   3 files changed, 15 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> >> index ed5d34925ad0..6a4b41484afe 100644
> >> --- a/kernel/debug/kdb/kdb_io.c
> >> +++ b/kernel/debug/kdb/kdb_io.c
> >> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >>       int count;
> >>       int i;
> >>       int diag, dtab_count;
> >> -    int key;
> >> +    int key, buf_size, ret;
> >>           diag = kdbgetintenv("DTABCOUNT", &dtab_count);
> >> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >>           else
> >>               p_tmp = tmpbuffer;
> >>           len = strlen(p_tmp);
> >> -        count = kallsyms_symbol_complete(p_tmp,
> >> -                         sizeof(tmpbuffer) -
> >> -                         (p_tmp - tmpbuffer));
> >> +        buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
> >> +        count = kallsyms_symbol_complete(p_tmp, buf_size);
> >>           if (tab == 2 && count > 0) {
> >>               kdb_printf("\n%d symbols are found.", count);
> >>               if (count > dtab_count) {
> >> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >>               }
> >>               kdb_printf("\n");
> >>               for (i = 0; i < count; i++) {
> >> -                if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
> >> +                ret = kallsyms_symbol_next(p_tmp, i, buf_size);
> >> +                if (WARN_ON(!ret))
> >>                       break;
> > I'm getting confused by having two different branches on ret.
> > 
> > Don't get a WARN_ON() when ret == -E2BIG?
> > 
> > 
> 
> Should we WARN on a really long symbol?  I don't think we should as we're
> handling that by truncating the output and adding ellipses below.

It's OK. You describe the behaviour I expect but I was misreading the
code (not realizing that kallsyms_symbol_next() had not become a 0 on
success success function).

However after reviewing the code (properly this time) I wonder if the
WARN_ON() should be improved to match the return value for the function:

  WARN_ON(ret >= 0 && ret < len)

That said, checking the symbol length is pretty paranoid and getting
close to nitpicking so I'll leave it up to you and, with or without, the
change:

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH v2] kdb: Use strscpy with destination buffer size

[Prarit Bhargava]

On 10/04/2018 08:45 AM, Daniel Thompson wrote:
> On Thu, Oct 04, 2018 at 08:25:30AM -0400, Prarit Bhargava wrote:
>> On 10/02/2018 11:53 AM, Daniel Thompson wrote:
>>> On 20/09/2018 13:59, Prarit Bhargava wrote:
>>>> gcc 8.1.0 warns with:
>>>>
>>>> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
>>>> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
>>>> depends on the length of the source argument [-Wstringop-overflow=]
>>>>       strncpy(prefix_name, name, strlen(name)+1);
>>>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>>>>
>>>> Use strscpy() with the destination buffer size, and use ellipses when
>>>> displaying truncated symbols.
>>>>
>>>> v2: Use strscpy()
>>>>
>>>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>>> Cc: Jonathan Toppins <jtoppins@redhat.com>
>>>> Cc: Jason Wessel <jason.wessel@windriver.com>
>>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>>> Cc: kgdb-bugreport@lists.sourceforge.net
>>>> ---
>>>>   kernel/debug/kdb/kdb_io.c      | 15 +++++++++------
>>>>   kernel/debug/kdb/kdb_private.h |  2 +-
>>>>   kernel/debug/kdb/kdb_support.c | 10 +++++-----
>>>>   3 files changed, 15 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>>>> index ed5d34925ad0..6a4b41484afe 100644
>>>> --- a/kernel/debug/kdb/kdb_io.c
>>>> +++ b/kernel/debug/kdb/kdb_io.c
>>>> @@ -216,7 +216,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>>       int count;
>>>>       int i;
>>>>       int diag, dtab_count;
>>>> -    int key;
>>>> +    int key, buf_size, ret;
>>>>           diag = kdbgetintenv("DTABCOUNT", &dtab_count);
>>>> @@ -336,9 +336,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>>           else
>>>>               p_tmp = tmpbuffer;
>>>>           len = strlen(p_tmp);
>>>> -        count = kallsyms_symbol_complete(p_tmp,
>>>> -                         sizeof(tmpbuffer) -
>>>> -                         (p_tmp - tmpbuffer));
>>>> +        buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
>>>> +        count = kallsyms_symbol_complete(p_tmp, buf_size);
>>>>           if (tab == 2 && count > 0) {
>>>>               kdb_printf("\n%d symbols are found.", count);
>>>>               if (count > dtab_count) {
>>>> @@ -350,9 +349,13 @@ static char *kdb_read(char *buffer, size_t bufsize)
>>>>               }
>>>>               kdb_printf("\n");
>>>>               for (i = 0; i < count; i++) {
>>>> -                if (WARN_ON(!kallsyms_symbol_next(p_tmp, i)))
>>>> +                ret = kallsyms_symbol_next(p_tmp, i, buf_size);
>>>> +                if (WARN_ON(!ret))
>>>>                       break;
>>> I'm getting confused by having two different branches on ret.
>>>
>>> Don't get a WARN_ON() when ret == -E2BIG?
>>>
>>>
>>
>> Should we WARN on a really long symbol?  I don't think we should as we're
>> handling that by truncating the output and adding ellipses below.
> 
> It's OK. You describe the behaviour I expect but I was misreading the
> code (not realizing that kallsyms_symbol_next() had not become a 0 on
> success success function).
> 
> However after reviewing the code (properly this time) I wonder if the
> WARN_ON() should be improved to match the return value for the function:
> 
>   WARN_ON(ret >= 0 && ret < len)
> 
> That said, checking the symbol length is pretty paranoid and getting
> close to nitpicking so I'll leave it up to you and, with or without, the
> change:
> 
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> 
> 

I think we should leave it as is for now.  OOC was this patch picked up by
anyone's repo for staging for 4.20?

P.

> Daniel.
>