* [PATCH v2 0/2] Fix two kernel warnings in glink driver
@ 2021-10-25 23:37 Sujit Kautkar
2021-10-25 23:37 ` [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release() Sujit Kautkar
2021-10-25 23:37 ` [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device() Sujit Kautkar
0 siblings, 2 replies; 5+ messages in thread
From: Sujit Kautkar @ 2021-10-25 23:37 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen
Cc: Stephen Boyd, Sibi Sankar, Bjorn Andersson, Sujit Kautkar,
linux-arm-msm, linux-kernel, linux-remoteproc
These changes addresses kernel warnings which shows up after enabling
debug kernel. First one fixes use-after-free warning and second fixes
warning by updating cdev APIs
Changes in v2:
- Fix typo in commit message
Sujit Kautkar (2):
rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release()
rpmsg: glink: Update cdev add/del API in
rpmsg_ctrldev_release_device()
drivers/rpmsg/qcom_glink_native.c | 5 ++++-
drivers/rpmsg/rpmsg_char.c | 5 ++---
2 files changed, 6 insertions(+), 4 deletions(-)
--
2.31.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release()
2021-10-25 23:37 [PATCH v2 0/2] Fix two kernel warnings in glink driver Sujit Kautkar
@ 2021-10-25 23:37 ` Sujit Kautkar
2021-10-26 0:29 ` Matthias Kaehlcke
2021-10-25 23:37 ` [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device() Sujit Kautkar
1 sibling, 1 reply; 5+ messages in thread
From: Sujit Kautkar @ 2021-10-25 23:37 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen
Cc: Stephen Boyd, Sibi Sankar, Bjorn Andersson, Sujit Kautkar,
linux-arm-msm, linux-kernel, linux-remoteproc
qcom_glink_rpdev_release() sets channel->rpdev to NULL. However, with
debug enabled kernel, qcom_glink_rpdev_release() gets delayed due to
delayed kobject release and channel gets released by that time and
triggers below kernel warning. To avoid this use-after-free, add a
condition to checks if channel was already released.
| BUG: KASAN: use-after-free in qcom_glink_rpdev_release+0x54/0x70
| Write of size 8 at addr ffffffaba438e8d0 by task kworker/6:1/54
|
| CPU: 6 PID: 54 Comm: kworker/6:1 Not tainted 5.4.109-lockdep #16
| Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
| Workqueue: events kobject_delayed_cleanup
| Call trace:
| dump_backtrace+0x0/0x284
| show_stack+0x20/0x2c
| dump_stack+0xd4/0x170
| print_address_description+0x3c/0x4a8
| __kasan_report+0x144/0x168
| kasan_report+0x10/0x18
| __asan_report_store8_noabort+0x1c/0x24
| qcom_glink_rpdev_release+0x54/0x70
| device_release+0x68/0x14c
| kobject_delayed_cleanup+0x158/0x2cc
| process_one_work+0x7cc/0x10a4
| worker_thread+0x80c/0xcec
| kthread+0x2a8/0x314
| ret_from_fork+0x10/0x18
Signed-off-by: Sujit Kautkar <sujitka@chromium.org>
---
(no changes since v1)
drivers/rpmsg/qcom_glink_native.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index e1444fefdd1c0..cc3556a9385a9 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -270,6 +270,7 @@ static void qcom_glink_channel_release(struct kref *ref)
spin_unlock_irqrestore(&channel->intent_lock, flags);
kfree(channel->name);
+ channel = NULL;
kfree(channel);
}
@@ -1372,8 +1373,10 @@ static void qcom_glink_rpdev_release(struct device *dev)
{
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
struct glink_channel *channel = to_glink_channel(rpdev->ept);
+ if (channel) {
+ channel->rpdev = NULL;
+ }
- channel->rpdev = NULL;
kfree(rpdev);
}
--
2.31.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device()
2021-10-25 23:37 [PATCH v2 0/2] Fix two kernel warnings in glink driver Sujit Kautkar
2021-10-25 23:37 ` [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release() Sujit Kautkar
@ 2021-10-25 23:37 ` Sujit Kautkar
2021-10-26 0:39 ` Matthias Kaehlcke
1 sibling, 1 reply; 5+ messages in thread
From: Sujit Kautkar @ 2021-10-25 23:37 UTC (permalink / raw)
To: Andy Gross, Ohad Ben-Cohen
Cc: Stephen Boyd, Sibi Sankar, Bjorn Andersson, Sujit Kautkar,
linux-kernel, linux-remoteproc
Replace cdev add/del APIs with cdev_device_add/cdev_device_del to avoid
below kernel warning. This correctly takes a reference to the parent
device so the parent will not get released until all references to the
cdev are released.
| ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x7c
| WARNING: CPU: 7 PID: 19892 at lib/debugobjects.c:488 debug_print_object+0x13c/0x1b0
| CPU: 7 PID: 19892 Comm: kworker/7:4 Tainted: G W 5.4.147-lockdep #1
| ==================================================================
| Hardware name: Google CoachZ (rev1 - 2) with LTE (DT)
| Workqueue: events kobject_delayed_cleanup
| pstate: 60c00009 (nZCv daif +PAN +UAO)
| pc : debug_print_object+0x13c/0x1b0
| lr : debug_print_object+0x13c/0x1b0
| sp : ffffff83b2ec7970
| x29: ffffff83b2ec7970 x28: dfffffd000000000
| x27: ffffff83d674f000 x26: dfffffd000000000
| x25: ffffffd06b8fa660 x24: dfffffd000000000
| x23: 0000000000000000 x22: ffffffd06b7c5108
| x21: ffffffd06d597860 x20: ffffffd06e2c21c0
| x19: ffffffd06d5974c0 x18: 000000000001dad8
| x17: 0000000000000000 x16: dfffffd000000000
| BUG: KASAN: use-after-free in qcom_glink_rpdev_release+0x54/0x70
| x15: ffffffffffffffff x14: 79616c6564203a74
| x13: 0000000000000000 x12: 0000000000000080
| Write of size 8 at addr ffffff83d95768d0 by task kworker/3:1/150
| x11: 0000000000000001 x10: 0000000000000000
| x9 : fc9e8edec0ad0300 x8 : fc9e8edec0ad0300
|
| x7 : 0000000000000000 x6 : 0000000000000000
| x5 : 0000000000000080 x4 : 0000000000000000
| CPU: 3 PID: 150 Comm: kworker/3:1 Tainted: G W 5.4.147-lockdep #1
| x3 : ffffffd06c149574 x2 : ffffff83f77f7498
| x1 : ffffffd06d596f60 x0 : 0000000000000061
| Hardware name: Google CoachZ (rev1 - 2) with LTE (DT)
| Call trace:
| debug_print_object+0x13c/0x1b0
| Workqueue: events kobject_delayed_cleanup
| __debug_check_no_obj_freed+0x25c/0x3c0
| debug_check_no_obj_freed+0x18/0x20
| Call trace:
| slab_free_freelist_hook+0xb4/0x1bc
| kfree+0xe8/0x2d8
| dump_backtrace+0x0/0x27c
| rpmsg_ctrldev_release_device+0x78/0xb8
| device_release+0x68/0x14c
| show_stack+0x20/0x2c
| kobject_cleanup+0x12c/0x298
| kobject_delayed_cleanup+0x10/0x18
| dump_stack+0xe0/0x19c
| process_one_work+0x578/0x92c
| worker_thread+0x804/0xcf8
| print_address_description+0x3c/0x4a8
| kthread+0x2a8/0x314
| ret_from_fork+0x10/0x18
| __kasan_report+0x100/0x124
Signed-off-by: Sujit Kautkar <sujitka@chromium.org>
---
(no changes since v1)
drivers/rpmsg/rpmsg_char.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 876ce43df732b..b63a5c396da57 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -458,7 +458,7 @@ static void rpmsg_ctrldev_release_device(struct device *dev)
ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
- cdev_del(&ctrldev->cdev);
+ cdev_device_del(&ctrldev->cdev, &ctrldev->dev);
kfree(ctrldev);
}
@@ -493,14 +493,13 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
dev->id = ret;
dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
- ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
+ ret = cdev_device_add(&ctrldev->cdev, &ctrldev->dev);
if (ret)
goto free_ctrl_ida;
/* We can now rely on the release function for cleanup */
dev->release = rpmsg_ctrldev_release_device;
- ret = device_add(dev);
if (ret) {
dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
put_device(dev);
--
2.31.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release()
2021-10-25 23:37 ` [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release() Sujit Kautkar
@ 2021-10-26 0:29 ` Matthias Kaehlcke
0 siblings, 0 replies; 5+ messages in thread
From: Matthias Kaehlcke @ 2021-10-26 0:29 UTC (permalink / raw)
To: Sujit Kautkar
Cc: Andy Gross, Ohad Ben-Cohen, Stephen Boyd, Sibi Sankar,
Bjorn Andersson, linux-arm-msm, linux-kernel, linux-remoteproc
On Mon, Oct 25, 2021 at 04:37:52PM -0700, Sujit Kautkar wrote:
> qcom_glink_rpdev_release() sets channel->rpdev to NULL. However, with
> debug enabled kernel, qcom_glink_rpdev_release() gets delayed due to
> delayed kobject release and channel gets released by that time and
> triggers below kernel warning. To avoid this use-after-free, add a
> condition to checks if channel was already released.
>
> | BUG: KASAN: use-after-free in qcom_glink_rpdev_release+0x54/0x70
> | Write of size 8 at addr ffffffaba438e8d0 by task kworker/6:1/54
> |
> | CPU: 6 PID: 54 Comm: kworker/6:1 Not tainted 5.4.109-lockdep #16
> | Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
> | Workqueue: events kobject_delayed_cleanup
> | Call trace:
> | dump_backtrace+0x0/0x284
> | show_stack+0x20/0x2c
> | dump_stack+0xd4/0x170
> | print_address_description+0x3c/0x4a8
> | __kasan_report+0x144/0x168
> | kasan_report+0x10/0x18
> | __asan_report_store8_noabort+0x1c/0x24
> | qcom_glink_rpdev_release+0x54/0x70
> | device_release+0x68/0x14c
> | kobject_delayed_cleanup+0x158/0x2cc
> | process_one_work+0x7cc/0x10a4
> | worker_thread+0x80c/0xcec
> | kthread+0x2a8/0x314
> | ret_from_fork+0x10/0x18
>
> Signed-off-by: Sujit Kautkar <sujitka@chromium.org>
> ---
>
> (no changes since v1)
>
> drivers/rpmsg/qcom_glink_native.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index e1444fefdd1c0..cc3556a9385a9 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -270,6 +270,7 @@ static void qcom_glink_channel_release(struct kref *ref)
> spin_unlock_irqrestore(&channel->intent_lock, flags);
>
> kfree(channel->name);
> + channel = NULL;
This doesn't make much sense, 'channel' is a local variable, the only effect
this has is that the memory of 'channel' isn't freed by the 'kfree' below.
Maybe this is debug code and wasn't intended to be part of this patch?
> kfree(channel);
> }
>
> @@ -1372,8 +1373,10 @@ static void qcom_glink_rpdev_release(struct device *dev)
> {
> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> struct glink_channel *channel = to_glink_channel(rpdev->ept);
> + if (channel) {
> + channel->rpdev = NULL;
> + }
Remove curly braces for single line branch.
>
> - channel->rpdev = NULL;
> kfree(rpdev);
> }
>
> --
> 2.31.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device()
2021-10-25 23:37 ` [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device() Sujit Kautkar
@ 2021-10-26 0:39 ` Matthias Kaehlcke
0 siblings, 0 replies; 5+ messages in thread
From: Matthias Kaehlcke @ 2021-10-26 0:39 UTC (permalink / raw)
To: Sujit Kautkar
Cc: Andy Gross, Ohad Ben-Cohen, Stephen Boyd, Sibi Sankar,
Bjorn Andersson, linux-kernel, linux-remoteproc
On Mon, Oct 25, 2021 at 04:37:54PM -0700, Sujit Kautkar wrote:
> Replace cdev add/del APIs with cdev_device_add/cdev_device_del to avoid
> below kernel warning. This correctly takes a reference to the parent
> device so the parent will not get released until all references to the
> cdev are released.
>
> | ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x7c
> | WARNING: CPU: 7 PID: 19892 at lib/debugobjects.c:488 debug_print_object+0x13c/0x1b0
> | CPU: 7 PID: 19892 Comm: kworker/7:4 Tainted: G W 5.4.147-lockdep #1
> | ==================================================================
> | Hardware name: Google CoachZ (rev1 - 2) with LTE (DT)
> | Workqueue: events kobject_delayed_cleanup
> | pstate: 60c00009 (nZCv daif +PAN +UAO)
> | pc : debug_print_object+0x13c/0x1b0
> | lr : debug_print_object+0x13c/0x1b0
> | sp : ffffff83b2ec7970
> | x29: ffffff83b2ec7970 x28: dfffffd000000000
> | x27: ffffff83d674f000 x26: dfffffd000000000
> | x25: ffffffd06b8fa660 x24: dfffffd000000000
> | x23: 0000000000000000 x22: ffffffd06b7c5108
> | x21: ffffffd06d597860 x20: ffffffd06e2c21c0
> | x19: ffffffd06d5974c0 x18: 000000000001dad8
> | x17: 0000000000000000 x16: dfffffd000000000
> | BUG: KASAN: use-after-free in qcom_glink_rpdev_release+0x54/0x70
> | x15: ffffffffffffffff x14: 79616c6564203a74
> | x13: 0000000000000000 x12: 0000000000000080
> | Write of size 8 at addr ffffff83d95768d0 by task kworker/3:1/150
> | x11: 0000000000000001 x10: 0000000000000000
> | x9 : fc9e8edec0ad0300 x8 : fc9e8edec0ad0300
> |
> | x7 : 0000000000000000 x6 : 0000000000000000
> | x5 : 0000000000000080 x4 : 0000000000000000
> | CPU: 3 PID: 150 Comm: kworker/3:1 Tainted: G W 5.4.147-lockdep #1
> | x3 : ffffffd06c149574 x2 : ffffff83f77f7498
> | x1 : ffffffd06d596f60 x0 : 0000000000000061
> | Hardware name: Google CoachZ (rev1 - 2) with LTE (DT)
> | Call trace:
> | debug_print_object+0x13c/0x1b0
> | Workqueue: events kobject_delayed_cleanup
> | __debug_check_no_obj_freed+0x25c/0x3c0
> | debug_check_no_obj_freed+0x18/0x20
> | Call trace:
> | slab_free_freelist_hook+0xb4/0x1bc
> | kfree+0xe8/0x2d8
> | dump_backtrace+0x0/0x27c
> | rpmsg_ctrldev_release_device+0x78/0xb8
> | device_release+0x68/0x14c
> | show_stack+0x20/0x2c
> | kobject_cleanup+0x12c/0x298
> | kobject_delayed_cleanup+0x10/0x18
> | dump_stack+0xe0/0x19c
> | process_one_work+0x578/0x92c
> | worker_thread+0x804/0xcf8
> | print_address_description+0x3c/0x4a8
> | kthread+0x2a8/0x314
> | ret_from_fork+0x10/0x18
> | __kasan_report+0x100/0x124
>
> Signed-off-by: Sujit Kautkar <sujitka@chromium.org>
> ---
>
> (no changes since v1)
>
> drivers/rpmsg/rpmsg_char.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 876ce43df732b..b63a5c396da57 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -458,7 +458,7 @@ static void rpmsg_ctrldev_release_device(struct device *dev)
>
> ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> - cdev_del(&ctrldev->cdev);
> + cdev_device_del(&ctrldev->cdev, &ctrldev->dev);
> kfree(ctrldev);
> }
>
> @@ -493,14 +493,13 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> dev->id = ret;
> dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
>
> - ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> + ret = cdev_device_add(&ctrldev->cdev, &ctrldev->dev);
> if (ret)
> goto free_ctrl_ida;
>
> /* We can now rely on the release function for cleanup */
> dev->release = rpmsg_ctrldev_release_device;
>
> - ret = device_add(dev);
> if (ret) {
> dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> put_device(dev);
Also remove the error check? There is already a check above for the
status of 'cdev_device_add'.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-26 0:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 23:37 [PATCH v2 0/2] Fix two kernel warnings in glink driver Sujit Kautkar
2021-10-25 23:37 ` [PATCH v2 1/2] rpmsg: glink: Fix use-after-free in qcom_glink_rpdev_release() Sujit Kautkar
2021-10-26 0:29 ` Matthias Kaehlcke
2021-10-25 23:37 ` [PATCH v2 2/2] rpmsg: glink: Update cdev add/del API in rpmsg_ctrldev_release_device() Sujit Kautkar
2021-10-26 0:39 ` Matthias Kaehlcke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).