[PATCH v1] dmaengine: qcom: hidma-mgmt: Fix sysfs cleanup on setup failure

[PATCH v1] dmaengine: qcom: hidma-mgmt: Fix sysfs cleanup on setup failure

[Yuho Choi]

hidma_mgmt_init_sys() creates the chanops kobject, per-channel
kobjects and sysfs files incrementally. If a later creation step fails,
the function returns without tearing down the objects already created.

Those sysfs callbacks reference devm-managed driver data. A later probe
failure can free that data while the sysfs entries and kobjects remain
registered.

Track the chanops kobject in struct hidma_mgmt_dev, unwind the sysfs
files and channel kobjects on setup failure, and register the same
cleanup with devm after successful setup.

Fixes: 7f8f209fd6e0 ("dmaengine: add Qualcomm Technologies HIDMA management driver")
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
 drivers/dma/qcom/hidma_mgmt.h     |  1 +
 drivers/dma/qcom/hidma_mgmt_sys.c | 65 +++++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/qcom/hidma_mgmt.h b/drivers/dma/qcom/hidma_mgmt.h
index 30e8095988bf..4fb6759e3371 100644
--- a/drivers/dma/qcom/hidma_mgmt.h
+++ b/drivers/dma/qcom/hidma_mgmt.h
@@ -24,6 +24,7 @@ struct hidma_mgmt_dev {
 	resource_size_t addrsize;
 
 	struct kobject **chroots;
+	struct kobject *chanops;
 	struct platform_device *pdev;
 };
 
diff --git a/drivers/dma/qcom/hidma_mgmt_sys.c b/drivers/dma/qcom/hidma_mgmt_sys.c
index 930eae0a6257..280b3af6ec03 100644
--- a/drivers/dma/qcom/hidma_mgmt_sys.c
+++ b/drivers/dma/qcom/hidma_mgmt_sys.c
@@ -231,20 +231,52 @@ static int create_sysfs_entry_channel(struct hidma_mgmt_dev *mdev, char *name,
 	return sysfs_create_file(parent, &chattr->attr.attr);
 }
 
+static void hidma_mgmt_uninit_sys(struct hidma_mgmt_dev *mdev,
+				  unsigned int sysfs_count,
+				  unsigned int chroot_count)
+{
+	unsigned int i;
+
+	for (i = 0; i < sysfs_count; i++) {
+		struct attribute attr = { .name = hidma_mgmt_files[i].name };
+
+		sysfs_remove_file(&mdev->pdev->dev.kobj, &attr);
+	}
+
+	for (i = 0; i < chroot_count; i++) {
+		kobject_put(mdev->chroots[i]);
+		mdev->chroots[i] = NULL;
+	}
+
+	if (mdev->chanops) {
+		kobject_put(mdev->chanops);
+		mdev->chanops = NULL;
+	}
+}
+
+static void hidma_mgmt_uninit_sys_action(void *data)
+{
+	struct hidma_mgmt_dev *mdev = data;
+
+	hidma_mgmt_uninit_sys(mdev, ARRAY_SIZE(hidma_mgmt_files),
+			      mdev->dma_channels);
+}
+
 int hidma_mgmt_init_sys(struct hidma_mgmt_dev *mdev)
 {
+	unsigned int chroot_count = 0;
+	unsigned int sysfs_count = 0;
 	unsigned int i;
-	int rc;
 	int required;
-	struct kobject *chanops;
+	int rc;
 
 	required = sizeof(*mdev->chroots) * mdev->dma_channels;
 	mdev->chroots = devm_kmalloc(&mdev->pdev->dev, required, GFP_KERNEL);
 	if (!mdev->chroots)
 		return -ENOMEM;
 
-	chanops = kobject_create_and_add("chanops", &mdev->pdev->dev.kobj);
-	if (!chanops)
+	mdev->chanops = kobject_create_and_add("chanops", &mdev->pdev->dev.kobj);
+	if (!mdev->chanops)
 		return -ENOMEM;
 
 	/* create each channel directory here */
@@ -252,9 +284,12 @@ int hidma_mgmt_init_sys(struct hidma_mgmt_dev *mdev)
 		char name[20];
 
 		snprintf(name, sizeof(name), "chan%d", i);
-		mdev->chroots[i] = kobject_create_and_add(name, chanops);
-		if (!mdev->chroots[i])
-			return -ENOMEM;
+		mdev->chroots[i] = kobject_create_and_add(name, mdev->chanops);
+		if (!mdev->chroots[i]) {
+			rc = -ENOMEM;
+			goto err_uninit;
+		}
+		chroot_count++;
 	}
 
 	/* populate common parameters */
