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

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

[Maninder Singh]

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 <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 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

[PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Maninder Singh]

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 <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 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

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Michael Ellerman]

Maninder Singh <maninder1.s@samsung.com> 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

> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  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

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

[Petr Mladek]

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 <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

With the updated commit hash:

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

Best Regards,
Petr

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Miguel Ojeda]

On Mon, May 29, 2023 at 1:14 PM Maninder Singh <maninder1.s@samsung.com> 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

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

[Miguel Ojeda]

On Mon, May 29, 2023 at 1:14 PM Maninder Singh <maninder1.s@samsung.com> 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 <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

With the updated commit hash:

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

Cheers,
Miguel

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

[David Laight]

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

> 
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
> 
> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  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

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

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Michael Ellerman]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:
> On Mon, May 29, 2023 at 1:14 PM Maninder Singh <maninder1.s@samsung.com> 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?

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

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Miguel Ojeda]

On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman <mpe@ellerman.id.au> 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?
>
> 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).

> > 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

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Michael Ellerman]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:
> On Thu, Jun 1, 2023 at 4:02 AM Michael Ellerman <mpe@ellerman.id.au> 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?
>>
>> 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

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

[Michael Ellerman]

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

Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size

[Benjamin Gray]

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?