On Tue 2022-05-17 09:28:10, Maninder Singh wrote:

Hi Kees,

quoted

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.

quoted

- 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