[PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[PATCH 1/1] arch:hexagon/powerpc: 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 and 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")
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/hexagon/kernel/traps.c | 2 +-
 arch/powerpc/xmon/xmon.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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;
 
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/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Miguel Ojeda]

On Mon, May 29, 2023 at 7:44 AM 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 powerpc and 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")
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

Thanks for this!

There is no `From:` at the top. Since I cannot locate the patch in
Lore, did you mean to put both of you as authors perhaps? In that
case, please use a `Co-developed-by` as needed.

Perhaps it is a good idea to submit each arch independently, too.

The changes themselves look fine on a quick inspection, though the
`xmon.c` one is a global buffer (and there is another equally-sized
buffer in `xmon.c` with a hard-coded `128` constant that would be nice
to clarify).

Cheers,
Miguel

RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Maninder Singh]

Hi,

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

> Thanks for this!
> 
> There is no `From:` at the top. Since I cannot locate the patch in
> Lore, did you mean to put both of you as authors perhaps? In that
> case, please use a `Co-developed-by` as needed.
> 

I Will add co-developed-by` tag.
because this change was identified while we were working on kallsyms some time back.
https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/

this patch set is pending and we will start working on that again, so i thought better
to send bugfix first.

> Perhaps it is a good idea to submit each arch independently, too.
> 

ok, I will share 2 separate patches.

> The changes themselves look fine on a quick inspection, though the
> `xmon.c` one is a global buffer (and there is another equally-sized
> buffer in `xmon.c` with a hard-coded `128` constant that would be nice
> to clarify).

Yes, I think second buffer was not related to kallsyms, so I have not touched that.

Thanks,
Maninder Singh

Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Miguel Ojeda]

On Mon, May 29, 2023 at 1:08 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> I Will add co-developed-by` tag.
> because this change was identified while we were working on kallsyms some time back.
> https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
>
> this patch set is pending and we will start working on that again, so i thought better
> to send bugfix first.

Sounds good to me!

(Fixed Wedson's email address)

> Yes, I think second buffer was not related to kallsyms, so I have not touched that.

Kees: what is the current stance on `[static N]` parameters? Something like:

    const char *kallsyms_lookup(unsigned long addr,
                                unsigned long *symbolsize,
                                unsigned long *offset,
    -                           char **modname, char *namebuf);
    +                           char **modname, char namebuf[static
KSYM_NAME_LEN]);

makes the compiler complain about cases like these (even if trivial):

    arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
        contains 128 elements, callee requires at least 512
[-Werror,-Warray-bounds]
            name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
                 ^                                           ~~~~~~
    ./include/linux/kallsyms.h:86:29: note: callee declares array
parameter as static here
            char **modname, char namebuf[static KSYM_NAME_LEN]);
                                 ^      ~~~~~~~~~~~~~~~~~~~~~~

But I only see 2 files in the kernel using `[static N]` (from 2020 and
2021). Should something else be used instead (e.g. `__counted_by`),
even if constexpr-sized?.

Also, I went through the other callers to `kallsyms_lookup` to see
other issues -- one I am not sure about is `fetch_store_symstring` in
`kernel/trace/trace_probe_tmpl.h`. Steven/Masami: is that "with max
length" in the function docs enough? Is it 0xffff?

Thanks!

Cheers,
Miguel

Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Petr Mladek]

