Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200213150553.313596-1-daniel.thompson@linaro.org> <CAD=FV=Uqgjgf+hL7=tpBixyxiDhXA_k5kfZ1RUtk6f+CVgUikg@mail.gmail.com>
In-Reply-To: <CAD=FV=Uqgjgf+hL7=tpBixyxiDhXA_k5kfZ1RUtk6f+CVgUikg@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 7 May 2020 16:13:41 -0700
Message-ID: <CAD=FV=VUj3gPmY0TznsneWMgWO6m_WqauTiYocnihm6byVU=YQ@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 Tue, Mar 3, 2020 at 12:52 PM Doug Anderson <dianders@chromium.org> wrote:
>
> 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;

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