Re: [PATCH 6/9] nvmem: imx-ocotp-ele: Support the ELE API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The dynamically allocated read buffer `p` is leaked when `imx_se_read_fuse()` returns an error in `imx_ocotp_reg_read()`.
- [High] Lack of unbind protection for the supplier Secure Enclave (`se_dev`) device leads to a potential Use-After-Free of its cached private data.
--

commit ce01a1e44e5619e1342b66aca8b1bf267d01e93e
Author: Frieder Schrempf <frieder.schrempf@kontron.de>

nvmem: imx-ocotp-ele: Support the ELE API

The fuses inside the Edgelock Secure Enclave are currently not accessed via
its API but through the FSB block which provides limited access to some
fuses. The ELE API allows us to access all fuses with read/write
permissions.

> diff --git a/drivers/nvmem/imx-ocotp-ele.c b/drivers/nvmem/imx-ocotp-ele.c
> index 9d3f94e35508f..92cc061b9b347 100644
> --- a/drivers/nvmem/imx-ocotp-ele.c
> +++ b/drivers/nvmem/imx-ocotp-ele.c

[ ... ]

> @@ -92,6 +97,19 @@ static int imx_ocotp_reg_read(void *context, unsigned int offset, void *val, siz
>  	buf = p;
>  
>  	for (i = index; i < (index + count); i++) {
> +		/*
> +		 * All fuse registers can be read via ELE. If the SE device is
> +		 * available, always prefer it.
> +		 */
> +		if (priv->se_data) {
> +			ret = imx_se_read_fuse(priv->se_data, i, buf++);
> +			if (ret) {
> +				mutex_unlock(&priv->lock);
> +				return ret;

[Severity: High]
Does this error path leak the memory allocated for p earlier in the function?

The allocation happens here:

	p = kzalloc(num_bytes, GFP_KERNEL);
	if (!p)
		return -ENOMEM;

Returning directly here will bypass the kfree(p) at the end of the function.

> +			}
> +			continue;
> +		}
> +

[ ... ]

> @@ -152,16 +203,36 @@ static int imx_ele_ocotp_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->base))
>  		return PTR_ERR(priv->base);
>  
> +	np = of_parse_phandle(pdev->dev.of_node, "secure-enclave", 0);
> +	if (!np) {
> +		dev_info(&pdev->dev, "missing or invalid SE handle, using readonly FSB\n");
> +	} else {
> +		priv->se_dev = of_find_device_by_node(np);
> +		of_node_put(np);
> +		if (!priv->se_dev)
> +			return dev_err_probe(&pdev->dev, -ENODEV, "failed to find SE device\n");
> +
> +		ret = devm_add_action_or_reset(&pdev->dev, imx_ocotp_put_se_dev,
> +					       priv->se_dev);
> +		if (ret)
> +			return ret;
> +
> +		priv->se_data = platform_get_drvdata(priv->se_dev);

[Severity: High]
Is there anything preventing the SE driver from being unbound via sysfs after we
cache its private data in priv->se_data here?

If the SE driver is unbound, its devres allocations (which likely include the
se_data) would be freed. Since we haven't established a device link
(e.g. using device_link_add) to enforce unbind ordering, we might be left
with a dangling pointer in priv->se_data.

Could this result in a use-after-free when imx_ocotp_reg_read() or
imx_ocotp_reg_write() later attempts to dereference priv->se_data?

> +		if (!priv->se_data)
> +			return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +					     "SE device not ready\n");
> +	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-upstreaming-next-20260609-imx-ocotp-ele-v1-0-cb7f3698c3e6@kontron.de?part=6