Date: Mon, 15 Apr 2024 14:45:56 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Jason Wessel <jason.wessel@...driver.com>,
	Douglas Anderson <dianders@...omium.org>,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] kdb: replace deprecated strncpy

On Tue, Apr 09, 2024 at 01:48:38PM -0700, Justin Stitt wrote:
> Hi,
>
> On Tue, Apr 9, 2024 at 11:36 AM Daniel Thompson
> <daniel.thompson@...aro.org> wrote:
> >
> > On Mon, Apr 08, 2024 at 05:46:42PM -0700, Justin Stitt wrote:
> > > On Fri, Apr 5, 2024 at 2:51 AM Daniel Thompson
> > > <daniel.thompson@...aro.org> wrote:
> > > >
> > > > >                       len_tmp = strlen(p_tmp);
> > > > > -                     strncpy(cp, p_tmp+len, len_tmp-len + 1);
> > > > > +                     strscpy(cp, p_tmp+len, len_tmp-len + 1);
> > > >
> > > > Again, I really don't think the third argument provides the number of
> > > > characters in the destination buffer.
> > > >
> > >
> > > Right, the third argument is the length of the "remaining" characters
> > > from the completion point.
> >
> > Which is not how strscpy() is designed to be used.
> >
> >
> > > if you type "tes" and press tab then kallsyms_symbol_complete() will
> > > populate p_tmp with "test". Prior to rendering to the user, @cp points
> > > to "s", we need to catch the user up and print the rest of the symbol
> > > name since they've already typed "tes" we only need to print out "t".
> >
> > I'm more concerned about the case where you fill the buffer entirely
> > then move the cursor left until you get to the tes and then press Tab.
> > I think at the point we write too many bytes to cp.
> >
> >
> > > len_tmp is the length of the entire symbol part as returned by
> > > kallsyms_symbol_complete() and len is the length of only the
> > > user-typed symbol. Therefore, the amount of remaining characters to
> > > print is given by len_tmp-len (and +1 for a NUL-byte).
> > >
> > > So, yeah, you're right. This isn't the length of the destination but I
> > > don't see why we can't use memcpy() (or strscpy()) and have this not
> > > be considered "broken". The pointer arithmetic checks out.
> >
> > The problem with substituting strncpy() with memcpy() is that is *not*
> > obviously wrong... but it could be subtly wrong.
> >
> > We can see that the person who originally wrote this code made a pretty
> > serious mistake with strncpy() and the third argument if garbage. It is
> > therefore important to figure out what the *correct* value for argument
> > #3 should have been *before* we attempt to replace strncpy() with
> > anything.
> >
> > Transforming something we know to be broken without fixing it first
> > means it is impossible to know if the transformation is correct or not.
> > Hence the original question, how do we know there is enough space
> > after cp to store the string?
>
> Gotcha, I will find time to seriously refactor/rewrite this function
> (or at the very least the tab handling part of it).
>
> At the end of the day, though, I just want this strncpy() gone.

So... I starting looking into what it takes to provoke kdb_read() into
overflowing it's buffers. So far I have found two ways (one of which
does affect this strncpy() code as I thought).

I took the view that the strncpy() (or any other copy) into
tmpbuffer/p_tmp is just the wrong thing to do. A memmove() is simply
a better approach!

Short verison, whilst there is other refactoring needed, this change
fixes the overflow. I hope it also meets your ambition with respect
to strncpy().


Daniel.



>From aab83fc9d0e97987fdec51613b536fc32a63c544 Mon Sep 17 00:00:00 2001
From: Daniel Thompson <daniel.thompson@...aro.org>
Date: Mon, 15 Apr 2024 14:35:06 +0100
Subject: [PATCH] kdb: Fix buffer overflow during tab-complete

Currently, when the user attempts symbol completion with the Tab key, kdb
will use strncpy() to insert the completed symbol into the command buffer.
Unfortunately it passes the size of the source buffer rather than the
destination to strncpy() with predictably horrible results. Most obviously
if the command buffer is already full but cp, the cursor position, is in
the middle of the buffer, then we will write past the end of the supplied
buffer.

Even when the buffer does not overflow there are other problems when cp is
in the middle of a line. For example the cursor position is not updated
correctly meaning what is shown to the user on the console is not what is
actually present in the command buffer.

Fix this by replacing the dubious strncpy() calls with memmove()/memcpy()
calls plus explicit boundary checks to make sure we have enough space
before we start moving characters around. We also add a second call to
kdb_printf() to fix the console cursor position if needed.

Reported-by: Justin Stitt <justinstitt@...gle.com>
Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
---
 kernel/debug/kdb/kdb_io.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9443bc63c5a24..3a1b9dd890f45 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -367,14 +367,23 @@ static char *kdb_read(char *buffer, size_t bufsize)
 			kdb_printf(kdb_prompt_str);
 			kdb_printf("%s", buffer);
 		} else if (tab != 2 && count > 0) {
-			len_tmp = strlen(p_tmp);
-			strncpy(p_tmp+len_tmp, cp, lastchar-cp+1);
-			len_tmp = strlen(p_tmp);
-			strncpy(cp, p_tmp+len, len_tmp-len + 1);
-			len = len_tmp - len;
-			kdb_printf("%s", cp);
-			cp += len;
-			lastchar += len;
+			/* How many new characters do we want from tmpbuffer? */
+			len_tmp = strlen(p_tmp) - len;
+			if (lastchar + len_tmp >= bufend)
+				len_tmp = bufend - lastchar - 1;
+
+			if (len_tmp) {
+				/* + 1 ensures the '\0' is memmove'd */
+				memmove(cp+len_tmp, cp, (lastchar-cp) + 1);
+				memcpy(cp, p_tmp+len, len_tmp);
+				lastchar += len_tmp;
+
+				kdb_printf("%s", cp);
+				cp += len_tmp;
+				if (cp != lastchar)
+					kdb_printf("\r%s%.*s", kdb_prompt_str,
+						   (int)(cp - buffer), buffer);
+			}
 		}
 		kdb_nextline = 1; /* reset output line number */
 		break;
--
2.43.0

Powered by blists - more mailing lists