[PATCH] misc: ocxl: fix possible name leak in ocxl_file_register_afu()

If device_register() returns error in ocxl_file_register_afu(),
the name allocated by dev_set_name() need be freed. As comment
of device_register() says, it should use put_device() to give
up the reference in the error path. So fix this by calling
put_device(), then the name can be freed in kobject_cleanup(),
and info is freed in info_release().

Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
Signed-off-by: Yang Yingliang <redacted>
---
 drivers/misc/ocxl/file.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d46dba2df5a1..452d5777a0e4 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -541,8 +541,11 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)
 		goto err_put;
 
 	rc = device_register(&info->dev);
-	if (rc)
-		goto err_put;
+	if (rc) {
+		free_minor(info);
+		put_device(&info->dev);
+		return rc;
+	}
 
 	rc = ocxl_sysfs_register_afu(info);
 	if (rc)

-- 
2.25.1

On 11/11/2022 15:59, Yang Yingliang wrote:

While I agree that a put_device() is needed on that error path, the fix 
above is not correct as it forgets to release the afu reference and the 
memory allocated in info. That was taken care of by the jump to the 
err_put label, so it should be kept. Something like:

-	if (rc)
+	if (rc) {
+		put_device((&info->dev);
  		goto err_put;
+	}


   Fred

Hi,

On 2022/11/14 19:23, Frederic Barrat wrote:

On 11/11/2022 15:59, Yang Yingliang wrote:

quoted

If device_register() returns error in ocxl_file_register_afu(),
the name allocated by dev_set_name() need be freed. As comment
of device_register() says, it should use put_device() to give
up the reference in the error path. So fix this by calling
put_device(), then the name can be freed in kobject_cleanup(),
and info is freed in info_release().

Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl 
backend & frontend")
Signed-off-by: Yang Yingliang <redacted>
---
  drivers/misc/ocxl/file.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d46dba2df5a1..452d5777a0e4 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -541,8 +541,11 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)

          goto err_put;
        rc = device_register(&info->dev);
-    if (rc)
-        goto err_put;
+    if (rc) {
+        free_minor(info);
+        put_device(&info->dev);
+        return rc;
+    }

While I agree that a put_device() is needed on that error path, the 
fix above is not correct as it forgets to release the afu reference 
and the memory allocated in info. That was taken care of by the jump 
to the err_put label, so it should be kept. Something like:

-    if (rc)
+    if (rc) {
+        put_device((&info->dev);
         goto err_put;
+    }

The 'info' and the reference is released in info_release().

Here is call chain:
put_device()
   kobject_release()
     kobject_cleanup()
       device_release()
         info_release()

static void info_release(struct device *dev)
{
         struct ocxl_file_info *info = container_of(dev, struct 
ocxl_file_info, dev);

         ocxl_afu_put(info->afu);
         kfree(info);
}
So it don't need jump to the error label in this case.

Thanks,
Yang

  Fred

.

On 14/11/2022 12:46, Yang Yingliang wrote:

Hi,

On 2022/11/14 19:23, Frederic Barrat wrote:

quoted

On 11/11/2022 15:59, Yang Yingliang wrote:

quoted

If device_register() returns error in ocxl_file_register_afu(),
the name allocated by dev_set_name() need be freed. As comment
of device_register() says, it should use put_device() to give
up the reference in the error path. So fix this by calling
put_device(), then the name can be freed in kobject_cleanup(),
and info is freed in info_release().

Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl 
backend & frontend")
Signed-off-by: Yang Yingliang <redacted>
---
  drivers/misc/ocxl/file.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
index d46dba2df5a1..452d5777a0e4 100644
--- a/drivers/misc/ocxl/file.c
+++ b/drivers/misc/ocxl/file.c

@@ -541,8 +541,11 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)

          goto err_put;
        rc = device_register(&info->dev);
-    if (rc)
-        goto err_put;
+    if (rc) {
+        free_minor(info);
+        put_device(&info->dev);
+        return rc;
+    }

While I agree that a put_device() is needed on that error path, the 
fix above is not correct as it forgets to release the afu reference 
and the memory allocated in info. That was taken care of by the jump 
to the err_put label, so it should be kept. Something like:

-    if (rc)
+    if (rc) {
+        put_device((&info->dev);
         goto err_put;
+    }

The 'info' and the reference is released in info_release().

Here is call chain:
put_device()
   kobject_release()
     kobject_cleanup()
       device_release()
         info_release()

static void info_release(struct device *dev)
{
         struct ocxl_file_info *info = container_of(dev, struct 
ocxl_file_info, dev);

         ocxl_afu_put(info->afu);
         kfree(info);
}
So it don't need jump to the error label in this case.

You're right, I went too fast and the patch is correct.
So:
Acked-by: Frederic Barrat <redacted>

   Fred

On Fri, 2022-11-11 at 22:59 +0800, Yang Yingliang wrote:

If device_register() returns error in ocxl_file_register_afu(),
the name allocated by dev_set_name() need be freed. As comment
of device_register() says, it should use put_device() to give
up the reference in the error path. So fix this by calling
put_device(), then the name can be freed in kobject_cleanup(),
and info is freed in info_release().

Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl
backend & frontend")
Signed-off-by: Yang Yingliang <redacted>

Thanks for the fix - as you point out, put_device() should clean
everything up that needs cleaning up.

Acked-by: Andrew Donnellan <redacted>

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

On Fri, 11 Nov 2022 22:59:29 +0800, Yang Yingliang wrote:

If device_register() returns error in ocxl_file_register_afu(),
the name allocated by dev_set_name() need be freed. As comment
of device_register() says, it should use put_device() to give
up the reference in the error path. So fix this by calling
put_device(), then the name can be freed in kobject_cleanup(),
and info is freed in info_release().

[...]

Applied to powerpc/next.

[1/1] misc: ocxl: fix possible name leak in ocxl_file_register_afu()
      https://git.kernel.org/powerpc/c/295faa17722a11cac8dbf51e4c9f9405a5e07ef1

cheers