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

[Frieder Schrempf <frieder.schrempf@kontron.de>]

On 16.06.26 14:04, sashiko-bot@kernel.org wrote:
> 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.

Will be fixed using __free(kfree) in v2.

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

Will be fixed by creating a device link in v2.

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