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 > > <[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.
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 <[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. Offending? What would I be offended about? > > Thanks, > > Dawei
