[PATCH 1/2] hexagon/traps.c: use KSYM_NAME_LEN in array size

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")

Co-developed-by: Onkarnath <redacted>
Signed-off-by: Onkarnath <redacted>
Signed-off-by: Maninder Singh <redacted>
---
 arch/hexagon/kernel/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index 6447763ce5a9..65b30b6ea226 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c

@@ -82,7 +82,7 @@ static void do_show_stack(struct task_struct *task, unsigned long *fp,
 	const char *name = NULL;
 	unsigned long *newfp;
 	unsigned long low, high;
-	char tmpstr[128];
+	char tmpstr[KSYM_NAME_LEN];
 	char *modname;
 	int i;
 

-- 
2.17.1

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for powerpc it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")

Co-developed-by: Onkarnath <redacted>
Signed-off-by: Onkarnath <redacted>
Signed-off-by: Maninder Singh <redacted>
---
 arch/powerpc/xmon/xmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 728d3c257e4a..70c4c59a1a8f 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c

@@ -88,7 +88,7 @@ static unsigned long ndump = 64;
 static unsigned long nidump = 16;
 static unsigned long ncsum = 4096;
 static int termch;
-static char tmpstr[128];
+static char tmpstr[KSYM_NAME_LEN];
 static int tracing_enabled;
 
 static long bus_error_jmp[JMP_BUF_LEN];

-- 
2.17.1

Maninder Singh [off-list ref] writes:

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for powerpc it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")

AFAICS that's the wrong sha.

That commit appears in linux-next, but the commit that actually went
into mainline is:

  b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512")

So I'll update the change log to refer to that.

cheers

On Mon 2023-05-29 16:43:36, Maninder Singh wrote:

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")

As mentioned by Michael Ellerman for the 2nd patch, the right upstream
commit is:

b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512")

Co-developed-by: Onkarnath <redacted>
Signed-off-by: Onkarnath <redacted>
Signed-off-by: Maninder Singh <redacted>

With the updated commit hash:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

On Mon, May 29, 2023 at 1:14 PM Maninder Singh [off-list ref] wrote:

+static char tmpstr[KSYM_NAME_LEN];

Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being
used, but the name seems discarded? Can
`kallsyms_lookup_size_offset()` be used instead, thus avoiding the
usage of the buffer there to begin with?

Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which
then is used to do a `kallsyms_lookup_name()`, so I guess symbols
larger than 64 couldn't be found. I have no idea about what are the
external constraints here, but perhaps it is possible to increase the
`line` buffer etc. to then allow for bigger symbols to be found.

Cheers,
Miguel

On Mon, May 29, 2023 at 1:14 PM Maninder Singh [off-list ref] wrote:

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")

Co-developed-by: Onkarnath <redacted>
Signed-off-by: Onkarnath <redacted>
Signed-off-by: Maninder Singh <redacted>

With the updated commit hash:

Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

From: Maninder Singh

Sent: 29 May 2023 12:14

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

The only safe way to pass a fixed size string is to embed the char[] in
a structure and pass the structure address.

Pretty much anything else is doomed to be buggy.

Whether is it actually sane to require the caller allocate
such a large buffer (hi rust) is another matter entirely.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Miguel Ojeda [off-list ref] writes:

On Mon, May 29, 2023 at 1:14 PM Maninder Singh [off-list ref] wrote:

quoted

+static char tmpstr[KSYM_NAME_LEN];

Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being
used, but the name seems discarded? Can
`kallsyms_lookup_size_offset()` be used instead, thus avoiding the
usage of the buffer there to begin with?

A few lines below it uses the modname, and AFAICS there's no (easy) way
to lookup the modname without also looking up the name.

Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which
then is used to do a `kallsyms_lookup_name()`, so I guess symbols
larger than 64 couldn't be found. I have no idea about what are the
external constraints here, but perhaps it is possible to increase the
`line` buffer etc. to then allow for bigger symbols to be found.

Yeah that looks wrong. I don't see any symbols that long in current
kernels, but we should fix it.

Thanks for looking.

cheers

On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman [off-list ref] wrote:

quoted

Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being
used, but the name seems discarded? Can
`kallsyms_lookup_size_offset()` be used instead, thus avoiding the
usage of the buffer there to begin with?

A few lines below it uses the modname, and AFAICS there's no (easy) way
to lookup the modname without also looking up the name.

Hmm... I think you are looking at the `xmon_print_symbol()` one? I was
referring to the `get_function_bounds()` one, where the `modname`
parameter is `NULL` (and the `name` contents are not used, only
whether it was found or not).

quoted

Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which
then is used to do a `kallsyms_lookup_name()`, so I guess symbols
larger than 64 couldn't be found. I have no idea about what are the
external constraints here, but perhaps it is possible to increase the
`line` buffer etc. to then allow for bigger symbols to be found.

Yeah that looks wrong. I don't see any symbols that long in current
kernels, but we should fix it.

Thanks for looking.

My pleasure!

Cheers,
Miguel

Miguel Ojeda [off-list ref] writes:

On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman [off-list ref] wrote:

quoted

quoted

Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being
used, but the name seems discarded? Can
`kallsyms_lookup_size_offset()` be used instead, thus avoiding the
usage of the buffer there to begin with?

A few lines below it uses the modname, and AFAICS there's no (easy) way
to lookup the modname without also looking up the name.

Hmm... I think you are looking at the `xmon_print_symbol()` one? I was
referring to the `get_function_bounds()` one, where the `modname`
parameter is `NULL` (and the `name` contents are not used, only
whether it was found or not).

Yes you're right, apparently I can't read :}

cheers

On Mon, 29 May 2023 16:43:36 +0530, Maninder Singh wrote:

kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
writes on index "KSYM_NAME_LEN - 1".

Thus array size should be KSYM_NAME_LEN.

for hexagon it was defined as "128" directly.
and commit '61968dbc2d5d' changed define value to 512,
So both were missed to update with new size.

[...]

Patch 2 applied to powerpc/fixes.

[2/2] powerpc/xmon: use KSYM_NAME_LEN in array size
      https://git.kernel.org/powerpc/c/719dfd5925e186e09a2a6f23016936ac436f3d78

cheers

On 30/5/23 10:54 pm, Miguel Ojeda wrote:

Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being
used, but the name seems discarded? Can
`kallsyms_lookup_size_offset()` be used instead, thus avoiding the
usage of the buffer there to begin with?

I'm not familiar with the kallsyms infrastructure, but looking over the 
implementations of kallsyms_lookup() and kallsyms_lookup_size_offset() 
it looks like the existing kallsyms_lookup()
  handles an extra case over kallsyms_lookup_size_offset()?

kallsyms_lookup_buildid() (the implementation of kallsyms_lookup()) has

   /* See if it's in a module or a BPF JITed image. */
   ret = module_address_lookup(addr, symbolsize, offset,
                               modname, modbuildid, namebuf);
   if (!ret)
           ret = bpf_address_lookup(addr, symbolsize,
                                    offset, modname, namebuf);

   if (!ret)
           ret = ftrace_mod_address_lookup(addr, symbolsize,
                                           offset, modname, namebuf);

while kallsyms_lookup_size_offset() is missing the ftrace case

   return !!module_address_lookup(addr, symbolsize, offset,
                                  NULL, NULL, namebuf) ||
          !!__bpf_address_lookup(addr, symbolsize, offset, namebuf);

Might this be a concern for xmon?