Re: [PATCH] kdb: Eliminate strncpy() warnings by replacing with strscpy()

[Doug Anderson]

Hi,

On Tue, Mar 3, 2020 at 12:52 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Feb 13, 2020 at 7:06 AM Daniel Thompson
> <daniel.thompson@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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:

20200507161125.1.I2cce9ac66e141230c3644b8174b6c15d4e769232@changeid">https://lore.kernel.org/r/20200507161125.1.I2cce9ac66e141230c3644b8174b6c15d4e769232@changeid

-Doug