Date:   Tue, 17 May 2022 09:28:10 +0530
From:   Maninder Singh <maninder1.s@...sung.com>
To:     Kees Cook <keescook@...omium.org>
CC:     "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "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.or>,
        "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

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);


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.

otherwise there is no benefit to take care of size at some places only.

> - duplicated logic (maybe? hard to review due to the changes)

We tried to avoid all duplicacies, but will check to clean patch more.
 
> Details below...
> 
> > 
..
> > 
> > (C) With patched kernel
> > with KALLYSMS:
> > [41.974576] ps function_1 [crash]
> > [41.975173] pS function_1+0x4/0x2c [crash]
> > [41.975386] pSb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee]
> > [41.975879] pB function_1+0x4/0x2c [crash]
> > [41.976076] pBb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee]
> > 
> > without KALLSYMS:
> > [9.624152] ps 0xffff800001bd008c [crash]	// similar to original, no changes
> 
> Why is this not hashed?
>

Because without patch also we print complete address in case of %ps,
only addition is module name now.

if we made it to hashed, then I will make one patch after this to hash it
as Andy told it will change current behaviour.
 
> > [9.624548] pS 0x(____ptrval____)+0x8c [crash]   // base address hashed and offset is without hash
> > [9.624847] pSb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee]
> > [9.625388] pB 0x(____ptrval____)+0x8c [crash]
> > [9.625594] pBb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee]
> 
> Why is "____ptrval____" visible here after 9 seconds of boot? I would
> expect a hashed value to be present.
> 

Actually I verified it on QEMU setup with minimal roots and in this case
hashed values were not present.

I tested it on my target;s old kernel for actual hash values.
But pasted the logs of latets kernel only with __ptr___

> > 
> > with disable hashing:
> > [8.563916] ps 0xffff800000f2008c [crash]
..
> >  
> >  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);
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 46d4d5f2516e..66f4491249c5 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -682,6 +682,20 @@ static inline bool is_livepatch_module(struct module *mod)
> >  
> >  void set_module_sig_enforced(void);
> >  
> > +static inline int fill_name_build_id(char *buffer, char *modname,
> > +			    int add_buildid, const unsigned char *buildid,
> > +			    int len)
> > +{
> > +	len += sprintf(buffer + len, " [%s", modname);
> 
> This patch uses sprintf() everywhere. This needs to be using
> scnprintf(). https://lwn.net/Articles/69419/
> 

These were already present .
Moved sprintf from __sprint_symbol to new function fill_name_build_id

and same logic added when KALLSYMS is disabled.

new API sprint_module_info added 2 new sprintf, which can be taken care of with
passing size of buffer.

But as already explained either we should take care of all APIs with one separate patch.
or we can allow these 2 sprintfs also to make same dsign with KALLSYMS.



> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 79f2eb617a62..e6e96b2e0257 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -465,19 +465,8 @@ static int __sprint_symbol(char *buffer, unsigned long address,
> >  	if (add_offset)
> >  		len += sprintf(buffer + len, "+%#lx/%#lx", offset, size);
> >  
> > -	if (modname) {
> > -		len += sprintf(buffer + len, " [%s", modname);
> > -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> > -		if (add_buildid && buildid) {
> > -			/* build ID should match length of sprintf */
> > -#if IS_ENABLED(CONFIG_MODULES)
> > -			static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
> > -#endif
> > -			len += sprintf(buffer + len, " %20phN", buildid);
> > -		}
> > -#endif
> > -		len += sprintf(buffer + len, "]");
> > -	}
> > +	if (modname)
> > +		len += fill_name_build_id(buffer, modname, add_buildid, buildid, len);
> 
> Build ID is now part of sprint_module_info?
> 

It was part of sprint_module_info earlier also.
just modular the code with new API fill_name_build_id()
to call it in case of disabled KALLSYMS also.

> >  
> >  	return len;
> >  }
> > @@ -572,6 +561,18 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
> >  	return __sprint_symbol(buffer, address, -1, 1, 1);
> >  }
> >  
> > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id,
> > +			    int backtrace, int symbol)
> 
> I find the build_id argument order unexpected: I'd think it'd match the
> ordering of __sprint_symbol?
> 
> > +{
> > +	if (backtrace)
> > +		return __sprint_symbol(buffer, address, -1, 1, build_id);
> > +
> > +	if (symbol)
> > +		return __sprint_symbol(buffer, address, 0, 1, build_id);
> > +
> > +	return __sprint_symbol(buffer, address, 0, 0, 0);
> > +}
> > +
> >  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
> >  struct kallsym_iter {
> >  	loff_t pos;
..

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 40d26a07a133..e0aba2c80b8e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -999,33 +999,92 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
> >  }
> >  #endif
> >  
..
> > +	preempt_enable();
> > +	if (!mod)
> > +		return 0;
> > +
> > +	/* address belongs to module */
> > +	if (add_offset)
> > +		len = sprintf(buf, "0x%p+0x%lx", base, offset);
> > +	else
> > +		len = sprintf(buf, "0x%lx", value);
> > +
> > +	return len + fill_name_build_id(buf, modname, modbuildid, buildid, len);
> 
> Doesn't this duplicate a bunch of logic elsewhere?
>

