From 120fb87ced8eb95531ff69ec269fcbcc5be24e9c Mon Sep 17 00:00:00 2001 From: Yuran Pereira Date: Mon, 28 Oct 2024 19:19:16 +0000 Subject: [PATCH 1/6] kdb: Replace the use of simple_strto with safer kstrto in kdb_main The simple_str* family of functions perform no error checking in scenarios where the input value overflows the intended output variable. This results in these functions successfully returning even when the output does not match the input string. Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(), simple_strtoul(), and simple_strtoull() functions explicitly ignore overflows, which may lead to unexpected results in callers." Hence, the use of those functions is discouraged. This patch replaces all uses of the simple_strto* series of functions with their safer kstrto* alternatives. Side effects of this patch: - Every string to long or long long conversion using kstrto* is now checked for failure. - kstrto* errors are handled with appropriate `KDB_BADINT` wherever applicable. - A good side effect is that we end up saving a few lines of code since unlike in simple_strto* functions, kstrto functions do not need an additional "end pointer" variable, and the return values of the latter can be directly checked in an "if" statement without the need to define additional `ret` or `err` variables. This, of course, results in cleaner, yet still easy to understand code. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull Signed-off-by: Yuran Pereira [nir: addressed review comments by fixing styling, invalid conversion and a missing error return] Signed-off-by: Nir Lichtman Reviewed-by: Douglas Anderson Link: https://lore.kernel.org/r/20241028191916.GA918454@lichtman.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_main.c | 69 +++++++++++-------------------------- 1 file changed, 21 insertions(+), 48 deletions(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index f5f7d7fb5936..f8703ab760d9 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -306,8 +306,8 @@ static int kdbgetulenv(const char *match, unsigned long *value) return KDB_NOTENV; if (strlen(ep) == 0) return KDB_NOENVVALUE; - - *value = simple_strtoul(ep, NULL, 0); + if (kstrtoul(ep, 0, value)) + return KDB_BADINT; return 0; } @@ -402,42 +402,23 @@ static void kdb_printenv(void) */ int kdbgetularg(const char *arg, unsigned long *value) { - char *endp; - unsigned long val; - - val = simple_strtoul(arg, &endp, 0); - - if (endp == arg) { - /* - * Also try base 16, for us folks too lazy to type the - * leading 0x... - */ - val = simple_strtoul(arg, &endp, 16); - if (endp == arg) + /* + * If the first fails, also try base 16, for us + * folks too lazy to type the leading 0x... + */ + if (kstrtoul(arg, 0, value)) { + if (kstrtoul(arg, 16, value)) return KDB_BADINT; } - - *value = val; - return 0; } int kdbgetu64arg(const char *arg, u64 *value) { - char *endp; - u64 val; - - val = simple_strtoull(arg, &endp, 0); - - if (endp == arg) { - - val = simple_strtoull(arg, &endp, 16); - if (endp == arg) + if (kstrtou64(arg, 0, value)) { + if (kstrtou64(arg, 16, value)) return KDB_BADINT; } - - *value = val; - return 0; } @@ -473,10 +454,10 @@ int kdb_set(int argc, const char **argv) */ if (strcmp(argv[1], "KDBDEBUG") == 0) { unsigned int debugflags; - char *cp; + int ret; - debugflags = simple_strtoul(argv[2], &cp, 0); - if (cp == argv[2] || debugflags & ~KDB_DEBUG_FLAG_MASK) { + ret = kstrtouint(argv[2], 0, &debugflags); + if (ret || debugflags & ~KDB_DEBUG_FLAG_MASK) { kdb_printf("kdb: illegal debug flags '%s'\n", argv[2]); return 0; @@ -1619,10 +1600,10 @@ static int kdb_md(int argc, const char **argv) if (!argv[0][3]) valid = 1; else if (argv[0][3] == 'c' && argv[0][4]) { - char *p; - repeat = simple_strtoul(argv[0] + 4, &p, 10); + if (kstrtouint(argv[0] + 4, 10, &repeat)) + return KDB_BADINT; mdcount = ((repeat * bytesperword) + 15) / 16; - valid = !*p; + valid = 1; } last_repeat = repeat; } else if (strcmp(argv[0], "md") == 0) @@ -2083,15 +2064,10 @@ static int kdb_dmesg(int argc, const char **argv) if (argc > 2) return KDB_ARGCOUNT; if (argc) { - char *cp; - lines = simple_strtol(argv[1], &cp, 0); - if (*cp) + if (kstrtoint(argv[1], 0, &lines)) lines = 0; - if (argc > 1) { - adjust = simple_strtoul(argv[2], &cp, 0); - if (*cp || adjust < 0) - adjust = 0; - } + if (argc > 1 && (kstrtoint(argv[2], 0, &adjust) || adjust < 0)) + adjust = 0; } /* disable LOGGING if set */ @@ -2428,14 +2404,12 @@ static int kdb_help(int argc, const char **argv) static int kdb_kill(int argc, const char **argv) { long sig, pid; - char *endp; struct task_struct *p; if (argc != 2) return KDB_ARGCOUNT; - sig = simple_strtol(argv[1], &endp, 0); - if (*endp) + if (kstrtol(argv[1], 0, &sig)) return KDB_BADINT; if ((sig >= 0) || !valid_signal(-sig)) { kdb_printf("Invalid signal parameter.<-signal>\n"); @@ -2443,8 +2417,7 @@ static int kdb_kill(int argc, const char **argv) } sig = -sig; - pid = simple_strtol(argv[2], &endp, 0); - if (*endp) + if (kstrtol(argv[2], 0, &pid)) return KDB_BADINT; if (pid <= 0) { kdb_printf("Process ID must be large than 0.\n"); From 0c10cc2435115c36dcb611f8e1ed99ba6de6f17f Mon Sep 17 00:00:00 2001 From: Yuran Pereira Date: Mon, 28 Oct 2024 19:21:00 +0000 Subject: [PATCH 2/6] trace: kdb: Replace simple_strtoul with kstrtoul in kdb_ftdump The function simple_strtoul performs no error checking in scenarios where the input value overflows the intended output variable. This results in this function successfully returning, even when the output does not match the input string (aka the function returns successfully even when the result is wrong). Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(), simple_strtoul(), and simple_strtoull() functions explicitly ignore overflows, which may lead to unexpected results in callers." Hence, the use of those functions is discouraged. This patch replaces all uses of the simple_strtoul with the safer alternatives kstrtoint and kstrtol. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull Signed-off-by: Yuran Pereira Reviewed-by: Douglas Anderson Acked-by: Masami Hiramatsu (Google) Acked-by: Steven Rostedt (Google) [nir: style fixes] Signed-off-by: Nir Lichtman Link: https://lore.kernel.org/r/20241028192100.GB918454@lichtman.org Signed-off-by: Daniel Thompson --- kernel/trace/trace_kdb.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c index 59857a1ee44c..1e72d20b3c2f 100644 --- a/kernel/trace/trace_kdb.c +++ b/kernel/trace/trace_kdb.c @@ -96,22 +96,19 @@ static int kdb_ftdump(int argc, const char **argv) { int skip_entries = 0; long cpu_file; - char *cp; + int err; int cnt; int cpu; if (argc > 2) return KDB_ARGCOUNT; - if (argc) { - skip_entries = simple_strtol(argv[1], &cp, 0); - if (*cp) - skip_entries = 0; - } + if (argc && kstrtoint(argv[1], 0, &skip_entries)) + return KDB_BADINT; if (argc == 2) { - cpu_file = simple_strtol(argv[2], &cp, 0); - if (*cp || cpu_file >= NR_CPUS || cpu_file < 0 || + err = kstrtol(argv[2], 0, &cpu_file); + if (err || cpu_file >= NR_CPUS || cpu_file < 0 || !cpu_online(cpu_file)) return KDB_BADINT; } else { From 9131d6a7a726d7a372bc35d27df9ebe3f011508e Mon Sep 17 00:00:00 2001 From: Nir Lichtman Date: Mon, 28 Oct 2024 19:22:28 +0000 Subject: [PATCH 3/6] kdb: Remove fallback interpretation of arbitrary numbers as hex Remove logic that enables a fallback of interpreting numbers supplied in KDB CLI to be interpreted as hex without explicit "0x" prefix as this can be confusing for the end users. Suggested-by: Douglas Anderson Reviewed-by: Douglas Anderson Signed-off-by: Nir Lichtman Link: https://lore.kernel.org/r/20241028192228.GC918454@lichtman.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index f8703ab760d9..5f4be507d79f 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -402,23 +402,15 @@ static void kdb_printenv(void) */ int kdbgetularg(const char *arg, unsigned long *value) { - /* - * If the first fails, also try base 16, for us - * folks too lazy to type the leading 0x... - */ - if (kstrtoul(arg, 0, value)) { - if (kstrtoul(arg, 16, value)) - return KDB_BADINT; - } + if (kstrtoul(arg, 0, value)) + return KDB_BADINT; return 0; } int kdbgetu64arg(const char *arg, u64 *value) { - if (kstrtou64(arg, 0, value)) { - if (kstrtou64(arg, 16, value)) - return KDB_BADINT; - } + if (kstrtou64(arg, 0, value)) + return KDB_BADINT; return 0; } From 272fad470b6548d8f9fb293a3fc6918c9d053d91 Mon Sep 17 00:00:00 2001 From: Nir Lichtman Date: Sun, 27 Oct 2024 20:47:29 +0000 Subject: [PATCH 4/6] kdb: Fix breakpoint enable to be silent if already enabled Fix the breakpoint enable command (be) to a logic that is inline with the breakpoint disable command (bd) in which if the breakpoint is already in an enabled state, do not print the message of enabled again to the user. Also a small nit fix of the new line in a separate print. Signed-off-by: Nir Lichtman Link: https://lore.kernel.org/r/20241027204729.GA907155@lichtman.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_bp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c index 372025cf1ca3..c0c2072f5452 100644 --- a/kernel/debug/kdb/kdb_bp.c +++ b/kernel/debug/kdb/kdb_bp.c @@ -460,13 +460,15 @@ static int kdb_bc(int argc, const char **argv) break; case KDBCMD_BE: + if (bp->bp_enabled) + break; + bp->bp_enabled = 1; kdb_printf("Breakpoint %d at " - kdb_bfd_vma_fmt " enabled", + kdb_bfd_vma_fmt " enabled\n", i, bp->bp_addr); - kdb_printf("\n"); break; case KDBCMD_BD: if (!bp->bp_enabled) From 9c98750eb3079ee6e47a448fe095347821a21b45 Mon Sep 17 00:00:00 2001 From: Daniel Thompson Date: Fri, 8 Nov 2024 08:30:45 +0000 Subject: [PATCH 5/6] MAINTAINERS: Use Daniel Thompson's korg address for kgdb work Going forward, I'll be using my kernel.org address for upstream work. Reviewed-by: Douglas Anderson Link: https://lore.kernel.org/r/20241108-new-maintainer-address-2-v1-2-47c9d71aac11@linaro.org Signed-off-by: Daniel Thompson --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index a27407950242..2f20596c822e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12659,7 +12659,7 @@ F: samples/kfifo/ KGDB / KDB /debug_core M: Jason Wessel -M: Daniel Thompson +M: Daniel Thompson R: Douglas Anderson L: kgdb-bugreport@lists.sourceforge.net S: Maintained From 24b2455fe8fce17258fab4bb945d8e6929baeb77 Mon Sep 17 00:00:00 2001 From: Nir Lichtman Date: Mon, 11 Nov 2024 21:56:22 +0000 Subject: [PATCH 6/6] kdb: fix ctrl+e/a/f/b/d/p/n broken in keyboard mode Problem: When using kdb via keyboard it does not react to control characters which are supported in serial mode. Example: Chords such as ctrl+a/e/d/p do not work in keyboard mode Solution: Before disregarding non-printable key characters, check if they are one of the supported control characters, I have took the control characters from the switch case upwards in this function that translates scan codes of arrow keys/backspace/home/.. to the control characters. Suggested-by: Douglas Anderson Signed-off-by: Nir Lichtman Reviewed-by: Douglas Anderson Link: https://lore.kernel.org/r/20241111215622.GA161253@lichtman.org Signed-off-by: Daniel Thompson --- kernel/debug/kdb/kdb_keyboard.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c index 3c2987f46f6e..3a74604fdb8a 100644 --- a/kernel/debug/kdb/kdb_keyboard.c +++ b/kernel/debug/kdb/kdb_keyboard.c @@ -25,6 +25,8 @@ #define KBD_STAT_OBF 0x01 /* Keyboard output buffer full */ #define KBD_STAT_MOUSE_OBF 0x20 /* Mouse output buffer full */ +#define CTRL(c) ((c) - 64) + static int kbd_exists; static int kbd_last_ret; @@ -123,24 +125,24 @@ int kdb_get_kbd_char(void) return 8; } - /* Special Key */ + /* Translate special keys to equivalent CTRL control characters */ switch (scancode) { case 0xF: /* Tab */ - return 9; + return CTRL('I'); case 0x53: /* Del */ - return 4; + return CTRL('D'); case 0x47: /* Home */ - return 1; + return CTRL('A'); case 0x4F: /* End */ - return 5; + return CTRL('E'); case 0x4B: /* Left */ - return 2; + return CTRL('B'); case 0x48: /* Up */ - return 16; + return CTRL('P'); case 0x50: /* Down */ - return 14; + return CTRL('N'); case 0x4D: /* Right */ - return 6; + return CTRL('F'); } if (scancode == 0xe0) @@ -172,6 +174,19 @@ int kdb_get_kbd_char(void) switch (KTYP(keychar)) { case KT_LETTER: case KT_LATIN: + switch (keychar) { + /* non-printable supported control characters */ + case CTRL('A'): /* Home */ + case CTRL('B'): /* Left */ + case CTRL('D'): /* Del */ + case CTRL('E'): /* End */ + case CTRL('F'): /* Right */ + case CTRL('I'): /* Tab */ + case CTRL('N'): /* Down */ + case CTRL('P'): /* Up */ + return keychar; + } + if (isprint(keychar)) break; /* printable characters */ fallthrough;