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 <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
---
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
On Mon, May 29, 2023 at 7:44 AM Maninder Singh <[email protected]> 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 <[email protected]>
> Signed-off-by: Maninder Singh <[email protected]>
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
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 <[email protected]>
>> Signed-off-by: Maninder Singh <[email protected]>
> 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/[email protected]/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
On Mon, May 29, 2023 at 1:08 PM Maninder Singh <[email protected]> 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/[email protected]/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
On Mon 2023-05-29 16:50:45, Miguel Ojeda wrote:
> On Mon, May 29, 2023 at 1:08 PM Maninder Singh <[email protected]> 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/[email protected]/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
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/[email protected]/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/[email protected]
https://lore.kernel.org/r/[email protected]
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
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
On Wed, May 31, 2023 at 1:14 AM Kees Cook <[email protected]> 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