@@ -262,7 +297,9 @@ int hidma_mgmt_init_sys(struct hidma_mgmt_dev *mdev)
 		rc = create_sysfs_entry(mdev, hidma_mgmt_files[i].name,
 					hidma_mgmt_files[i].mode);
 		if (rc)
-			return rc;
+			goto err_uninit;
+
+		sysfs_count++;
 	}
 
 	/* populate parameters that are per channel */
@@ -271,15 +308,21 @@ int hidma_mgmt_init_sys(struct hidma_mgmt_dev *mdev)
 						(S_IRUGO | S_IWUGO), i,
 						mdev->chroots[i]);
 		if (rc)
-			return rc;
+			goto err_uninit;
 
 		rc = create_sysfs_entry_channel(mdev, "weight",
 						(S_IRUGO | S_IWUGO), i,
 						mdev->chroots[i]);
 		if (rc)
-			return rc;
+			goto err_uninit;
 	}
 
-	return 0;
+	return devm_add_action_or_reset(&mdev->pdev->dev,
+					hidma_mgmt_uninit_sys_action, mdev);
+
+err_uninit:
+	hidma_mgmt_uninit_sys(mdev, sysfs_count, chroot_count);
+
+	return rc;
 }
 EXPORT_SYMBOL_GPL(hidma_mgmt_init_sys);
-- 
2.43.0

Re: [PATCH v1] dmaengine: qcom: hidma-mgmt: Fix sysfs cleanup on setup failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Reliance on `kobject_put` without `kobject_del` for sysfs cleanup leads to use-after-free
--

commit a6dc90d05cf8a69c255834fbadc8ca4eb60dbe56
Author: Yuho Choi <dbgh9129@gmail.com>

dmaengine: qcom: hidma-mgmt: Fix sysfs cleanup on setup failure

hidma_mgmt_init_sys() creates the chanops kobject, per-channel
kobjects and sysfs files incrementally. If a later creation step fails,
the function returns without tearing down the objects already created.

> diff --git a/drivers/dma/qcom/hidma_mgmt_sys.c b/drivers/dma/qcom/hidma_mgmt_sys.c
> --- a/drivers/dma/qcom/hidma_mgmt_sys.c
> +++ b/drivers/dma/qcom/hidma_mgmt_sys.c

[ ... ]

> @@ -231,20 +231,52 @@ static int create_sysfs_entry_channel(struct hidma_mgmt_dev *mdev, char *name,
>  	return sysfs_create_file(parent, &chattr->attr.attr);
>  }
>  
> +static void hidma_mgmt_uninit_sys(struct hidma_mgmt_dev *mdev,
> +				  unsigned int sysfs_count,
> +				  unsigned int chroot_count)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < sysfs_count; i++) {
> +		struct attribute attr = { .name = hidma_mgmt_files[i].name };
> +
> +		sysfs_remove_file(&mdev->pdev->dev.kobj, &attr);
> +	}
> +
> +	for (i = 0; i < chroot_count; i++) {
> +		kobject_put(mdev->chroots[i]);

[Severity: High]
This is a pre-existing issue, but does this teardown logic still leave a
potential use-after-free?

Since the sysfs attributes (chattr) and their names are allocated using
devm_kmalloc() and devm_kstrdup() in create_sysfs_entry_channel(), devres
will free them immediately upon probe failure or device unbind.

If userspace holds an open file descriptor to any of the per-channel sysfs
files, or if CONFIG_DEBUG_KOBJECT_RELEASE is enabled, kobject_put() will
only decrement the reference count and the implicit kobject_del() inside
kobject_release() will be delayed. A subsequent read or write to the open
sysfs file would then dereference the already freed chattr memory.

Should kobject_del() be explicitly called before kobject_put() here to
synchronously remove the sysfs directory and files before devres frees the
backing memory?

> +		mdev->chroots[i] = NULL;
> +	}
> +
> +	if (mdev->chanops) {
> +		kobject_put(mdev->chanops);
> +		mdev->chanops = NULL;
> +	}
> +}

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