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 <daniel.thompson@linaro.org>
---
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.0Hi,
On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson
<daniel.thompson@linaro.org> 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 <daniel.thompson@linaro.org>
> ---
> kernel/debug/kdb/kdb_io.c | 40 +++++++++++++++++-----------------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
Definitely an improvement.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 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
On Mon, Apr 22, 2024 at 04:52:52PM -0700, Doug Anderson wrote: > On Mon, Apr 22, 2024 at 9:38 AM Daniel Thompson > <daniel.thompson@linaro.org> 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.
© 2016 - 2026 Red Hat, Inc.