Re: [PATCH v14 3/9] remoteproc: qcom: add hexagon based WCSS secure PIL driver

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

New issues:
- [High] Teardown asymmetry between `devm_rproc_add` and manual `remove` cleanup causes subdevices to skip shutdown routines and causes lockless list corruption.
- [High] `wcss_sec_copy_segment` silently returns on `ioremap_wc` failure, corrupting the devcoredump with uninitialized memory or zeroes.
- [Medium] Permanent `ioremap_wc` mapping leak during coredump segment reading due to unreachable unmap condition.
- [Medium] Error paths in `wcss_sec_dump_segments` permanently leak previously added custom coredump segments.
- [Medium] Missing NULL check on `of_device_get_match_data` during driver probe.

Pre-existing issues:
- [High] Unconditionally calling `qcom_remove_ssr_subdev()` on uninitialized subdev causes a kernel panic if `qcom_add_ssr_subdev()` failed silently.
--

--- Patch [3]: [PATCH v14 3/9] remoteproc: qcom: add hexagon based WCSS secure PIL driver ---
commit f96fb4a46b22ed73d132da71366a2ebbfe51729c
Author: Vignesh Viswanathan <[email protected]>
Subject: remoteproc: qcom: add hexagon based WCSS secure PIL driver

Adds support for a Hexagon-based WCSS secure PIL driver for IPQxxxx SoCs
using TrustZone (TZ) for authentication and loading.

> diff --git a/drivers/remoteproc/qcom_q6v5_wcss_sec.c b/drivers/remoteproc/qcom_q6v5_wcss_sec.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_q6v5_wcss_sec.c
[ ... ]
> +static void wcss_sec_copy_segment(struct rproc *rproc,
> +				  struct rproc_dump_segment *segment,
> +				  void *dest, size_t offset, size_t size)
> +{
> +	struct wcss_sec *wcss = rproc->priv;
> +	struct device *dev = wcss->dev;
> +
> +	if (!segment->io_ptr)
> +		segment->io_ptr = ioremap_wc(segment->da, segment->size);
> +
> +	if (!segment->io_ptr) {
> +		dev_err(dev, "Failed to ioremap segment %pad size 0x%zx\n",
> +			&segment->da, segment->size);
> +		return;

[Severity: High]
If ioremap_wc() fails, this returns early without populating the userspace
buffer. Because it returns void, the devcoredump core assumes the copy
succeeded. 

Does this cause the framework to leak uninitialized kernel memory
(if not zeroed) or zeroes to userspace, silently corrupting the coredump
instead of gracefully failing?

> +	}
> +
> +	if (offset + size <= segment->size) {
> +		memcpy_fromio(dest, segment->io_ptr + offset, size);
> +	} else {
> +		iounmap(segment->io_ptr);
> +		segment->io_ptr = NULL;
> +	}

[Severity: Medium]
Because the remoteproc framework bounds the read size such that 
offset + size <= segment->size is always true, is this else branch
mathematically dead code?

During cleanup, rproc_coredump_cleanup() frees the segment structure but
lacks a hook to unmap the custom io_ptr. Could this permanently leak vmalloc
space on every crash dump?

> +}
[ ... ]
> +static int wcss_sec_dump_segments(struct rproc *rproc,
> +				  const struct firmware *fw)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct reserved_mem *rmem = NULL;
> +	struct device_node *node;
> +	int num_segs, index;
> +	int ret;
> +
> +	/*
> +	 * Parse through additional reserved memory regions for the rproc
> +	 * and add them to the coredump segments
> +	 */
> +	num_segs = of_count_phandle_with_args(dev->of_node,
> +					      "memory-region", NULL);
> +	for (index = 0; index < num_segs; index++) {
> +		node = of_parse_phandle(dev->of_node,
> +					"memory-region", index);
> +		if (!node)
> +			return -EINVAL;
> +
> +		rmem = of_reserved_mem_lookup(node);
> +		of_node_put(node);
> +		if (!rmem) {
> +			dev_err(dev, "unable to acquire memory-region index %d num_segs %d\n",
> +				index, num_segs);
> +			return -EINVAL;

[Severity: Medium]
If an error occurs midway through the segment parsing loop, does returning
early here leave previously added segments on the rproc->dump_segments list?

The remoteproc core's rproc_fw_boot() handles a parse_fw() failure by jumping
directly to unprepare_rproc, bypassing rproc_resource_cleanup(). Will any
segments successfully added in earlier iterations be permanently leaked?

> +		}
[ ... ]
> +static int wcss_sec_probe(struct platform_device *pdev)
> +{
> +	const struct wcss_data *desc = of_device_get_match_data(&pdev->dev);
> +	const char *fw_name = NULL;
> +	struct wcss_sec *wcss;
> +	struct clk *sleep_clk;
> +	struct clk *int_clk;
> +	struct rproc *rproc;
> +	int ret;
> +
> +	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
> +				      &fw_name);
> +	if (ret < 0)
> +		return ret;
> +
> +	rproc = devm_rproc_alloc(&pdev->dev, desc->ss_name, &wcss_sec_ops,
> +				 fw_name, sizeof(*wcss));

[Severity: Medium]
Is a NULL check needed for the return value of of_device_get_match_data()?

If the driver is bound manually via sysfs to a platform device with a
compatible of_node that lacks match data in the driver's table, it could
return NULL and cause a NULL pointer dereference on desc->ss_name.

> +	if (!rproc) {
[ ... ]
> +	qcom_add_glink_subdev(rproc, &wcss->glink_subdev, desc->ss_name);
> +	qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, desc->ss_name);
> +
> +	rproc->auto_boot = desc->auto_boot;
> +	rproc->dump_conf = RPROC_COREDUMP_INLINE;
> +	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
> +
> +	ret = devm_rproc_add(&pdev->dev, rproc);
> +	if (ret) {
> +		qcom_remove_ssr_subdev(rproc, &wcss->ssr_subdev);

[Severity: High]
This is a pre-existing issue, but if qcom_add_ssr_subdev() fails (e.g., due
to -ENOMEM), it returns early and leaves ssr->subdev.node zero-initialized.

Later, unconditionally calling qcom_remove_ssr_subdev() here during probe
error handling (or in wcss_sec_remove() below) invokes list_del() on the
zero-initialized list head. Can this dereference a NULL prev pointer and
cause a kernel panic?

> +		qcom_remove_glink_subdev(rproc, &wcss->glink_subdev);
> +		qcom_q6v5_deinit(&wcss->q6);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	return 0;
> +}
> +
> +static void wcss_sec_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct wcss_sec *wcss = rproc->priv;
> +
> +	qcom_remove_ssr_subdev(rproc, &wcss->ssr_subdev);
> +	qcom_remove_glink_subdev(rproc, &wcss->glink_subdev);

[Severity: High]
Because rproc_del() is deferred to devres cleanup (after remove()),
does manually removing subdevices from rproc->subdevs here while the rproc
is still active skip their .stop callbacks?

Since they are no longer in the list when rproc_del() executes, it seems
this leaves GLINK edges active and dangling for a powered-off remoteproc,
potentially corrupting the GLINK state machine and freeing structures still
in use. Should the subdevices be left for rproc_del() to clean up?

> +	qcom_q6v5_deinit(&wcss->q6);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3