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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Mar 2022 12:30:46 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Maninder Singh <maninder1.s@...sung.com>
Cc:     mcgrof@...nel.org, pmladek@...e.com, rostedt@...dmis.org,
        senozhatsky@...omium.org, linux@...musvillemoes.dk,
        akpm@...ux-foundation.org, wangkefeng.wang@...wei.com,
        v.narang@...sung.com, swboyd@...omium.org, ojeda@...nel.org,
        linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org,
        avimalin@...il.com, atomlin@...hat.com
Subject: Re: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is
 disabled

On Tue, Mar 15, 2022 at 03:21:12PM +0530, Maninder Singh wrote:
> print module information when KALLSYMS is disabled.
> 
> No change for %pB, as it needs to know symbol name to adjust address
> value which can't be done without KALLSYMS.

...

> +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.

> +}

...

> +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.

> +{
> +	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?

> +			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?

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

	return len + ...;

?

> +}

-- 
With Best Regards,
Andy Shevchenko