rpmsg: char: Implement eptdev based on anonymous inode

bug report Dan Carpenter 3 participants 6 messages Oct 22, 2025 – Nov 11, 2025 Remote processor messaging (rpmsg)

Share & tools

What do you expect me to do about it?
1. Send an independent fix patch.
2. Squash the fix patch into previous ones and resend series again.
3. Wait for other (if any) bug reports and fix them in a whole.

I am fine with all of them.

Thanks,

Dawei

Mathieu Poirier

maintainer

2025-10-22 16:41 UTC #2 lore raw

Quoted 87 lines · Dawei Li

Hi Dan,

Thanks for the report.

Quoted 46 lines · Dan Carpenter

Hello Dawei Li,

Commit 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode") from Oct 15, 2025 (linux-next), leads to the following Smatch static checker warning:

drivers/rpmsg/rpmsg_char.c:548 rpmsg_anonymous_eptdev_create()
error: dereferencing freed memory 'eptdev' (line 546)

drivers/rpmsg/rpmsg_char.c

    538         /* Anonymous inode only supports these file flags */
    539         if (flags & ~(O_ACCMODE | O_NONBLOCK | O_CLOEXEC))
    540                 return -EINVAL;
    541
    542         eptdev = rpmsg_eptdev_alloc(rpdev, parent, false);
    543         if (IS_ERR(eptdev))
    544                 return PTR_ERR(eptdev);
    545
    546         ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
    547         if (ret) {
--> 548                 dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                                 ^^^^^^                             ^^^^^^

The rpmsg_eptdev_add() function frees "eptdev" on error.

    549                 return ret;
    550         }
    551
    552         fd = anon_inode_getfd("rpmsg-eptdev", &rpmsg_anonymous_eptdev_fops, eptdev, flags);
    553         if (fd < 0) {
    554                 put_device(&eptdev->dev);
    555                 return fd;
    556         }
    557
    558         mutex_lock(&eptdev->ept_lock);
    559         ret = __rpmsg_eptdev_open(eptdev);

Should we free eptdev if __rpmsg_eptdev_open() fails?

    560         mutex_unlock(&eptdev->ept_lock);
    561
    562         if (!ret)
    563                 *pfd = fd;
    564
    565         return ret;
    566 }

Quoted 2 lines

regards,
dan carpenter

Diff below should do the trick.

drivers/rpmsg/rpmsg_char.c +3 -1

index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);

@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }

@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

Quoted 9 lines

Mathieu, Bjorn,
What do you expect me to do about it?
1. Send an independent fix patch.
2. Squash the fix patch into previous ones and resend series again.
3. Wait for other (if any) bug reports and fix them in a whole.
I am fine with all of them.

Please send another patch I can apply on top.

Quoted 4 lines

Thanks,
Dawei

Mathieu Poirier

maintainer

2025-11-10 17:05 UTC #3 lore raw

Quoted 96 lines · Mathieu Poirier

Quoted 94 lines · Dawei Li

Hi Dan,

Thanks for the report.

Quoted 46 lines · Dan Carpenter

Hello Dawei Li,

Commit 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode") from Oct 15, 2025 (linux-next), leads to the following Smatch static checker warning:

drivers/rpmsg/rpmsg_char.c:548 rpmsg_anonymous_eptdev_create()
error: dereferencing freed memory 'eptdev' (line 546)

drivers/rpmsg/rpmsg_char.c

    538         /* Anonymous inode only supports these file flags */
    539         if (flags & ~(O_ACCMODE | O_NONBLOCK | O_CLOEXEC))
    540                 return -EINVAL;
    541
    542         eptdev = rpmsg_eptdev_alloc(rpdev, parent, false);
    543         if (IS_ERR(eptdev))
    544                 return PTR_ERR(eptdev);
    545
    546         ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
    547         if (ret) {
--> 548                 dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                                 ^^^^^^                             ^^^^^^

The rpmsg_eptdev_add() function frees "eptdev" on error.

    549                 return ret;
    550         }
    551
    552         fd = anon_inode_getfd("rpmsg-eptdev", &rpmsg_anonymous_eptdev_fops, eptdev, flags);
    553         if (fd < 0) {
    554                 put_device(&eptdev->dev);
    555                 return fd;
    556         }
    557
    558         mutex_lock(&eptdev->ept_lock);
    559         ret = __rpmsg_eptdev_open(eptdev);

Should we free eptdev if __rpmsg_eptdev_open() fails?

    560         mutex_unlock(&eptdev->ept_lock);
    561
    562         if (!ret)
    563                 *pfd = fd;
    564
    565         return ret;
    566 }

Quoted 2 lines

regards,
dan carpenter

Diff below should do the trick.

drivers/rpmsg/rpmsg_char.c +3 -1

index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);

@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }

@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

Mathieu, Bjorn,

Quoted 4 lines

Quoted 2 lines
I am fine with all of them.
Please send another patch I can apply on top.

I haven't received a fix for this yet. After looking into this bug report with more scrutiny I am of the opinion that @eptdev should be free'd in rpmsg_anonymous_eptdev_create() where it was allocated. Furthermore, if function anon_inode_getfd() in rpmsg_anonymous_eptdev_create() fails, function rpmsg_eptdev_release_device() will be called but I don't see a call to cdev_device_del() in there. Am I missing something?

Quoted 5 lines

Quoted 5 lines
Thanks,
Dawei

Dawei Li

2025-11-11 12:53 UTC #4 lore raw

Hi Mathieu,

