From 5b6d8ef6f056d8130168746c5459d7a3fb494859 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Thu, 28 Mar 2024 15:00:17 +0100 Subject: kdb: Use str_plural() to fix Coccinelle warning Fixes the following Coccinelle/coccicheck warning reported by string_choices.cocci: opportunity for str_plural(days) Signed-off-by: Thorsten Blum Reviewed-by: Douglas Anderson Link: https://lore.kernel.org/r/20240328140015.388654-3-thorsten.blum@toblux.com Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index d05066cb40b2..664bae55f2c9 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2517,7 +2517,7 @@ static int kdb_summary(int argc, const char **argv) if (val.uptime > (24*60*60)) { int days = val.uptime / (24*60*60); val.uptime %= (24*60*60); - kdb_printf("%d day%s ", days, days == 1 ? "" : "s"); + kdb_printf("%d day%s ", days, str_plural(days)); } kdb_printf("%02ld:%02ld\n", val.uptime/(60*60), (val.uptime/60)%60); -- cgit v1.2.3 From e9730744bf3af04cda23799029342aa3cddbc454 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:34 +0100 Subject: 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. 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 Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/ Cc: stable@vger.kernel.org Reviewed-by: Douglas Anderson Reviewed-by: Justin Stitt Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-1-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 9443bc63c5a2..06dfbccb1033 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -367,14 +367,19 @@ poll_again: 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; -- cgit v1.2.3 From 09b35989421dfd5573f0b4683c7700a7483c71f9 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:35 +0100 Subject: kdb: Use format-strings rather than '\0' injection in kdb_read() 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 Tested-by: Justin Stitt Reviewed-by: Douglas Anderson Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-2-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 55 +++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 21 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 06dfbccb1033..50789c99b3ba 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -184,6 +184,33 @@ char kdb_getchar(void) unreachable(); } +/** + * kdb_position_cursor() - Place cursor in the correct horizontal position + * @prompt: Nil-terminated string containing the prompt string + * @buffer: Nil-terminated string containing the entire command line + * @cp: Cursor position, pointer the character in buffer where the cursor + * should be positioned. + * + * The cursor is positioned by sending a carriage-return and then printing + * the content of the line until we reach the correct cursor position. + * + * There is some additional fine detail here. + * + * Firstly, even though kdb_printf() will correctly format zero-width fields + * we want the second call to kdb_printf() to be conditional. That keeps things + * a little cleaner when LOGGING=1. + * + * Secondly, we can't combine everything into one call to kdb_printf() since + * that renders into a fixed length buffer and the combined print could result + * in unwanted truncation. + */ +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 * @@ -212,7 +239,6 @@ static char *kdb_read(char *buffer, size_t bufsize) * and null byte */ char *lastchar; char *p_tmp; - char tmp; static char tmpbuffer[CMD_BUFLEN]; int len = strlen(buffer); int len_tmp; @@ -249,12 +275,8 @@ poll_again: } *(--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 +294,14 @@ poll_again: 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 +407,9 @@ poll_again: 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; -- cgit v1.2.3 From db2f9c7dc29114f531df4a425d0867d01e1f1e28 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:36 +0100 Subject: kdb: Fix console handling when editing and tab-completing commands 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 Reviewed-by: Douglas Anderson Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-3-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 50789c99b3ba..5fccb46f399e 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -383,6 +383,8 @@ poll_again: 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; @@ -396,6 +398,9 @@ poll_again: 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 */ -- cgit v1.2.3 From 6244917f377bf64719551b58592a02a0336a7439 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:37 +0100 Subject: kdb: Merge identical case statements in kdb_read() The code that handles case 14 (down) and case 16 (up) has been copy and pasted despite being byte-for-byte identical. Combine them. Cc: stable@vger.kernel.org # Not a bug fix but it is needed for later bug fixes Reviewed-by: Douglas Anderson Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-4-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 5fccb46f399e..a73779529803 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -317,6 +317,7 @@ poll_again: } break; case 14: /* Down */ + case 16: /* Up */ memset(tmpbuffer, ' ', strlen(kdb_prompt_str) + (lastchar-buffer)); *(tmpbuffer+strlen(kdb_prompt_str) + @@ -331,15 +332,6 @@ poll_again: ++cp; } break; - case 16: /* Up */ - memset(tmpbuffer, ' ', - strlen(kdb_prompt_str) + (lastchar-buffer)); - *(tmpbuffer+strlen(kdb_prompt_str) + - (lastchar-buffer)) = '\0'; - kdb_printf("\r%s\r", tmpbuffer); - *lastchar = (char)key; - *(lastchar+1) = '\0'; - return lastchar; case 9: /* Tab */ if (tab < 2) ++tab; -- cgit v1.2.3 From c9b51ddb66b1d96e4d364c088da0f1dfb004c574 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:38 +0100 Subject: kdb: Use format-specifiers rather than memset() for padding in kdb_read() Currently when the current line should be removed from the display kdb_read() uses memset() to fill a temporary buffer with spaces. The problem is not that this could be trivially implemented using a format string rather than open coding it. The real problem is that it is possible, on systems with a long kdb_prompt_str, to write past the end of the tmpbuffer. Happily, as mentioned above, this can be trivially implemented using a format string. Make it so! Cc: stable@vger.kernel.org Reviewed-by: Douglas Anderson Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-5-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index a73779529803..2aeaf9765b24 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -318,11 +318,9 @@ poll_again: break; case 14: /* Down */ case 16: /* Up */ - memset(tmpbuffer, ' ', - strlen(kdb_prompt_str) + (lastchar-buffer)); - *(tmpbuffer+strlen(kdb_prompt_str) + - (lastchar-buffer)) = '\0'; - kdb_printf("\r%s\r", tmpbuffer); + kdb_printf("\r%*c\r", + (int)(strlen(kdb_prompt_str) + (lastchar - buffer)), + ' '); *lastchar = (char)key; *(lastchar+1) = '\0'; return lastchar; -- cgit v1.2.3 From 80bd73c154e3063c4f9293163daf3262335f9f86 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:39 +0100 Subject: kdb: Replace double memcpy() with memmove() in kdb_read() At several points in kdb_read() there are variants of the following code pattern (with offsets slightly altered): memcpy(tmpbuffer, cp, lastchar - cp); memcpy(cp-1, tmpbuffer, lastchar - cp); *(--lastchar) = '\0'; There is no need to use tmpbuffer here, since we can use memmove() instead so refactor in the obvious way. Additionally the strings that are being copied are already properly terminated so let's also change the code so that the library calls also move the terminator. Changing how the terminators are managed has no functional effect for now but might allow us to retire lastchar at a later point. lastchar, although stored as a pointer, is functionally equivalent to caching strlen(buffer). Reviewed-by: Douglas Anderson Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-6-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 2aeaf9765b24..40617f36a6db 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -269,12 +269,9 @@ poll_again: switch (key) { case 8: /* backspace */ if (cp > buffer) { - if (cp < lastchar) { - memcpy(tmpbuffer, cp, lastchar - cp); - memcpy(cp-1, tmpbuffer, lastchar - cp); - } - *(--lastchar) = '\0'; - --cp; + memmove(cp-1, cp, lastchar - cp + 1); + lastchar--; + cp--; kdb_printf("\b%s ", cp); kdb_position_cursor(kdb_prompt_str, buffer, cp); } @@ -291,9 +288,8 @@ poll_again: return buffer; case 4: /* Del */ if (cp < lastchar) { - memcpy(tmpbuffer, cp+1, lastchar - cp - 1); - memcpy(cp, tmpbuffer, lastchar - cp - 1); - *(--lastchar) = '\0'; + memmove(cp, cp+1, lastchar - cp); + lastchar--; kdb_printf("%s ", cp); kdb_position_cursor(kdb_prompt_str, buffer, cp); } @@ -398,9 +394,8 @@ poll_again: default: if (key >= 32 && lastchar < bufend) { if (cp < lastchar) { - memcpy(tmpbuffer, cp, lastchar - cp); - memcpy(cp+1, tmpbuffer, lastchar - cp); - *++lastchar = '\0'; + memmove(cp+1, cp, lastchar - cp + 1); + lastchar++; *cp = key; kdb_printf("%s", cp); ++cp; -- cgit v1.2.3 From 64d504cfcd514743aaed3a5b79c060f0143149e9 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Wed, 24 Apr 2024 15:03:40 +0100 Subject: kdb: Simplify management of tmpbuffer in kdb_read() The current approach to filling tmpbuffer with completion candidates is confusing, with the buffer management being especially hard to reason about. That's because it doesn't copy the completion canidate into tmpbuffer, instead of copies a whole bunch of other nonsense and then runs the completion search from the middle of tmpbuffer! Change this to copy nothing but the completion candidate into tmpbuffer. Pretty much everything else in this patch is renaming to reflect the above change: s/p_tmp/tmpbuffer/ s/buf_size/sizeof(tmpbuffer)/ Reviewed-by: Douglas Anderson Tested-by: Justin Stitt Link: https://lore.kernel.org/r/20240424-kgdb_read_refactor-v3-7-f236dbe9828d@linaro.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_io.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) (limited to 'kernel') diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 40617f36a6db..3131334d7a81 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -239,6 +239,7 @@ static char *kdb_read(char *buffer, size_t bufsize) * and null byte */ char *lastchar; char *p_tmp; + char tmp; static char tmpbuffer[CMD_BUFLEN]; int len = strlen(buffer); int len_tmp; @@ -246,8 +247,7 @@ static char *kdb_read(char *buffer, size_t bufsize) int count; int i; int diag, dtab_count; - int key, buf_size, ret; - + int key, ret; diag = kdbgetintenv("DTABCOUNT", &dtab_count); if (diag) @@ -329,21 +329,16 @@ poll_again: 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)); + *cp = tmp; + + len = strlen(tmpbuffer); + count = kallsyms_symbol_complete(tmpbuffer, sizeof(tmpbuffer)); if (tab == 2 && count > 0) { kdb_printf("\n%d symbols are found.", count); if (count > dtab_count) { @@ -355,14 +350,14 @@ poll_again: } kdb_printf("\n"); for (i = 0; i < count; i++) { - ret = kallsyms_symbol_next(p_tmp, i, buf_size); + ret = kallsyms_symbol_next(tmpbuffer, i, sizeof(tmpbuffer)); if (WARN_ON(!ret)) break; if (ret != -E2BIG) - kdb_printf("%s ", p_tmp); + kdb_printf("%s ", tmpbuffer); else - kdb_printf("%s... ", p_tmp); - *(p_tmp + len) = '\0'; + kdb_printf("%s... ", tmpbuffer); + tmpbuffer[len] = '\0'; } if (i >= dtab_count) kdb_printf("..."); @@ -373,14 +368,14 @@ poll_again: 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; + len_tmp = strlen(tmpbuffer) - 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); + memcpy(cp, tmpbuffer+len, len_tmp); kdb_printf("%s", cp); cp += len_tmp; lastchar += len_tmp; -- cgit v1.2.3