Re: [PATCH] dmaengine: qcom: hidma_mgmt: fix autosuspend cleanup on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing sysfs teardown and `.remove` callback leads to memory leaks on probe failure and a critical Use-After-Free / NULL pointer dereference upon driver unbind.
- [High] The return value of `pm_runtime_get_sync()` is ignored, risking unclocked register accesses and system panics.
- [High] Sysfs files are exposed to userspace before the driver data is fully initialized, allowing a NULL pointer dereference.
- [High] The `hidma_mgmt_setup` function updates hardware registers via read-modify-write sequences and is reachable concurrently via sysfs without any locking.
- [Medium] Global module parameters are permanently overwritten with device-specific properties during probe, breaking support for multiple device instances.
--

--- Patch [1]: [PATCH] dmaengine: qcom: hidma_mgmt: fix autosuspend cleanup on probe failure ---
commit e3267e79e146ae55fb7b58fe0ff314bb28430310
Author: Guangshuo Li <lgs201920130244@gmail.com>

dmaengine: qcom: hidma_mgmt: fix autosuspend cleanup on probe failure

hidma_mgmt_probe() calls pm_runtime_use_autosuspend(), but its failure
path does not call the matching pm_runtime_dont_use_autosuspend()
before disabling runtime PM.

If the autosuspend delay is set to a negative value while autosuspend
is enabled, the runtime PM core increments usage_count to prevent
runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
during cleanup, this reference is not dropped and usage_count remains
unbalanced.

Add the missing pm_runtime_dont_use_autosuspend() call to the probe
failure path before disabling runtime PM.

This issue was found by manual code inspection.

Fixes: 7f8f209fd6e0 ("dmaengine: add Qualcomm Technologies HIDMA management driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>

> diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
> index 4805ce390ffac..470c88f36d256 100644
> --- a/drivers/dma/qcom/hidma_mgmt.c
> +++ b/drivers/dma/qcom/hidma_mgmt.c
> @@ -168,7 +168,11 @@ static int hidma_mgmt_probe(struct platform_device *pdev)
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, HIDMA_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +
> +	rc = devm_pm_runtime_enable(&pdev->dev);
> +	if (rc)
> +		return rc;
> +
>  	pm_runtime_get_sync(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_get_sync() here?

If power or clock enabling fails, pm_runtime_get_sync() returns a negative
error code. By unconditionally ignoring this return value, the driver assumes
the hardware is powered on and proceeds to map and access I/O memory:

    virtaddr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

Later, hidma_mgmt_setup() calls readl() on the hardware registers.

Could this lead to unclocked hardware register accesses resulting in bus errors
or system panics?

>  
>  	virtaddr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

[ ... ]

> @@ -310,7 +314,6 @@ static int hidma_mgmt_probe(struct platform_device *pdev)
>  	return 0;
>  out:
>  	pm_runtime_put_sync_suspend(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
>  	return rc;
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but does this driver lack a .remove
callback and sysfs teardown?

Looking at hidma_mgmt_driver, there is no .remove callback defined.
Additionally, hidma_mgmt_init_sys() creates sysfs entries using
device_create_file(), but these are never removed on probe failure or unbind.

Could this lead to a memory leak on probe failure, and a use-after-free or
NULL pointer dereference if devres frees the structures while the sysfs files
are still accessible after unbind?

[Severity: Medium]
This is a pre-existing issue, but does hidma_mgmt_probe() permanently
overwrite the global module parameter max_write_request?

In hidma_mgmt_probe():

    if (max_write_request &&
        (max_write_request != mgmtdev->max_write_request)) {
        ...
        mgmtdev->max_write_request = max_write_request;
    } else {
        max_write_request = mgmtdev->max_write_request;
    }

Since max_write_request is a global module parameter, if a second device
instance probes, wouldn't it see a non-zero global parameter and incorrectly
overwrite its own values with the values hijacked from the first device?

[Severity: High]
This isn't a bug introduced by this patch, but are sysfs files exposed to
userspace before the driver data is fully initialized?

In hidma_mgmt_probe(), hidma_mgmt_init_sys() is called before
platform_set_drvdata():

    rc = hidma_mgmt_init_sys(mgmtdev);
    ...
    platform_set_drvdata(pdev, mgmtdev);

If userspace accesses these sysfs files in this exact time window,
show_values() in hidma_mgmt_sys.c calls dev_get_drvdata() which would return
NULL. The sysfs handlers then blindly dereference this pointer.

Could this race condition allow an unprivileged local user to trigger a NULL
pointer dereference?

[Severity: High]
This is a pre-existing issue, but does hidma_mgmt_setup() update hardware
registers without any locking?

The sysfs store handlers like set_priority() in hidma_mgmt_sys.c are
world-writable and modify shared state:

    tmp = mdev->priority[i];
    mdev->priority[i] = val;
    rc = hidma_mgmt_setup(mdev);

Inside hidma_mgmt_setup(), it performs read-modify-write sequences on shared
hardware registers:

    val = readl(mgmtdev->virtaddr + HIDMA_MAX_BUS_REQ_LEN_OFFSET);
    val &= ~(HIDMA_MAX_BUS_REQ_LEN_MASK << HIDMA_MAX_BUS_WR_REQ_BIT_POS);
    ...
    writel(val, mgmtdev->virtaddr + HIDMA_MAX_BUS_REQ_LEN_OFFSET);

Could concurrent writes from userspace to these sysfs files corrupt the
hardware configuration state and driver internal variables due to the lack of
serialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808112043.2781244-1-lgs201920130244@gmail.com?part=1