Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 May 2022 16:35:25 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     Maninder Singh <maninder1.s@samsung.com>
Cc:     Kees Cook <keescook@chromium.org>,
        "mcgrof@kernel.org" <mcgrof@kernel.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "senozhatsky@chromium.org" <senozhatsky@chromium.org>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "linux@rasmusvillemoes.dk" <linux@rasmusvillemoes.dk>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
        Vaneet Narang <v.narang@samsung.com>,
        "swboyd@chromium.org" <swboyd@chromium.org>,
        "ojeda@kernel.or" <ojeda@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "avimalin@gmail.com" <avimalin@gmail.com>,
        "atomlin@redhat.com" <atomlin@redhat.com>,
        Onkarnath <onkarnath.1@samsung.com>
Subject: Re: [PATCH modules-next 1/1] kallsyms: enhance %pS/s/b printing when
 KALLSYSMS is disabled
Message-ID: <YoOykGzkWB+k7Las@alley>
References: <202205111513.3AD646936@keescook>
 <20220421041542.9195-1-maninder1.s@samsung.com>
 <CGME20220421041604epcas5p414cf2e851d1bd828a151dc6b2b004680@epcms5p3>
 <20220517035810epcms5p3f7dc52887e356559b55a2cbf9d04a039@epcms5p3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220517035810epcms5p3f7dc52887e356559b55a2cbf9d04a039@epcms5p3>
Precedence: bulk

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