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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, Nov 11, 2025 at 08:53:02PM +0800, Dawei Li wrote:
> 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.

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

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

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

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

Offending?  What would I be offended about?

> 
> Thanks,
> 
> 	Dawei