LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Nick Desaulniers <nick.desaulniers@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Jason Wessel <jason.wessel@windriver.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Baolin Wang <baolin.wang@linaro.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	kgdb-bugreport@lists.sourceforge.net,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ebiggers@google.com
Subject: Re: [PATCH] kdb: prefer strlcpy to strncpy
Date: Wed, 30 May 2018 15:34:15 +0100	[thread overview]
Message-ID: <20180530143415.ksc6fb4zo6m7xb25@holly.lan> (raw)
In-Reply-To: <CAH7mPvhQo5S5EbgvkteoL1xftLWjWBqkfoanG0dTLzaa=FtTgw@mail.gmail.com>

On Tue, May 29, 2018 at 07:01:35PM -0700, Nick Desaulniers wrote:
> On Tue, May 29, 2018 at 12:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tue, May 29, 2018 at 7:57 AM, Nick Desaulniers
> > <nick.desaulniers@gmail.com> wrote:
> >> Fixes stringop-truncation and stringop-overflow warnings from gcc-8.
> >
> > That patch description should really explain whether gcc is right or not. What's
> > the worst thing that could happen here?
> >
> > I would also recommend citing the exact warning you got.
> >
> >> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> >> index ed5d349..b5dfff1 100644
> >> --- a/kernel/debug/kdb/kdb_io.c
> >> +++ b/kernel/debug/kdb/kdb_io.c
> >> @@ -443,7 +443,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >>  char *kdb_getstr(char *buffer, size_t bufsize, const char *prompt)
> >>  {
> >>         if (prompt && kdb_prompt_str != prompt)
> >> -               strncpy(kdb_prompt_str, prompt, CMD_BUFLEN);
> >> +               strlcpy(kdb_prompt_str, prompt, CMD_BUFLEN);
> >>         kdb_printf(kdb_prompt_str);
> >>         kdb_nextline = 1;       /* Prompt and input resets line number */
> >>         return kdb_read(buffer, bufsize);
> >> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> >> index e405677..c30a0d8 100644
> >> --- a/kernel/debug/kdb/kdb_main.c
> >> +++ b/kernel/debug/kdb/kdb_main.c
> >> @@ -1103,12 +1103,12 @@ static int handle_ctrl_cmd(char *cmd)
> >>         case CTRL_P:
> >>                 if (cmdptr != cmd_tail)
> >>                         cmdptr = (cmdptr-1) % KDB_CMD_HISTORY_COUNT;
> >> -               strncpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> >> +               strlcpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> >>                 return 1;
> >>         case CTRL_N:
> >>                 if (cmdptr != cmd_head)
> >>                         cmdptr = (cmdptr+1) % KDB_CMD_HISTORY_COUNT;
> >> -               strncpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> >> +               strlcpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> >>                 return 1;
> >>         }
> >>         return 0;
> >
> > Those three all look good.
> >
> >> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> >> index 990b3cc..dcfbf8f 100644
> >> --- a/kernel/debug/kdb/kdb_support.c
> >> +++ b/kernel/debug/kdb/kdb_support.c
> >> @@ -236,7 +236,7 @@ int kallsyms_symbol_next(char *prefix_name, int flag)
> >>
> >>         while ((name = kdb_walk_kallsyms(&pos))) {
> >>                 if (strncmp(name, prefix_name, prefix_len) == 0) {
> >> -                       strncpy(prefix_name, name, strlen(name)+1);
> >> +                       strlcpy(prefix_name, name, prefix_len);
> >>                         return 1;
> >>                 }
> >
> > I don't know what this does, but you are changing the behavior: the previous
> > 'strlen(name)+1' argument was the size of the source string (which makes
> > the strncpy() behave the same as a plain strcpy()), the new one means
> > we only copy at most as many bytes as the previous length of the destination
> > string.
> >
> > Is that intended? If yes, better explain it in the patch description.
> >
> >         Arnd
> 
> Eric points out that this will leak kernel memory if size is less than
> sizeof src.

Don't quite understand what this means (there's no allocation here, how
can there be a leak?) but the symbol completion certainly won't work if
we truncate the copy here.

My understanding is that the only way to make this overflow safe is to
change the signature of kallsyms_symbol_next() so it takes a max_len
argument similar to what is done for kallsyms_symbol_complete().

It might even be worth using strscpy() here and propagating the -E2BIG
to the caller. That allows the caller to print the partial symbol and
an elipsis to show the user that the symbol has been truncated.


Daniel.

  reply	other threads:[~2018-05-30 14:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-29  5:57 Nick Desaulniers
2018-05-29  7:57 ` Arnd Bergmann
2018-05-30  2:01   ` Nick Desaulniers
2018-05-30 14:34     ` Daniel Thompson [this message]
2018-05-30 20:47       ` Geert Uytterhoeven
2018-05-31  8:24         ` Daniel Thompson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180530143415.ksc6fb4zo6m7xb25@holly.lan \
    --to=daniel.thompson@linaro.org \
    --cc=arnd@arndb.de \
    --cc=baolin.wang@linaro.org \
    --cc=ebiederm@xmission.com \
    --cc=ebiggers@google.com \
    --cc=jason.wessel@windriver.com \
    --cc=kgdb-bugreport@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nick.desaulniers@gmail.com \
    --cc=rdunlap@infradead.org \
    --subject='Re: [PATCH] kdb: prefer strlcpy to strncpy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).