linux-kernel - RE: [PATCH v2] 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]

Date:   Tue, 15 Mar 2022 16:17:40 +0530
From:   Maninder Singh <maninder1.s@...sung.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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>,
        "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.org" <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>
Subject: RE: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is
 disabled

Hi,

> > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id,
> > +			    int backtrace, int symbol)
> > +{
> > +	if (backtrace)
> > +		return __sprint_symbol(buffer, address, -1, 1, build_id);
> 
> > +	else if (symbol)
> > +		return __sprint_symbol(buffer, address, 0, 1, build_id);
> > +	else
> > +		return __sprint_symbol(buffer, address, 0, 0, 0);
> 
> Redundant 'else' in both cases.
>

Ok, will change it

> > +}
> 
> ...
> 
> > +static int sprint_module_info(char *buf, char *end, unsigned long value,
> > +			     const char *fmt, int modbuildid, int backtrace, int symbol)
> 
> fmt is not used.

Yes, did not notice it.(will remove both end and gmt)

> > +{
> > +	struct module *mod;
> > +	unsigned long offset = 0;
> 
> > +	unsigned long base;
> 
> Can it be the same type as core_layout.base? Why not?
>
> > +	char *modname;
> > +	int len;
> > +	const unsigned char *buildid = NULL;
> > +
> > +	if (is_ksym_addr(value))
> > +		return 0;
> > +
> > +	if (backtrace || symbol)
> > +		offset = 1;
> 
> I would expect here to have
> 
> 	else
> 		offset = 0;
> 
> But see below.
> 
> > +	preempt_disable();
> > +	mod = __module_address(value);
> > +	if (mod) {
> > +		modname = mod->name;
> > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
> > +		if (modbuildid)
> > +			buildid = mod->build_id;
> > +#endif
> 
> > +		if (offset) {
> 
> This seems quite confusing because semantically you use offset as a boolean
> flag and offset. Why not add a boolean variable with a clear name?
>

Ok, will add 2 separate variables.

> > +			base = (unsigned long)mod->core_layout.base;
> > +			offset = value - base;
> > +		}
> > +	}
> 
> > +
> 
> Probably you can drop this blank line to group entire critical section,
> or add a blank line above.
> 
> > +	preempt_enable();
> > +	if (!mod)
> > +		return 0;
> > +
> > +	/* address belongs to module */
> > +	if (offset)
> > +		len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset);
> > +	else
> 
> > +		len = sprintf(buf, "0x%lx", (void *)value);
> 
> What this casting is for? Don't you have a compilation warning?

My Bad, earlier I made patch with hashing this value also (%p), but after that
changed it to %lx to have same original behavior in case of %ps, forgot to update final patch
to remove typecast.

> 
> > +	len += fill_name_build_id(buf, modname, modbuildid, buildid, len);
> > +	return len;
> 
> 	return len + ...;
> 
> ?
> 
> > +}

Will modify patch with all changes.

Thanks,
Maninder Singh
Download attachment "rcptInfo.txt" of type "application/octet-stream" (1664 bytes)