Fix and rework of rpmsg_eptdev_add()

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complaint the absence of release():

[  159.112182] ------------[ cut here ]------------
[  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
[  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Cc [email protected]

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 1 -
  1 file changed, 1 deletion(-)

drivers/rpmsg/rpmsg_char.c +0 -0

index 34b35ea74aab..1b8297b373f0 100644

drivers/rpmsg/rpmsg_char.c +0 -0

  	kfree(eptdev);
 
  	return ret;

Quoted 4 lines · Dawei Li

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

Quoted 8 lines

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complaint the absence of release():

Hi Dawei,

Quoted 5 lines

[  159.112182] ------------[ cut here ]------------
[  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst. [  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90

Although my local checkpatch.pl didn’t complain about this log line exceeding 75 characters, could we simplify it or just provide a summary instead?

Quoted 15 lines

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Cc [email protected]

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 1 -

  1 file changed, 1 deletion(-)

drivers/rpmsg/rpmsg_char.c +0 -0

index 34b35ea74aab..1b8297b373f0 100644
@@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev 
 *eptdev,
       if (cdev)
           ida_free(&rpmsg_minor_ida, MINOR(dev->devt));

Quoted 2 lines

  free_eptdev:
-    put_device(dev);

Yes, remove put_device can solve the warning issue, however it would introduce one memleak issue of kobj->name.

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/ tree/drivers/rpmsg/rpmsg_char.c#n381

dev_set_name(dev, "rpmsg%d", ret); is already called, it depends on put_device to free memory, right?

Quoted 2 lines

      kfree(eptdev);
      return ret;

Quoted 4 lines · Dawei Li

put_device() is called on error path of rpmsg_eptdev_add() to cleanup resource attached to eptdev->dev, unfortunately it's bogus cause dev->release() is not set yet.

Quoted 8 lines

When a struct device instance is destroyed, driver core framework checks the possible release() callback from candidates below:
- struct device::release()
- dev->type->release()
- dev->class->dev_release()

Rpmsg eptdev owns none of them so WARN() will complaint the absence of release():

Hi Dawei,

Quoted 5 lines

[  159.112182] ------------[ cut here ]------------
[  159.112188] Device '(null)' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
[  159.112205] WARNING: CPU: 2 PID: 1975 at drivers/base/core.c:2567 device_release+0x7a/0x90

Although my local checkpatch.pl didn’t complain about this log line exceeding 75 characters, could we simplify it or just provide a summary instead?

Quoted 12 lines

Fixes c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")

Cc [email protected]

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 1 -
  1 file changed, 1 deletion(-)

drivers/rpmsg/rpmsg_char.c +0 -0

index 34b35ea74aab..1b8297b373f0 100644

drivers/rpmsg/rpmsg_char.c static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev, · line 497

Quoted 4 lines

drivers/rpmsg/rpmsg_char.c +0 -1

@@ -494,7 +494,6 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
  	if (cdev)
  		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
  free_eptdev:
-	put_device(dev);

Yes, remove put_device can solve the warning issue, however it would introduce one memleak issue of kobj->name.

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c#n381

dev_set_name(dev, "rpmsg%d", ret); is already called, it depends on put_device to free memory, right?

As I mentioned about the potential memory leak issue in patch 1/3, we could consider still using put_device for management, as this better aligns with the driver model standards and avoids potential issue. However, this requires assigning the release function in advance and also handling the special case where ida allocation fails in rpmsg_eptdev_add (removing the manual ida release).

Quoted 4 lines

drivers/rpmsg/rpmsg_char.c +0 -0

  	kfree(eptdev);
  	return ret;

Signature · 2 lines

Thx and BRs,
Zhongqiu Han

Potential UAF and memory leak exsit in exception handling paths for rpmsg_anonymous_eptdev_create(), fix them.

- If rpmsg_add_eptdev() failes, eptdev is freed in it. Subsequent call of dev_err(&eptdev->device) triggers a UAF.
- If __rpmsg_eptdev_open() fails, eptdev is supposed to be freed by put_device().

Fixes 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")

Reported-by Dan Carpenter <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Dawei Li <[email protected]>

---
 drivers/rpmsg/rpmsg_char.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/rpmsg/rpmsg_char.c +1 -0

index 1b8297b373f0..0919ad0a19df 100644
@@ -494,6 +494,7 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 	if (cdev)
 		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
 free_eptdev:
+	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
 	kfree(eptdev);
 
 	return ret;

drivers/rpmsg/rpmsg_char.c +0 -0

 	return ret;
 }

Signature · 1 line

2.25.1

Rework error handling of rpmsg_eptdev_add() and its callers, following rule of "release resource where it's allocated".

Fixes 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")

Reported-by Dan Carpenter <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Dawei Li <[email protected]>

---
  drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++-----------------
  1 file changed, 33 insertions(+), 27 deletions(-)

drivers/rpmsg/rpmsg_char.c +21 -26

index 0919ad0a19df..92c176e9b0e4 100644
@@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 
  	eptdev->chinfo = chinfo;
 
-	if (cdev) {
-		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
-		if (ret < 0)
-			goto free_eptdev;
-
-		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-	}
-
  	/* Anonymous inode device still need device name for dev_err() and friends */
  	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
  	if (ret < 0)
-		goto free_minor_ida;
+		return ret;
  	dev->id = ret;
  	dev_set_name(dev, "rpmsg%d", ret);
 
-	ret = 0;
-
  	if (cdev) {
+		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
+		if (ret < 0) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			return ret;
+		}
+
+		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
  		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-		if (ret)
-			goto free_ept_ida;
+		if (ret) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
+			return ret;
+		}
  	}
 
  	/* We can now rely on the release function for cleanup */
  	dev->release = rpmsg_eptdev_release_device;
 
-	return ret;
-
-free_ept_ida:
-	ida_free(&rpmsg_ept_ida, dev->id);
-free_minor_ida:
-	if (cdev)
-		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
-free_eptdev:
-	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
-	kfree(eptdev);
-
-	return ret;
+	return 0;
  }
 
  static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)

