linux-kernel - Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

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: <aF6md8uMuD8EpXvd@pluto>
Date: Fri, 27 Jun 2025 15:11:03 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Peng Fan <peng.fan@....com>
Cc: Sudeep Holla <sudeep.holla@....com>,
	Cristian Marussi <cristian.marussi@....com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, arm-scmi@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog and system info
> 
> Signed-off-by: Peng Fan <peng.fan@....com>
> ---
>  drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>  	return 0;
>  }
>  
> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct scmi_imx_misc_sys_sleep_rec *rec;
> +	struct scmi_imx_misc_syslog *syslog;
> +	int ret;
> +	size_t len = 0;
> +
> +	if (!ph)
> +		return 0;
> +
> +	syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> +	if (!syslog)
> +		return -ENOMEM;
> +
> +	ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);

...ah...so you basically cast to void a structure of u32 words and then
expect that the firmware perfectly aligned with how the struct is
defined here....size checks assures no out-of-bounds BUT the meaning of
he blob itself is entirely up to FW and kernel being aligned, since no
type checking is done of any kind and fields are NOT reference by
name...so may I ask why ? ..also because the scmi_imx_misc_syslog seems
pretty tiny to need iterators to parse back the reply...do you have so
tiny transpotr message size ?

..anyway I would suggest at least to add some sort of version-field to
the struct so that at least you can detect if the FW spits out something
that is not what your driver expect or is ready to handle...especially
if you plan to extend or modify the layout of the structs in the future.


> +	if (ret) {
> +		kfree(syslog);
> +		return ret;
> +	}
> +
> +	rec = &syslog->syssleeprecord;
> +
> +	len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> +	len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> +	len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> +	len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> +	len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> +	len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> +	len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> +	len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> +	len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> +

... how wonder how much is still frowned upon to serve such big multiline
structured information payloads from sysfs ... (asking for a friend :P)


> +	kfree(syslog);
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(syslog);
> +
> +static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct scmi_imx_misc_system_info *info;
> +	int len = 0;
> +	int ret;
> +
> +	if (!ph)
> +		return 0;
> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
> +	if (ret)
> +		goto err;
> +
> +	len += sysfs_emit_at(buf, len, "SM Version    = Build %u, Commit 08%x\n",
> +			     info->buildnum, info->buildcommit);
> +	len += sysfs_emit_at(buf, len, "SM Config     = %s, mSel=%u\n",
> +			     info->cfgname, info->msel);
> +	len += sysfs_emit_at(buf, len, "Silicon       = %s\n", info->siname);
> +	len += sysfs_emit_at(buf, len, "Board         = %s, attr=0x%08x\n",
> +			     info->brdname, info->brd_attributes);

...so some of this stuff Build/Silicon/BoaRD info has pretty much
'forever' lifetime...are you sure you want to query them out of the FW
each time you issue a sysfs show ?

Cannot you simply dump this stuff once for all at probve time and
instead query misc_cfg_info() upon each show to refresh only the data
that will change ?

(this also corroborates my idea that you should somehow partition this
data into distinct structs by their lifetime...

Thanks,
Cristian