On Tue 2022-05-17 09:28:10, Maninder Singh wrote:
> Hi Kees,
>
> >
> > I'd like this patch reverted from -next.
> > - too many logical changes is a single patch
>
> ok, will try to break patch in separate patches.
>
> > - addition of dangerous API usage
>
> sprintf was alraedy there, just changed its position
> and in current logic it seems not possible to change it.
>
> Because sprint_symbol interface is made without len of array
>
> int sprint_symbol(char *buffer, unsigned long address)
> {
> return __sprint_symbol(buffer, address, 0, 1, 0);
> }
> EXPORT_SYMBOL_GPL(sprint_symbol);
Sigh, the kallsyms API is not safe in general. For example,
the following functions have the same problem:
unsigned long kallsyms_lookup_name(const char *name);
const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
char **modname, char *namebuf);
int lookup_symbol_name(unsigned long addr, char *symname);
int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
> So either we need to change API declaration for all cases.
> please suggest, then I can make one separate change to include
> size of buffer as argument.
It would be lovely if you could fix the API.
> otherwise there is no benefit to take care of size at some places only.
Well, the sprint_*() APIs are more dangerous because the underlying
__sprint_symbol() could do several sprintf() calls. It is not easy
to compute the sufficient buffer size.
The *lookup*() APIs are slightly more safe because the buffer is
just for the symbol name. The size always should be KSYM_NAME_LEN.
Anyway, it would be great to make it safe as well.
Best Regards,
Petr
