[v2] kdb: Refactor and fix bugs in kdb_read()

[PATCH v2 1/7] kdb: Fix buffer overflow during tab-complete

Posted by Daniel Thompson 1 year, 9 months ago

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.

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.

Reported-by: Justin Stitt <justinstitt@google.com>
Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/kdb/kdb_io.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9443bc63c5a24..06dfbccb10336 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -367,14 +367,19 @@ 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;
+
+			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);
+				kdb_printf("%s", cp);
+				cp += len_tmp;
+				lastchar += len_tmp;
+			}
 		}
 		kdb_nextline = 1; /* reset output line number */
 		break;

-- 
2.43.0

Re: [PATCH v2 1/7] kdb: Fix buffer overflow during tab-complete

Posted by Doug Anderson 1 year, 9 months ago

Hi,

On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> 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.
>
> 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.
>
> Reported-by: Justin Stitt <justinstitt@google.com>
> Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)

Boy, this function (and especially the tab handling) is hard to read.
I spent a bit of time trying to grok it and, as far as I can tell,
your patch makes things better and I don't see any bugs.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 1/7] kdb: Fix buffer overflow during tab-complete

Posted by Daniel Thompson 1 year, 9 months ago

On Mon, Apr 22, 2024 at 04:51:49PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > 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.
> >
> > 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.
> >
> > Reported-by: Justin Stitt <justinstitt@google.com>
> > Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
>
> Boy, this function (and especially the tab handling) is hard to read.

Too right. I even rewrote it using offsets rather than pointers at one
point (it didn't really make it much clearer so I dropped that for now).


> I spent a bit of time trying to grok it and, as far as I can tell,
> your patch makes things better and I don't see any bugs.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!

Daniel.

Re: [PATCH v2 1/7] kdb: Fix buffer overflow during tab-complete

Posted by Justin Stitt 1 year, 9 months ago

Hi,

On Mon, Apr 22, 2024 at 05:35:54PM +0100, Daniel Thompson wrote:
> 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.
> 
> 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.
> 
> Reported-by: Justin Stitt <justinstitt@google.com>
> Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Nice! This is better than the conversions I tried to make earlier.

Your patch helps with https://github.com/KSPP/linux/issues/90

Reviewed-by: Justin Stitt <justinstitt@google.com>
> ---
>  kernel/debug/kdb/kdb_io.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9443bc63c5a24..06dfbccb10336 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -367,14 +367,19 @@ 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;
> +
> +			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);
> +				kdb_printf("%s", cp);
> +				cp += len_tmp;
> +				lastchar += len_tmp;
> +			}
>  		}
>  		kdb_nextline = 1; /* reset output line number */
>  		break;
> 
> -- 
> 2.43.0
> 
>

Thanks
Justin