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
> <[email protected]> wrote:
> >
> > On Wed, 22 Oct 2025 at 09:54, Dawei Li <[email protected]> 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 <[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]/
Signed-off-by: Dawei Li <[email protected]>
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
