Currently the code to manage the kdb history buffer uses strncpy() to
copy strings to/and from the history and exhibits the classic "but
nobody ever told me that strncpy() doesn't always terminate strings"
bug. Modern gcc compilers recognise this bug and issue a warning.
In reality these calls will only abridge the copied string if kdb_read()
has *already* overflowed the command buffer. Thus the use of counted
copies here is only used to reduce the secondary effects of a bug
elsewhere in the code.
Therefore transitioning these calls into strscpy() (without checking
the return code) is appropriate.
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index ba12e9f4661e..a4641be4123c 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1102,12 +1102,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);
+ strscpy(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);
+ strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
return 1;
}
return 0;
@@ -1314,7 +1314,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
if (*cmdbuf != '\n') {
if (*cmdbuf < 32) {
if (cmdptr == cmd_head) {
- strncpy(cmd_hist[cmd_head], cmd_cur,
+ strscpy(cmd_hist[cmd_head], cmd_cur,
CMD_BUFLEN);
*(cmd_hist[cmd_head] +
strlen(cmd_hist[cmd_head])-1) = '\0';
@@ -1324,7 +1324,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
cmdbuf = cmd_cur;
goto do_full_getstr;
} else {
- strncpy(cmd_hist[cmd_head], cmd_cur,
+ strscpy(cmd_hist[cmd_head], cmd_cur,
CMD_BUFLEN);
}
base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9
--
2.23.0
Hi,
On Thu, Feb 13, 2020 at 7:06 AM Daniel Thompson
<[email protected]> wrote:
>
> Currently the code to manage the kdb history buffer uses strncpy() to
> copy strings to/and from the history and exhibits the classic "but
> nobody ever told me that strncpy() doesn't always terminate strings"
> bug. Modern gcc compilers recognise this bug and issue a warning.
>
> In reality these calls will only abridge the copied string if kdb_read()
> has *already* overflowed the command buffer. Thus the use of counted
> copies here is only used to reduce the secondary effects of a bug
> elsewhere in the code.
>
> Therefore transitioning these calls into strscpy() (without checking
> the return code) is appropriate.
>
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_main.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index ba12e9f4661e..a4641be4123c 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -1102,12 +1102,12 @@ static int handle_ctrl_cmd(char *cmd)
> case CTRL_P:
> if (cmdptr != cmd_tail)
> cmdptr = (cmdptr-1) % KDB_CMD_HISTORY_COUNT;
The above line (not touched by your patch) is slightly worrying to me.
I always have it in mind that "%" of numbers that might be negative
isn't an amazingly good idea. Some searches say that this must be
true:
a == (a / b * b) + a % b
...which makes it feel like this is totally broken because "cmdptr"
will end up as -1. Huh?
OK, after much digging and some printouts, I figured this out. cmdptr
is _unsigned_ and KDB_CMD_HISTORY_COUNT is a power of 2 (it's 32)
which makes this work. AKA if you start out at 0 and subtract 1, you
get 0xffffffff and then that "% 32" is 31 which is the answer that was
desired. Totally non-obvious.
I guess a future change should make the above:
cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
KDB_CMD_HISTORY_COUNT;
> - strncpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> + strscpy(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);
> + strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> return 1;
> }
> return 0;
> @@ -1314,7 +1314,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
> if (*cmdbuf != '\n') {
> if (*cmdbuf < 32) {
> if (cmdptr == cmd_head) {
> - strncpy(cmd_hist[cmd_head], cmd_cur,
> + strscpy(cmd_hist[cmd_head], cmd_cur,
> CMD_BUFLEN);
> *(cmd_hist[cmd_head] +
> strlen(cmd_hist[cmd_head])-1) = '\0';
At first I thought the line after the strscpy() could now be removed,
but then I realized that it was stripping yet one character back.
It's a bit twisted that this relies on kdb_getstr() happening to leave
the buffer in a specific way (zero-terminated and with one extra
character at the end) when kdb_getstr() returns it's special control
characters, but not part of your change. I guess maybe a future
cleanup could try to make this saner...
One thing that could be done to at least make it slightly cleaner
(aside from adding a comment) would be to take advantage of the return
value of strscpy() to avoid the strlen().
As with all things kgdb, there are plenty of things to fix but your
change moves us in the right direction. Thus:
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Tue, Mar 3, 2020 at 12:52 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Feb 13, 2020 at 7:06 AM Daniel Thompson
> <[email protected]> wrote:
> >
> > Currently the code to manage the kdb history buffer uses strncpy() to
> > copy strings to/and from the history and exhibits the classic "but
> > nobody ever told me that strncpy() doesn't always terminate strings"
> > bug. Modern gcc compilers recognise this bug and issue a warning.
> >
> > In reality these calls will only abridge the copied string if kdb_read()
> > has *already* overflowed the command buffer. Thus the use of counted
> > copies here is only used to reduce the secondary effects of a bug
> > elsewhere in the code.
> >
> > Therefore transitioning these calls into strscpy() (without checking
> > the return code) is appropriate.
> >
> > Signed-off-by: Daniel Thompson <[email protected]>
> > ---
> > kernel/debug/kdb/kdb_main.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index ba12e9f4661e..a4641be4123c 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -1102,12 +1102,12 @@ static int handle_ctrl_cmd(char *cmd)
> > case CTRL_P:
> > if (cmdptr != cmd_tail)
> > cmdptr = (cmdptr-1) % KDB_CMD_HISTORY_COUNT;
>
> The above line (not touched by your patch) is slightly worrying to me.
> I always have it in mind that "%" of numbers that might be negative
> isn't an amazingly good idea. Some searches say that this must be
> true:
>
> a == (a / b * b) + a % b
>
> ...which makes it feel like this is totally broken because "cmdptr"
> will end up as -1. Huh?
>
> OK, after much digging and some printouts, I figured this out. cmdptr
> is _unsigned_ and KDB_CMD_HISTORY_COUNT is a power of 2 (it's 32)
> which makes this work. AKA if you start out at 0 and subtract 1, you
> get 0xffffffff and then that "% 32" is 31 which is the answer that was
> desired. Totally non-obvious.
>
> I guess a future change should make the above:
>
> cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
> KDB_CMD_HISTORY_COUNT;
This has been sitting in the back of my mind for a while. Finally posted:
https://lore.kernel.org/r/20200507161125.1.I2cce9ac66e141230c3644b8174b6c15d4e769232@changeid
-Doug