* [PATCH] kdb: prefer strlcpy to strncpy
@ 2018-05-29 5:57 Nick Desaulniers
2018-05-29 7:57 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2018-05-29 5:57 UTC (permalink / raw)
To: jason.wessel, daniel.thompson
Cc: Nick Desaulniers, Arnd Bergmann, Randy Dunlap, Baolin Wang,
Eric W. Biederman, kgdb-bugreport, linux-kernel
Fixes stringop-truncation and stringop-overflow warnings from gcc-8.
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
kernel/debug/kdb/kdb_io.c | 2 +-
kernel/debug/kdb/kdb_main.c | 4 ++--
kernel/debug/kdb/kdb_support.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
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;
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;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kdb: prefer strlcpy to strncpy
2018-05-29 5:57 [PATCH] kdb: prefer strlcpy to strncpy Nick Desaulniers
@ 2018-05-29 7:57 ` Arnd Bergmann
2018-05-30 2:01 ` Nick Desaulniers
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2018-05-29 7:57 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Jason Wessel, Daniel Thompson, Randy Dunlap, Baolin Wang,
Eric W. Biederman, kgdb-bugreport, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kdb: prefer strlcpy to strncpy
2018-05-29 7:57 ` Arnd Bergmann
@ 2018-05-30 2:01 ` Nick Desaulniers
2018-05-30 14:34 ` Daniel Thompson
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2018-05-30 2:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jason Wessel, Daniel Thompson, Randy Dunlap, Baolin Wang,
Eric W. Biederman, kgdb-bugreport, Linux Kernel Mailing List,
ebiggers
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kdb: prefer strlcpy to strncpy
2018-05-30 2:01 ` Nick Desaulniers
@ 2018-05-30 14:34 ` Daniel Thompson
2018-05-30 20:47 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Thompson @ 2018-05-30 14:34 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Arnd Bergmann, Jason Wessel, Randy Dunlap, Baolin Wang,
Eric W. Biederman, kgdb-bugreport, Linux Kernel Mailing List,
ebiggers
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kdb: prefer strlcpy to strncpy
2018-05-30 14:34 ` Daniel Thompson
@ 2018-05-30 20:47 ` Geert Uytterhoeven
2018-05-31 8:24 ` Daniel Thompson
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2018-05-30 20:47 UTC (permalink / raw)
To: Daniel Thompson
Cc: Nick Desaulniers, Arnd Bergmann, Jason Wessel, Randy Dunlap,
Baolin Wang, Eric W. Biederman, kgdb-bugreport,
Linux Kernel Mailing List, ebiggers
H Daniel,
On Wed, May 30, 2018 at 4:34 PM, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> 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.
>> 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.
Not leak an is memory leak, but leak as in information leak of uninitialized
data to userspace (if the buffer is ever copied to userspace).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kdb: prefer strlcpy to strncpy
2018-05-30 20:47 ` Geert Uytterhoeven
@ 2018-05-31 8:24 ` Daniel Thompson
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Thompson @ 2018-05-31 8:24 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Nick Desaulniers, Arnd Bergmann, Jason Wessel, Randy Dunlap,
Baolin Wang, Eric W. Biederman, kgdb-bugreport,
Linux Kernel Mailing List, ebiggers
On Wed, May 30, 2018 at 10:47:13PM +0200, Geert Uytterhoeven wrote:
> H Daniel,
>
> On Wed, May 30, 2018 at 4:34 PM, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > 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.
>
> >> 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.
>
> Not leak an is memory leak, but leak as in information leak of uninitialized
> data to userspace (if the buffer is ever copied to userspace).
I see... I saw "leak", I saw "memory" and was perhaps too quick to link
the two together.
The underlying bug is a buffer overflow (so a good catch and I look
forward to a v2) but, with or without Nick's change, I can't see a leak
in either sense of the word in the code that Arnd was commenting on[1].
Daniel.
[1] Clearly the undefined behaviour post-overflow *could* be a leak but
I stopped analyzing after the overflow.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-31 8:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 5:57 [PATCH] kdb: prefer strlcpy to strncpy Nick Desaulniers
2018-05-29 7:57 ` Arnd Bergmann
2018-05-30 2:01 ` Nick Desaulniers
2018-05-30 14:34 ` Daniel Thompson
2018-05-30 20:47 ` Geert Uytterhoeven
2018-05-31 8:24 ` Daniel Thompson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.