Re: [PATCH modules-next 1/1] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled

[Petr Mladek <pmladek@suse.com>]

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