| Message ID | 20230529052821.58175-1-maninder1.s@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
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
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
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
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
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
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.
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
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];
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(-)