Received-SPF: pass (google.com: domain of linux-kernel+bounces-154938-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 23 Apr 2024 12:02:12 +0100
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Jason Wessel <jason.wessel@windriver.com>,
	kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 7/7] kdb: Simplify management of tmpbuffer in
 kdb_read()
Message-ID: <20240423110212.GC1567803@aspen.lan>
References: <20240422-kgdb_read_refactor-v2-0-ed51f7d145fe@linaro.org>
 <20240422-kgdb_read_refactor-v2-7-ed51f7d145fe@linaro.org>
 <CAD=FV=XqSmD4WGyBp7Cv1i8X9yjk2gH1y2j_5qzkxtDL+GKv3g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD=FV=XqSmD4WGyBp7Cv1i8X9yjk2gH1y2j_5qzkxtDL+GKv3g@mail.gmail.com>

On Mon, Apr 22, 2024 at 04:52:52PM -0700, Doug Anderson wrote:
> On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 94a638a9d52fa..640208675c9a8 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -310,21 +309,16 @@ static char *kdb_read(char *buffer, size_t bufsize)
> >         case 9: /* Tab */
> >                 if (tab < 2)
> >                         ++tab;
> > -               p_tmp = buffer;
> > -               while (*p_tmp == ' ')
> > -                       p_tmp++;
> > -               if (p_tmp > cp)
> > -                       break;
> > -               memcpy(tmpbuffer, p_tmp, cp-p_tmp);
> > -               *(tmpbuffer + (cp-p_tmp)) = '\0';
> > -               p_tmp = strrchr(tmpbuffer, ' ');
> > -               if (p_tmp)
> > -                       ++p_tmp;
> > -               else
> > -                       p_tmp = tmpbuffer;
> > -               len = strlen(p_tmp);
> > -               buf_size = sizeof(tmpbuffer) - (p_tmp - tmpbuffer);
> > -               count = kallsyms_symbol_complete(p_tmp, buf_size);
> > +
> > +               tmp = *cp;
> > +               *cp = '\0';
> > +               p_tmp = strrchr(buffer, ' ');
> > +               p_tmp = (p_tmp ? p_tmp + 1 : buffer);
> > +               strscpy(tmpbuffer, p_tmp, sizeof(tmpbuffer));
>
> You're now using strscpy() here. Is that actually important, or are
> you just following good practices and being extra paranoid? If it's
> actually important, this probably also needs to be CCed to stable,
> right? The old code just assumed that it  could copy the whole buffer
> into tmpbuffer. I assume that was OK, but it wasn't documented in the
> function comments that there was a maximum size that buffer could
> be...

This is pretty much it.

I used strscpy() because the function does not document any upper limit
on the length of the supplied buffer. Thus using strscpy() means we are
resilient in the face of future refactoring.

I chose not to Cc: stable@... since it's only a theoretic overflow.
With the code as it currently is kdb_read() should never be passed a
buffer long enough to cause problems.


Daniel.