Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode

[Dawei Li <dawei.li@linux.dev>]

Hi Mathieu, 

On Mon, Nov 10, 2025 at 10:05:44AM -0700, Mathieu Poirier wrote:
> On Wed, 22 Oct 2025 at 10:41, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Wed, 22 Oct 2025 at 09:54, Dawei Li <dawei.li@linux.dev> wrote:

[...]

> > > >     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         }

[...]

> > > >     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.
> > >
> > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> > > index 34b35ea74aab..c322df56394f 100644
> > > --- a/drivers/rpmsg/rpmsg_char.c
> > > +++ b/drivers/rpmsg/rpmsg_char.c
> > > @@ -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;
> > >  }
> > >

[...]

> >
> > 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.

> 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.

> 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 <dawei.li@linux.dev>
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 <dan.carpenter@linaro.org>
    Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/
    Signed-off-by: Dawei Li <dawei.li@linux.dev>

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 34b35ea74aab..c322df56394f 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -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