Re: [PATCH 4/7] firmware: imx: ele: simplify SoC device registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free and resource leak due to missing soc_device_unregister().
- [High] Information leak and functional breakage in get_se_soc_id() due to incorrect struct casting.
--

commit e7ad33f52996b1ca324a005406dedb896f547398
Author: Peng Fan <peng.fan@nxp.com>

firmware: imx: ele: simplify SoC device registration

This patch simplifies the SoC device registration for i.MX by replacing
the boolean 'soc_register' flag with a 'soc_name' string. The presence
of 'soc_name' now dictates registration, removing the need for a switch
statement.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
> @@ -56,7 +56,7 @@ struct se_var_info {
>  /* contains fixed information */
>  struct se_soc_info {
>  	const u16 soc_id;
> -	const bool soc_register;
> +	const char *soc_name;
>  	const struct se_fw_img_name se_fw_img_nm;
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does get_se_soc_id()
incorrectly cast the match data and leak kernel address information?

device_get_match_data() returns a pointer to struct se_if_node, but
get_se_soc_id() assigns it to a const struct se_soc_info * pointer.

Since the first member of struct se_if_node is a pointer (se_info)
rather than a u16 soc_id, will accessing se_info->soc_id read the lower
16 bits of the kernel pointer instead of the actual SoC ID?

[ ... ]
> @@ -181,14 +181,7 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
>  						FIELD_GET(DEV_GETINFO_MAJ_VER_MASK,
>  							  var_se_info.soc_rev));
>  
> -	switch (se_info->soc_id) {
> -	case SOC_ID_OF_IMX8ULP:
> -		attr->soc_id = "i.MX8ULP";
> -		break;
> -	case SOC_ID_OF_IMX93:
> -		attr->soc_id = "i.MX93";
> -		break;
> -	}
> +	attr->soc_id = se_info->soc_name;
>  
>  	err = of_property_read_string(of_root, "model", &attr->machine);
>  	if (err)

[Severity: High]
This is a pre-existing issue, but does this function leak the soc_device
and create a use-after-free vulnerability?

The attr structure is allocated with devm_kzalloc(), meaning it is freed
when the driver unbinds. However, when soc_device_register(attr) is called
later in this function, the returned struct soc_device * is discarded.

Without calling soc_device_unregister() in error paths or driver remove,
will the sysfs nodes remain active? If a read to an attribute like
/sys/devices/soc0/revision happens after the driver is unbound, will it
dereference the freed attr memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-ele-v1-v1-0-a9570c4bffc9@nxp.com?part=4