@@ -509,12 +499,17 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
  			       struct rpmsg_channel_info chinfo)
  {
  	struct rpmsg_eptdev *eptdev;
+	int ret;
 
  	eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, parent);
  	if (IS_ERR(eptdev))
  		return PTR_ERR(eptdev);
 
-	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	if (ret)
+		kfree(eptdev);
+
+	return ret;
  }
  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 

drivers/rpmsg/rpmsg_char.c +8 -1

+		 */
+		kfree(eptdev);
  		return ret;
  	}
 

@@ -572,6 +573,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
  	struct rpmsg_channel_info chinfo;
  	struct rpmsg_eptdev *eptdev;
  	struct device *dev = &rpdev->dev;
+	int ret;
 
  	memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
  	chinfo.src = rpdev->src;

@@ -590,7 +592,11 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
  	 */
  	eptdev->default_ept->priv = eptdev;
 
-	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+	if (ret)
+		kfree(eptdev);
+
+	return ret;
  }
 
  static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)

Rework error handling of rpmsg_eptdev_add() and its callers, following rule of "release resource where it's allocated".

Fixes 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")

Reported-by Dan Carpenter <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Dawei Li <[email protected]>

---
 drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 27 deletions(-)

drivers/rpmsg/rpmsg_char.c +15 -25

index 0919ad0a19df..92c176e9b0e4 100644
@@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
 
 	eptdev->chinfo = chinfo;
 
-	if (cdev) {
-		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
-		if (ret < 0)
-			goto free_eptdev;
-
-		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-	}
-
 	/* Anonymous inode device still need device name for dev_err() and friends */
 	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
 	if (ret < 0)
-		goto free_minor_ida;
+		return ret;
 	dev->id = ret;
 	dev_set_name(dev, "rpmsg%d", ret);
 
-	ret = 0;
-
 	if (cdev) {
+		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
+		if (ret < 0) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			return ret;
+		}
+
+		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
 		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-		if (ret)
-			goto free_ept_ida;
+		if (ret) {
+			ida_free(&rpmsg_ept_ida, dev->id);
+			ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
+			return ret;
+		}
 	}
 
 	/* We can now rely on the release function for cleanup */
 	dev->release = rpmsg_eptdev_release_device;
 
-	return ret;
-
-free_ept_ida:
-	ida_free(&rpmsg_ept_ida, dev->id);
-free_minor_ida:
-	if (cdev)
-		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
-free_eptdev:
-	dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
-	kfree(eptdev);
-
-	return ret;
+	return 0;
 }
 
 static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)

@@ -509,12 +499,17 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 			       struct rpmsg_channel_info chinfo)
 {
 	struct rpmsg_eptdev *eptdev;

drivers/rpmsg/rpmsg_char.c +2 -0

+
+	return ret;
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 

drivers/rpmsg/rpmsg_char.c +1 -0

 		return ret;
 	}
 

@@ -572,6 +573,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 	struct rpmsg_channel_info chinfo;
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev = &rpdev->dev;
+	int ret;
 
 	memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
 	chinfo.src = rpdev->src;

drivers/rpmsg/rpmsg_char.c +2 -0

+
+	return ret;
 }
 
 static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)

Signature · 1 line

2.25.1