* [bug report] rpmsg: char: Implement eptdev based on anonymous inode
@ 2025-10-22 11:05 Dan Carpenter
2025-10-22 15:53 ` Dawei Li
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2025-10-22 11:05 UTC (permalink / raw)
To: Dawei Li; +Cc: linux-remoteproc
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 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode
2025-10-22 11:05 [bug report] rpmsg: char: Implement eptdev based on anonymous inode Dan Carpenter
@ 2025-10-22 15:53 ` Dawei Li
2025-10-22 16:41 ` Mathieu Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Dawei Li @ 2025-10-22 15:53 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-remoteproc, andersson, mathieu.poirier
Hi Dan,
Thanks for the report.
On Wed, Oct 22, 2025 at 02:05:36PM +0300, Dan Carpenter wrote:
> 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 }
>
> regards,
> dan carpenter
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;
}
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.
Thanks,
Dawei
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode
2025-10-22 15:53 ` Dawei Li
@ 2025-10-22 16:41 ` Mathieu Poirier
2025-11-10 17:05 ` Mathieu Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Poirier @ 2025-10-22 16:41 UTC (permalink / raw)
To: Dawei Li; +Cc: Dan Carpenter, linux-remoteproc, andersson
On Wed, 22 Oct 2025 at 09:54, Dawei Li <dawei.li@linux.dev> wrote:
>
> Hi Dan,
>
> Thanks for the report.
>
> On Wed, Oct 22, 2025 at 02:05:36PM +0300, Dan Carpenter wrote:
> > 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 }
> >
> > regards,
> > dan carpenter
>
> 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;
> }
>
> 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.
> Thanks,
>
> Dawei
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode
2025-10-22 16:41 ` Mathieu Poirier
@ 2025-11-10 17:05 ` Mathieu Poirier
2025-11-11 12:53 ` Dawei Li
0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Poirier @ 2025-11-10 17:05 UTC (permalink / raw)
To: Dawei Li; +Cc: Dan Carpenter, linux-remoteproc, andersson
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:
> >
> > Hi Dan,
> >
> > Thanks for the report.
> >
> > On Wed, Oct 22, 2025 at 02:05:36PM +0300, Dan Carpenter wrote:
> > > 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 }
> > >
> > > regards,
> > > dan carpenter
> >
> > 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;
> > }
> >
> > 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.
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?
>
> > Thanks,
> >
> > Dawei
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode
2025-11-10 17:05 ` Mathieu Poirier
@ 2025-11-11 12:53 ` Dawei Li
2025-11-11 16:41 ` Mathieu Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Dawei Li @ 2025-11-11 12:53 UTC (permalink / raw)
To: Mathieu Poirier; +Cc: Dan Carpenter, linux-remoteproc, andersson, dawei.li
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] rpmsg: char: Implement eptdev based on anonymous inode
2025-11-11 12:53 ` Dawei Li
@ 2025-11-11 16:41 ` Mathieu Poirier
0 siblings, 0 replies; 6+ messages in thread
From: Mathieu Poirier @ 2025-11-11 16:41 UTC (permalink / raw)
To: Dawei Li; +Cc: Dan Carpenter, linux-remoteproc, andersson
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-11 16:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-22 11:05 [bug report] rpmsg: char: Implement eptdev based on anonymous inode Dan Carpenter
2025-10-22 15:53 ` Dawei Li
2025-10-22 16:41 ` Mathieu Poirier
2025-11-10 17:05 ` Mathieu Poirier
2025-11-11 12:53 ` Dawei Li
2025-11-11 16:41 ` Mathieu Poirier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.