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 two buffer overflows and a
screen redraw problem!
[1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/
Signed-off-by: Daniel Thompson <[email protected]>
---
Changes in v2:
- No code changes!
- I belatedly realized that one of the cleanups actually fixed a buffer
overflow so there are changes to Cc: (to add stable@...) and to one
of the patch descriptions.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
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: Merge identical case statements in kdb_read()
kdb: Use format-specifiers rather than memset() for padding in kdb_read()
kdb: Replace double memcpy() with memmove() 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,
--
Daniel Thompson <[email protected]>
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 <[email protected]>
Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
Cc: [email protected]
Signed-off-by: Daniel Thompson <[email protected]>
---
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
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 pas
the end of the tmpbuffer.
Happily, as mentioned above, this can be trivially implemented using a
format string. Make it so!
Cc: [email protected]
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index f167894b11b8e..2cd17313fe652 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -299,11 +299,9 @@ static char *kdb_read(char *buffer, size_t bufsize)
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;
--
2.43.0
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).
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 2cd17313fe652..94a638a9d52fa 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -250,12 +250,9 @@ static char *kdb_read(char *buffer, size_t bufsize)
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);
}
@@ -272,9 +269,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
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);
}
@@ -379,9 +375,8 @@ static char *kdb_read(char *buffer, size_t bufsize)
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;
--
2.43.0
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 stearch 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)/
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 40 +++++++++++++++++-----------------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 94a638a9d52fa..640208675c9a8 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -227,8 +227,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)
@@ -310,21 +309,16 @@ static char *kdb_read(char *buffer, size_t bufsize)
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) {
@@ -336,14 +330,14 @@ static char *kdb_read(char *buffer, size_t bufsize)
}
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("...");
@@ -354,14 +348,14 @@ static char *kdb_read(char *buffer, size_t bufsize)
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;
--
2.43.0
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: [email protected]
Signed-off-by: Daniel Thompson <[email protected]>
---
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 */
--
2.43.0
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: [email protected] # Not a bug fix but it is needed for later bug fixes
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 69549fe42e87b..f167894b11b8e 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -298,6 +298,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
}
break;
case 14: /* Down */
+ case 16: /* Up */
memset(tmpbuffer, ' ',
strlen(kdb_prompt_str) + (lastchar-buffer));
*(tmpbuffer+strlen(kdb_prompt_str) +
@@ -312,15 +313,6 @@ static char *kdb_read(char *buffer, size_t bufsize)
++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;
--
2.43.0
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 <[email protected]>
> Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> Cc: [email protected]
> Signed-off-by: Daniel Thompson <[email protected]>
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 <[email protected]>
> ---
> 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
Hi,
On Mon, Apr 22, 2024 at 05:35:53PM +0100, Daniel Thompson wrote:
> 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 two buffer overflows and a
> screen redraw problem!
>
> [1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/
>
> Signed-off-by: Daniel Thompson <[email protected]>
Seems to work nicely.
There is some weird behavior which was present before your patch and is
still present with it (let >< represent cursor position):
[0]kdb> test_ap>< (now press TAB)
[0]kdb> test_aperfmperf>< (so far so good, we got our autocomplete)
[0]kdb> test_ap><erfmperf (now, let's move the cursor back and press TAB again)
[0]kdb> test_aperfmperf><erfmperf
This is because the autocomplete engine is not considering the
characters after the cursor position. To be clear, this isn't really a
bug but rather a decision to be made about which functionality is
desired.
For example, my shell (zsh) will just simply move the cursor back to
the end of the complete match instead of re-writing stuff.
At any rate,
Tested-by: Justin Stitt <[email protected]>
> ---
> Changes in v2:
> - No code changes!
> - I belatedly realized that one of the cleanups actually fixed a buffer
> overflow so there are changes to Cc: (to add stable@...) and to one
> of the patch descriptions.
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> 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: Merge identical case statements in kdb_read()
> kdb: Use format-specifiers rather than memset() for padding in kdb_read()
> kdb: Replace double memcpy() with memmove() 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,
> --
> Daniel Thompson <[email protected]>
>
Thanks
Justin
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
<[email protected]> 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 <[email protected]>
> Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> Cc: [email protected]
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> 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 <[email protected]>
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
<[email protected]> wrote:
>
> 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: [email protected] # Not a bug fix but it is needed for later bug fixes
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
<[email protected]> wrote:
>
> 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 pas
nit: s/pas/past
> the end of the tmpbuffer.
>
> Happily, as mentioned above, this can be trivially implemented using a
> format string. Make it so!
>
> Cc: [email protected]
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
<[email protected]> wrote:
>
> 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).
>
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
<[email protected]> wrote:
>
> 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 stearch 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)/
>
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 40 +++++++++++++++++-----------------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
Definitely an improvement.
Reviewed-by: Douglas Anderson <[email protected]>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 94a638a9d52fa..640208675c9a8 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -227,8 +227,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)
> @@ -310,21 +309,16 @@ static char *kdb_read(char *buffer, size_t bufsize)
> 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));
You're now using strscpy() here. Is that actually important, or are
you just following good practices and being extra paranoid? If it's
actually important, this probably also needs to be CCed to stable,
right? The old code just assumed that it could copy the whole buffer
into tmpbuffer. I assume that was OK, but it wasn't documented in the
function comments that there was a maximum size that buffer could
be...
-Doug
Hi,
On Mon, Apr 22, 2024 at 9:37 AM Daniel Thompson
<[email protected]> wrote:
>
> 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: [email protected]
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 5 +++++
> 1 file changed, 5 insertions(+)
Reviewed-by: Douglas Anderson <[email protected]>
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
> <[email protected]> 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 <[email protected]>
> > Closes: https://lore.kernel.org/all/CAFhGd8qESuuifuHsNjFPR-Va3P80bxrw+LqvC8deA8GziUJLpw@mail.gmail.com/
> > Cc: [email protected]
> > Signed-off-by: Daniel Thompson <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
Daniel.
On Mon, Apr 22, 2024 at 04:52:52PM -0700, Doug Anderson wrote:
> On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
> <[email protected]> wrote:
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 94a638a9d52fa..640208675c9a8 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -310,21 +309,16 @@ static char *kdb_read(char *buffer, size_t bufsize)
> > 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));
>
> You're now using strscpy() here. Is that actually important, or are
> you just following good practices and being extra paranoid? If it's
> actually important, this probably also needs to be CCed to stable,
> right? The old code just assumed that it could copy the whole buffer
> into tmpbuffer. I assume that was OK, but it wasn't documented in the
> function comments that there was a maximum size that buffer could
> be...
This is pretty much it.
I used strscpy() because the function does not document any upper limit
on the length of the supplied buffer. Thus using strscpy() means we are
resilient in the face of future refactoring.
I chose not to Cc: stable@... since it's only a theoretic overflow.
With the code as it currently is kdb_read() should never be passed a
buffer long enough to cause problems.
Daniel.
On Mon, Apr 22, 2024 at 10:49:29PM +0000, Justin Stitt wrote:
> Hi,
>
> On Mon, Apr 22, 2024 at 05:35:53PM +0100, Daniel Thompson wrote:
> > 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 two buffer overflows and a
> > screen redraw problem!
> >
> > [1]: https://lore.kernel.org/all/20240403-strncpy-kernel-debug-kdb-kdb_io-c-v1-1-7f78a08e9ff4@google.com/
> >
> > Signed-off-by: Daniel Thompson <[email protected]>
>
> Seems to work nicely.
>
> There is some weird behavior which was present before your patch and is
> still present with it (let >< represent cursor position):
>
> [0]kdb> test_ap>< (now press TAB)
>
> [0]kdb> test_aperfmperf>< (so far so good, we got our autocomplete)
>
> [0]kdb> test_ap><erfmperf (now, let's move the cursor back and press TAB again)
>
> [0]kdb> test_aperfmperf><erfmperf
>
> This is because the autocomplete engine is not considering the
> characters after the cursor position. To be clear, this isn't really a
> bug but rather a decision to be made about which functionality is
> desired.
>
> For example, my shell (zsh) will just simply move the cursor back to
> the end of the complete match instead of re-writing stuff.
Interesting observation. I hadn't realized zsh does that. FWIW default
settings for both bash and gdb complete the same way as kdb. Overall
that makes me OK with the current kdb behaviour.
However I was curious about this and found "skip-completed-text" in
the GNU Readline documentation. I think that would give you zsh-like
completion in gdb if you ever want it!
> At any rate,
> Tested-by: Justin Stitt <[email protected]>
Thanks.
Daniel.