Messages in this thread | | | Subject | Re: [PATCH v2] kdb: Use strscpy with destination buffer size | From | Prarit Bhargava <> | Date | Thu, 18 Oct 2018 08:15:04 -0400 |
| |
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. >
| |