Yes little bit similar code in kallsyms.c for adding offset but purpose is different.
in 1 we need to hash.

but will check more if we can make some clean patch set with 2-3 patches rather than from 1 single patch change.

 
> > +}
> > +#else
> > +static inline int sprint_module_info(char *buf, unsigned long value,
> > +			     int modbuildid, int backtrace, int symbol)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static noinline_for_stack
> >  char *symbol_string(char *buf, char *end, void *ptr,
> >  		    struct printf_spec spec, const char *fmt)
> >  {
> >  	unsigned long value;
> > -#ifdef CONFIG_KALLSYMS
> >  	char sym[KSYM_SYMBOL_LEN];
> > -#endif
> > +	int backtrace = 0, symbol = 0, build_id = 0;
> >  
> >  	if (fmt[1] == 'R')
> >  		ptr = __builtin_extract_return_addr(ptr);
> >  	value = (unsigned long)ptr;
> >  
> > -#ifdef CONFIG_KALLSYMS
> > -	if (*fmt == 'B' && fmt[1] == 'b')
> > -		sprint_backtrace_build_id(sym, value);
> > -	else if (*fmt == 'B')
> > -		sprint_backtrace(sym, value);
> > -	else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b')))
> > -		sprint_symbol_build_id(sym, value);
> > -	else if (*fmt != 's')
> > -		sprint_symbol(sym, value);
> > -	else
> > -		sprint_symbol_no_offset(sym, value);
> > +	if (fmt[0] == 'B' && fmt[1] == 'b') {
> > +		backtrace = 1;
> > +		build_id = 1;
> > +	} else if (fmt[0] == 'B')
> > +		backtrace = 1;
> > +	else if (fmt[0] == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) {
> > +		symbol = 1;
> > +		build_id = 1;
> > +	} else if (fmt[0] != 's')
> > +		symbol = 1;
> > +	else {
> > +		/* Do Nothing, no offset */
> > +	}
> >  
> > +#ifdef CONFIG_KALLSYMS
> > +	sprint_kallsym_common(sym, value, build_id, backtrace, symbol);
> >  	return string_nocheck(buf, end, sym, spec);
> >  #else
> > +	if (sprint_module_info(sym, value, build_id, backtrace, symbol))
> > +		return string_nocheck(buf, end, sym, spec);
> 
> These take the same arguments, and only differ about their ability to
> look things up.
> 

Yes, Will make both API with same name, it will clean the code more.

> > +
> >  	return special_hex_number(buf, end, value, sizeof(void *));
> >  #endif
> >  }
> > -- 
> > 2.17.1
> > 
> 
> -- 

Thanks for input, please suggest about sprintf changes, then I will send
new patches with more clean code.

----
Maninder Singh

Powered by blists - more mailing lists