On Mon 2023-05-29 16:50:45, Miguel Ojeda wrote:
> On Mon, May 29, 2023 at 1:08 PM Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> > I Will add co-developed-by` tag.
> > because this change was identified while we were working on kallsyms some time back.
> > https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/
> >
> > this patch set is pending and we will start working on that again, so i thought better
> > to send bugfix first.
> 
> Sounds good to me!
> 
> (Fixed Wedson's email address)
> 
> > Yes, I think second buffer was not related to kallsyms, so I have not touched that.
> 
> Kees: what is the current stance on `[static N]` parameters? Something like:
> 
>     const char *kallsyms_lookup(unsigned long addr,
>                                 unsigned long *symbolsize,
>                                 unsigned long *offset,
>     -                           char **modname, char *namebuf);
>     +                           char **modname, char namebuf[static
> KSYM_NAME_LEN]);
> 
> makes the compiler complain about cases like these (even if trivial):
> 
>     arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
>         contains 128 elements, callee requires at least 512
> [-Werror,-Warray-bounds]
>             name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
>                  ^                                           ~~~~~~
>     ./include/linux/kallsyms.h:86:29: note: callee declares array
> parameter as static here
>             char **modname, char namebuf[static KSYM_NAME_LEN]);
>                                  ^      ~~~~~~~~~~~~~~~~~~~~~~
> 
> But I only see 2 files in the kernel using `[static N]` (from 2020 and
> 2021). Should something else be used instead (e.g. `__counted_by`),
> even if constexpr-sized?.
> 
> Also, I went through the other callers to `kallsyms_lookup` to see
> other issues -- one I am not sure about is `fetch_store_symstring` in
> `kernel/trace/trace_probe_tmpl.h`. Steven/Masami: is that "with max
> length" in the function docs enough? Is it 0xffff?

The best solution would be to pass the buffer size as an extra
parameter. Especially when some code passes buffers that are
allocated/reserved dynamically.

Sigh, I am not sure how many changes it would require in kallsyms
API and all the callers. But it would be really appreciated, IMHO.

Best Regards,
Petr

RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Maninder Singh]

Hi Peter,

>
> The best solution would be to pass the buffer size as an extra
> parameter. Especially when some code passes buffers that are
> allocated/reserved dynamically.
> 
> Sigh, I am not sure how many changes it would require in kallsyms
> API and all the callers. But it would be really appreciated, IMHO.
> 

yes we already prepared size changes 5-6 months back:

https://lore.kernel.org/lkml/YonTOL4zC4CytVrn@infradead.org/t/

[PATCH 1/5] kallsyms: pass buffer size in sprint_* APIs

But at that time  new API development(for replacement of seq_buf) was in progress and we decided to wait for that completion.

https://lore.kernel.org/r/20220604193042.1674951-2-kent.overstreet@gmail.com

https://lore.kernel.org/r/20220604193042.1674951-4-kent.overstreet@gmail.com

As I checeked these APIs are not pushed to mainline.

we will try to prepare new patch set for kallsym changes again 
with seq_buf to take care of length argument.

Thanks,
Maninder Singh

Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Kees Cook]

On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote:
> Kees: what is the current stance on `[static N]` parameters? Something like:
> 
>     const char *kallsyms_lookup(unsigned long addr,
>                                 unsigned long *symbolsize,
>                                 unsigned long *offset,
>     -                           char **modname, char *namebuf);
>     +                           char **modname, char namebuf[static KSYM_NAME_LEN]);
> 
> makes the compiler complain about cases like these (even if trivial):
> 
>     arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
>         contains 128 elements, callee requires at least 512
> [-Werror,-Warray-bounds]
>             name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
>                  ^                                           ~~~~~~
>     ./include/linux/kallsyms.h:86:29: note: callee declares array
> parameter as static here
>             char **modname, char namebuf[static KSYM_NAME_LEN]);
>                                  ^      ~~~~~~~~~~~~~~~~~~~~~~

Wouldn't that be a good thing? (I.e. complain about the size mismatch?)

> But I only see 2 files in the kernel using `[static N]` (from 2020 and
> 2021). Should something else be used instead (e.g. `__counted_by`),
> even if constexpr-sized?.

Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't
based too often, rather structs containing them.

But ultimately, yeah, everything could gain __counted_by and friends in
the future.

-- 
Kees Cook

Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size

[Miguel Ojeda]

On Wed, May 31, 2023 at 1:14 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote:
> > Kees: what is the current stance on `[static N]` parameters? Something like:
> >
> >     const char *kallsyms_lookup(unsigned long addr,
> >                                 unsigned long *symbolsize,
> >                                 unsigned long *offset,
> >     -                           char **modname, char *namebuf);
> >     +                           char **modname, char namebuf[static KSYM_NAME_LEN]);
> >
> > makes the compiler complain about cases like these (even if trivial):
> >
> >     arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small;
> >         contains 128 elements, callee requires at least 512
> > [-Werror,-Warray-bounds]
> >             name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
> >                  ^                                           ~~~~~~
> >     ./include/linux/kallsyms.h:86:29: note: callee declares array
> > parameter as static here
> >             char **modname, char namebuf[static KSYM_NAME_LEN]);
> >                                  ^      ~~~~~~~~~~~~~~~~~~~~~~
>
> Wouldn't that be a good thing? (I.e. complain about the size mismatch?)

Yeah, I would say so (i.e. I meant it as a good thing).

> > But I only see 2 files in the kernel using `[static N]` (from 2020 and
> > 2021). Should something else be used instead (e.g. `__counted_by`),
> > even if constexpr-sized?.
>
> Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't
> based too often, rather structs containing them.
>
> But ultimately, yeah, everything could gain __counted_by and friends in
> the future.

That would be nice!

Cheers,
Miguel