Inspired by a patch from [Justin][1] I took a closer look at kdb_read().
Despite Justin's patch being a (correct) one-line manipulation it was a tough patch to review because the surrounding code was hard to read and it looked like there were unfixed problems.
This series isn't enough to make kdb_read() beautiful but it does make it shorter, easier to reason about and fixes a buffer overflow and a screen redraw problem!
[1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-...
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- Daniel Thompson (7): kdb: Fix buffer overflow during tab-complete kdb: Use format-strings rather than '\0' injection in kdb_read() kdb: Fix console handling when editing and tab-completing commands kdb: Replace double memcpy() with memmove() in kdb_read() kdb: Merge identical case statements in kdb_read() kdb: Use format-specifiers rather than memset() for padding in kdb_read() kdb: Simplify management of tmpbuffer in kdb_read()
kernel/debug/kdb/kdb_io.c | 133 ++++++++++++++++++++-------------------------- 1 file changed, 58 insertions(+), 75 deletions(-) --- base-commit: dccce9b8780618986962ba37c373668bcf426866 change-id: 20240415-kgdb_read_refactor-2ea2dfc15dbb
Best regards,
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 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;
Currently when kdb_read() needs to reposition the cursor it uses copy and paste code that works by injecting an '\0' at the cursor position before delivering a carriage-return and reprinting the line (which stops at the '\0').
Tidy up the code by hoisting the copy and paste code into an appropriately named function. Additionally let's replace the '\0' injection with a proper field width parameter so that the string will be abridged during formatting instead.
Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 06dfbccb10336..a42607e4d1aba 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -184,6 +184,13 @@ char kdb_getchar(void) unreachable(); }
+static void kdb_position_cursor(char *prompt, char *buffer, char *cp) +{ + kdb_printf("\r%s", kdb_prompt_str); + if (cp > buffer) + kdb_printf("%.*s", (int)(cp - buffer), buffer); +} + /* * kdb_read * @@ -249,12 +256,8 @@ static char *kdb_read(char *buffer, size_t bufsize) } *(--lastchar) = '\0'; --cp; - kdb_printf("\b%s \r", cp); - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_printf("\b%s ", cp); + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 10: /* linefeed */ @@ -272,19 +275,14 @@ static char *kdb_read(char *buffer, size_t bufsize) memcpy(tmpbuffer, cp+1, lastchar - cp - 1); memcpy(cp, tmpbuffer, lastchar - cp - 1); *(--lastchar) = '\0'; - kdb_printf("%s \r", cp); - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_printf("%s ", cp); + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 1: /* Home */ if (cp > buffer) { - kdb_printf("\r"); - kdb_printf(kdb_prompt_str); cp = buffer; + kdb_position_cursor(kdb_prompt_str, buffer, cp); } break; case 5: /* End */ @@ -390,13 +388,9 @@ static char *kdb_read(char *buffer, size_t bufsize) memcpy(cp+1, tmpbuffer, lastchar - cp); *++lastchar = '\0'; *cp = key; - kdb_printf("%s\r", cp); + kdb_printf("%s", cp); ++cp; - tmp = *cp; - *cp = '\0'; - kdb_printf(kdb_prompt_str); - kdb_printf("%s", buffer); - *cp = tmp; + kdb_position_cursor(kdb_prompt_str, buffer, cp); } else { *++lastchar = '\0'; *cp++ = key;
Currently, if the cursor position is not at the end of the command buffer and the user uses the Tab-complete functions, then the console does not leave the cursor in the correct position.
For example consider the following buffer with the cursor positioned at the ^:
md kdb_pro 10 ^
Pressing tab should result in:
md kdb_prompt_str 10 ^
However this does not happen. Instead the cursor is placed at the end (after then 10) and further cursor movement redraws incorrectly. The same problem exists when we double-Tab but in a different part of the code.
Fix this by sending a carriage return and then redisplaying the text to the left of the cursor.
Cc: stable@vger.kernel.org Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- kernel/debug/kdb/kdb_io.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index a42607e4d1aba..69549fe42e87b 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -364,6 +364,8 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf("\n"); kdb_printf(kdb_prompt_str); kdb_printf("%s", buffer); + if (cp != lastchar) + kdb_position_cursor(kdb_prompt_str, buffer, cp); } else if (tab != 2 && count > 0) { /* How many new characters do we want from tmpbuffer? */ len_tmp = strlen(p_tmp) - len; @@ -377,6 +379,9 @@ static char *kdb_read(char *buffer, size_t bufsize) kdb_printf("%s", cp); cp += len_tmp; lastchar += len_tmp; + if (cp != lastchar) + kdb_position_cursor(kdb_prompt_str, + buffer, cp); } } kdb_nextline = 1; /* reset output line number */
linux-stable-mirror@lists.linaro.org
-
Daniel Thompson