Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
On Wed, May 31, 2023 at 1:14 AM Kees Cook 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, , , 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
Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
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, , , 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
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/yontol4zc4cyt...@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.overstr...@gmail.com https://lore.kernel.org/r/20220604193042.1674951-4-kent.overstr...@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
On Mon 2023-05-29 16:50:45, Miguel Ojeda wrote: > On Mon, May 29, 2023 at 1:08 PM Maninder Singh > 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/yontol4zc4cyt...@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, , , 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 0x? 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
On Mon, May 29, 2023 at 1:08 PM Maninder Singh 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/yontol4zc4cyt...@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, , , 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 0x? Thanks! Cheers, Miguel
RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
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
>> Signed-off-by: Maninder Singh
> 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/yontol4zc4cyt...@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
On Mon, May 29, 2023 at 7:44 AM 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 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
> Signed-off-by: Maninder Singh
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
