Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200213150553.313596-1-daniel.thompson@linaro.org>
In-Reply-To: <20200213150553.313596-1-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 3 Mar 2020 12:52:44 -0800
Message-ID: <CAD=FV=Uqgjgf+hL7=tpBixyxiDhXA_k5kfZ1RUtk6f+CVgUikg@mail.gmail.com>
Subject: Re: [PATCH] kdb: Eliminate strncpy() warnings by replacing with strscpy()
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        kgdb-bugreport@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>,
        Patch Tracking <patches@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Feb 13, 2020 at 7:06 AM Daniel Thompson
<daniel.thompson@linaro.org> 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 <daniel.thompson@linaro.org>
> ---
>  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 <dianders@chromium.org>