linux-kernel - Re: [PATCH 3/5] panic: add 'panic_sys_info' sysctl to take human readable string parameter

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]

Message-ID: <aJsWCxc42f2Qjbs3@pathway>
Date: Tue, 12 Aug 2025 12:23:07 +0200
From: Petr Mladek <pmladek@...e.com>
To: Feng Tang <feng.tang@...ux.alibaba.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Lance Yang <lance.yang@...ux.dev>, Jonathan Corbet <corbet@....net>,
	linux-kernel@...r.kernel.org, paulmck@...nel.org,
	john.ogness@...utronix.de
Subject: Re: [PATCH 3/5] panic: add 'panic_sys_info' sysctl to take human
 readable string parameter

On Thu 2025-07-03 10:10:02, Feng Tang wrote:
> Bitmap definition for 'panic_print' is hard to remember and decode.
> Add 'panic_sys_info='sysctl to take human readable string like
> "tasks,mem,timers,locks,ftrace,..." and translate it into bitmap.
> 
> The detailed mapping is:
> 	SYS_INFO_TASKS		"tasks"
> 	SYS_INFO_MEM		"mem"
> 	SYS_INFO_TIMERS		"timers"
> 	SYS_INFO_LOCKS		"locks"
> 	SYS_INFO_FTRACE		"ftrace"
> 	SYS_INFO_ALL_CPU_BT	"all_bt"
> 	SYS_INFO_BLOCKED_TASKS	"blocked_tasks"
> 
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> +static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> +
> +int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> +					  void *buffer, size_t *lenp,
> +					  loff_t *ppos)
> +{
> +	char names[sizeof(sys_info_avail) + 1];

The "+ 1" looks superfluous.

I guess that it is for the trailing '\0'. But sys_info_avail[] already
includes the trailing '\0' so it should be already counted by the sizeof().

Note that it would be needed with strlen(). But it should not be
needed with sizeof().

> +	struct ctl_table table;
> +	unsigned long *si_bits_global;
> +
> +	si_bits_global = ro_table->data;
> +
> +	if (write) {
> +		unsigned long si_bits;
> +		int ret;
> +
> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		ret = proc_dostring(&table, write, buffer, lenp, ppos);
> +		if (ret)
> +			return ret;
> +
> +		si_bits = sys_info_parse_param(names);
> +		/* The access to the global value is not synchronized. */
> +		WRITE_ONCE(*si_bits_global, si_bits);
> +		return 0;
> +	} else {
> +		/* for 'read' operation */
> +		char *delim = "";
> +		int i, len = 0;
> +

It looks to me that names[] can later be used non-initialized when
*si_bits_global == 0. We should initialized it here, something like:

		names[0] = '\0';

> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (*si_bits_global & si_names[i].bit) {
> +				len += scnprintf(names + len, sizeof(names) - len,
> +					"%s%s", delim, si_names[i].name);
> +				delim = ",";
> +			}
> +		}
> +
> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		return proc_dostring(&table, write, buffer, lenp, ppos);
> +	}
> +}
> +#endif

Otherwise, it looks good.

Best Regards,
Petr