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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YoOykGzkWB+k7Las@alley>
Date:   Tue, 17 May 2022 16:35:25 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Maninder Singh <maninder1.s@...sung.com>
Cc:     Kees Cook <keescook@...omium.org>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "senozhatsky@...omium.org" <senozhatsky@...omium.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
        Vaneet Narang <v.narang@...sung.com>,
        "swboyd@...omium.org" <swboyd@...omium.org>,
        "ojeda@...nel.or" <ojeda@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "avimalin@...il.com" <avimalin@...il.com>,
        "atomlin@...hat.com" <atomlin@...hat.com>,
        Onkarnath <onkarnath.1@...sung.com>
Subject: Re: [PATCH modules-next 1/1] kallsyms: enhance %pS/s/b printing when
 KALLSYSMS is disabled

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