Quoted 4 lines · Mathieu Poirier

Quoted 2 lines · Mathieu Poirier
On Wed, 22 Oct 2025 at 09:54, Dawei Li <[email protected]> wrote:

[...]

Quoted 8 lines

    546         ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
    547         if (ret) {
--> 548                 dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                                 ^^^^^^                             ^^^^^^

The rpmsg_eptdev_add() function frees "eptdev" on error.

    549                 return ret;
    550         }

[...]

Quoted 40 lines

Quoted 6 lines

    558         mutex_lock(&eptdev->ept_lock);
    559         ret = __rpmsg_eptdev_open(eptdev);

Should we free eptdev if __rpmsg_eptdev_open() fails?

    560         mutex_unlock(&eptdev->ept_lock);

Diff below should do the trick.

drivers/rpmsg/rpmsg_char.c +3 -1

index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);

@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }

@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

[...]

Quoted 4 lines

Quoted 2 lines
Please send another patch I can apply on top.
I haven't received a fix for this yet. After looking into this bug

Sorry about the late response.

Quoted 2 lines

report with more scrutiny I am of the opinion that @eptdev should be free'd in rpmsg_anonymous_eptdev_create() where it was allocated.

Yes, and it's exactly what diff above is doing.

Quoted 4 lines

Furthermore, if function anon_inode_getfd() in rpmsg_anonymous_eptdev_create() fails, function rpmsg_eptdev_release_device() will be called but I don't see a call to cdev_device_del() in there. Am I missing something?

1. rpmsg_anonymous_eptdev_create() does _not_ create cdev, so it's not supposed to call cdev_device_del().
https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c?h=rpmsg-next#n546

2. So I assume you mean "what about epdev based on chardev?". It's simply because cdev cleanup job is on rpmsg_chrdev_eptdev_destroy(), _not_ rpmsg_eptdev_release_device().

So my proposed fix patch still holds, IIUC:

Author: Dawei Li <[email protected]>
Date:   Sun Oct 26 23:18:06 2025 +0800

rpmsg: char: Fix UAF and memory leak

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

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 +3 -1

index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);

@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }

@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

I will send the fix patch if you find it not offending.

Thanks,

Dawei

Mathieu Poirier

maintainer

2025-11-11 16:41 UTC #5 lore raw

Quoted 65 lines · Dawei Li

Hi Mathieu,

Quoted 4 lines · Mathieu Poirier

Quoted 2 lines · Mathieu Poirier
On Wed, 22 Oct 2025 at 09:54, Dawei Li <[email protected]> wrote:

[...]

Quoted 8 lines

    546         ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
    547         if (ret) {
--> 548                 dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                                 ^^^^^^                             ^^^^^^

The rpmsg_eptdev_add() function frees "eptdev" on error.

    549                 return ret;
    550         }

[...]

Quoted 40 lines

Quoted 6 lines

    558         mutex_lock(&eptdev->ept_lock);
    559         ret = __rpmsg_eptdev_open(eptdev);

Should we free eptdev if __rpmsg_eptdev_open() fails?

    560         mutex_unlock(&eptdev->ept_lock);

Diff below should do the trick.

drivers/rpmsg/rpmsg_char.c +3 -1

index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);

@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }

@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

[...]

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Please send another patch I can apply on top.
I haven't received a fix for this yet. After looking into this bug
Sorry about the late response.

We are at rc5. If issues with this feature aren't addressed by rc7, it will be taken out.

Quoted 5 lines

Quoted 3 lines
report with more scrutiny I am of the opinion that @eptdev should be free'd in rpmsg_anonymous_eptdev_create() where it was allocated.
Yes, and it's exactly what diff above is doing.

The error path of rpmsg_eptdev_add() should not call put_device() and kfree(), this is something that belongs to the error handling of an rpmsg_eptdev_add() failure in rpmsg_anonymous_eptdev_create().

I'm also wondering why the two "if (cdev)" conditions in rpmsg_eptdev_add() can't be merged together in the bottom one.

Quoted 8 lines

Quoted 5 lines
Furthermore, if function anon_inode_getfd() in rpmsg_anonymous_eptdev_create() fails, function rpmsg_eptdev_release_device() will be called but I don't see a call to cdev_device_del() in there. Am I missing something?
1. rpmsg_anonymous_eptdev_create() does _not_ create cdev, so it's not supposed to call cdev_device_del().

You are correct.

Quoted 51 lines

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/rpmsg_char.c?h=rpmsg-next#n546
2. So I assume you mean "what about epdev based on chardev?". It's simply because cdev cleanup job is on rpmsg_chrdev_eptdev_destroy(), _not_ rpmsg_eptdev_release_device().
So my proposed fix patch still holds, IIUC:
Author: Dawei Li <[email protected]>
Date:   Sun Oct 26 23:18:06 2025 +0800
rpmsg: char: Fix UAF and memory leak
Potential UAF and memory leak exsit in exception handling paths for rpmsg_anonymous_eptdev_create(), fix them.
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 +3 -1
index 34b35ea74aab..c322df56394f 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);
        put_device(dev);
        kfree(eptdev);
 
@@ -545,7 +546,6 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
        if (ret) {
-               dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
                return ret;
        }
 
@@ -561,6 +561,8 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par
 
        if (!ret)
                *pfd = fd;
+       else
+               put_device(&eptdev->dev);
 
        return ret;
 }

Quoted 1 line

I will send the fix patch if you find it not offending.

Offending? What would I be offended about?

Quoted 4 lines

Thanks,
Dawei