* Re: [PATCH] dmaengine: qcom: hidma_mgmt: fix autosuspend cleanup on probe failure
2026-08-08 11:20 [PATCH] dmaengine: qcom: hidma_mgmt: fix autosuspend cleanup on probe failure Guangshuo Li
@ 2026-08-08 11:37 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-08 11:37 UTC (permalink / raw)
To: Guangshuo Li; +Cc: Frank.Li, dmaengine, vkoul
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
^ permalink raw reply [flat|nested] 2+ messages in thread