Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The generic PAS API functions in qcom_pas.c suffer from a TOCTOU race condition when reading the global ops_ptr, leading to a potential NULL pointer dereference. Pre-existing issues: - [High] Missing cleanup of SMD and Sysmon subdevices on wcnss_probe() error paths leads to resource leaks and Use-After-Free vulnerabilities. - [High] Removing iris before deleting rproc in wcnss_remove() creates a Use-After-Free race condition. - [High] Missing reinit_completion() causes spurious success on subsequent remoteproc starts or stops. - [High] Infinite polling loop in wcnss_configure_iris() can cause kernel hangs. -- commit f90353d8289b80221557c481e264c0861b3fde18 Author: Sumit Garg <[email protected]> remoteproc: qcom_wcnss: Switch to generic PAS TZ APIs This commit switches the qcom_wcnss client driver to use the generic PAS TZ APIs. This allows support for multiple TZ implementations like QTEE or OP-TEE. > diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c > index 4add9037dbd5a..0dbdd18ab3dd9 100644 > --- a/drivers/remoteproc/qcom_wcnss.c > +++ b/drivers/remoteproc/qcom_wcnss.c > @@ -257,7 +257,7 @@ static int wcnss_start(struct rproc *rproc) > wcnss_indicate_nv_download(wcnss); > wcnss_configure_iris(wcnss); [Severity: High] This is a pre-existing issue, but can the infinite polling loops inside wcnss_configure_iris() cause a hard kernel hang if the hardware is wedged? Inside wcnss_configure_iris(), we poll hardware registers without a timeout: drivers/remoteproc/qcom_wcnss.c:wcnss_configure_iris() { ... while (readl(wcnss->pmu_cfg) & WCNSS_PMU_IRIS_RESET_STS) cpu_relax(); ... while (readl(wcnss->pmu_cfg) & WCNSS_PMU_IRIS_XO_CFG_STS) cpu_relax(); ... } Could this lead to a denial of service if the register returns 0xffffffff or remains unresponsive? > > - ret = qcom_scm_pas_auth_and_reset(WCNSS_PAS_ID); > + ret = qcom_pas_auth_and_reset(WCNSS_PAS_ID); [Severity: High] Does this new qcom_pas_auth_and_reset() call introduce a race condition? Looking at the generic PAS API implementation in qcom_pas.c: drivers/firmware/qcom/qcom_pas.c:qcom_pas_auth_and_reset() { if (!ops_ptr) return -ENODEV; return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id); } Since ops_ptr is checked and then dereferenced without READ_ONCE() or locks, could a concurrent backend module unload set ops_ptr to NULL between the check and the dereference, causing a kernel panic? [Severity: High] This is a pre-existing issue, but does wcnss_start() risk spurious success on subsequent starts if a previous wait timed out? There is a call to wait_for_completion_timeout(&wcnss->start_done, ...) right below this in wcnss_start(). If it times out, but a delayed ready interrupt later calls complete(&wcnss->start_done), the next wcnss_start() could instantly succeed without actually waiting for the firmware. Should reinit_completion() be called before initiating the wait in both wcnss_start() and wcnss_stop()? > if (ret) { > dev_err(wcnss->dev, > "failed to authenticate image and release reset\n"); [ ... ] > @@ -557,10 +557,10 @@ static int wcnss_probe(struct platform_device *pdev) > > data = of_device_get_match_data(&pdev->dev); > > - if (!qcom_scm_is_available()) > + if (!qcom_pas_is_available()) > return -EPROBE_DEFER; [Severity: High] This is a pre-existing issue, but do the error paths in wcnss_probe() leak the SMD and Sysmon subdevices? In wcnss_probe(), qcom_add_sysmon_subdev() allocates memory and registers a notifier to a global chain. If a subsequent step like qcom_iris_probe() fails, the error path jumps to remove_iris or detach_pds: drivers/remoteproc/qcom_wcnss.c:wcnss_probe() { ... remove_iris: qcom_iris_remove(wcnss->iris); detach_pds: wcnss_release_pds(wcnss); return ret; } This path fails to call qcom_remove_sysmon_subdev() and qcom_remove_smd_subdev(). Could this leave the sysmon object in the global notifier chain, leading to a use-after-free when the notifier triggers? [Severity: High] This is a pre-existing issue, but does wcnss_remove() have a use-after-free race condition by freeing the iris object before stopping the remoteproc? drivers/remoteproc/qcom_wcnss.c:wcnss_remove() { ... qcom_iris_remove(wcnss->iris); rproc_del(wcnss->rproc); ... } Because rproc_del() hasn't stopped the remoteproc yet, userspace could concurrently trigger wcnss_start(). Since wcnss_start() checks if (!wcnss->iris), which passes because the pointer is freed but not nulled, it then calls qcom_iris_enable(wcnss->iris) on freed memory. Should rproc_del() be called before qcom_iris_remove() to prevent this? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
