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@xxxxxxxxxx>
> >> Cc: Jonathan Toppins <jtoppins@xxxxxxxxxx>
> >> Cc: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
> >> Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> >> Cc: kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> >> ---
> >> Â 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@xxxxxxxxxx>


Daniel.