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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 7 May 2020 16:13:41 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>,
        Patch Tracking <patches@...aro.org>
Subject: Re: [PATCH] kdb: Eliminate strncpy() warnings by replacing with strscpy()

Hi,

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