* [PATCH] virtio-mmio: don't break lifecycle of vm_dev
@ 2023-06-29 12:05 Wolfram Sang
2023-07-03 2:11 ` Xuan Zhuo
0 siblings, 1 reply; 3+ messages in thread
From: Wolfram Sang @ 2023-06-29 12:05 UTC (permalink / raw)
To: linux-renesas-soc
Cc: Wolfram Sang, Michael S. Tsirkin, Jason Wang, Xuan Zhuo,
weiping zhang, Cornelia Huck, virtualization, linux-kernel
vm_dev has a separate lifecycle because it has a 'struct device'
embedded. Thus, having a release callback for it is correct.
Allocating the vm_dev struct with devres totally breaks this protection,
though. Instead of waiting for the vm_dev release callback, the memory
is freed when the platform_device is removed. Resulting in a
use-after-free when finally the callback is to be called.
To easily see the problem, compile the kernel with
CONFIG_DEBUG_KOBJECT_RELEASE and unbind with sysfs.
The fix is easy, don't use devres in this case.
Found during my research about object lifetime problems.
Fixes: 7eb781b1bbb7 ("virtio_mmio: add cleanup for virtio_mmio_probe")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
drivers/virtio/virtio_mmio.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index a46a4a29e929..97760f611295 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -607,9 +607,8 @@ static void virtio_mmio_release_dev(struct device *_d)
struct virtio_device *vdev =
container_of(_d, struct virtio_device, dev);
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
- struct platform_device *pdev = vm_dev->pdev;
- devm_kfree(&pdev->dev, vm_dev);
+ kfree(vm_dev);
}
/* Platform device */
@@ -620,7 +619,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
unsigned long magic;
int rc;
- vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
+ vm_dev = kzalloc(sizeof(*vm_dev), GFP_KERNEL);
if (!vm_dev)
return -ENOMEM;
--
2.35.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] virtio-mmio: don't break lifecycle of vm_dev
2023-06-29 12:05 [PATCH] virtio-mmio: don't break lifecycle of vm_dev Wolfram Sang
@ 2023-07-03 2:11 ` Xuan Zhuo
2023-07-03 20:43 ` Wolfram Sang
0 siblings, 1 reply; 3+ messages in thread
From: Xuan Zhuo @ 2023-07-03 2:11 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, Michael S. Tsirkin, Jason Wang, weiping zhang,
Cornelia Huck, virtualization, linux-kernel, linux-renesas-soc
On Thu, 29 Jun 2023 14:05:26 +0200, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> vm_dev has a separate lifecycle because it has a 'struct device'
> embedded. Thus, having a release callback for it is correct.
>
> Allocating the vm_dev struct with devres totally breaks this protection,
device? or driver?
And why?
> though. Instead of waiting for the vm_dev release callback, the memory
> is freed when the platform_device is removed. Resulting in a
> use-after-free when finally the callback is to be called.
Can we have the break stack?
Thanks.
>
> To easily see the problem, compile the kernel with
> CONFIG_DEBUG_KOBJECT_RELEASE and unbind with sysfs.
>
> The fix is easy, don't use devres in this case.
>
> Found during my research about object lifetime problems.
>
> Fixes: 7eb781b1bbb7 ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/virtio/virtio_mmio.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a46a4a29e929..97760f611295 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -607,9 +607,8 @@ static void virtio_mmio_release_dev(struct device *_d)
> struct virtio_device *vdev =
> container_of(_d, struct virtio_device, dev);
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> - struct platform_device *pdev = vm_dev->pdev;
>
> - devm_kfree(&pdev->dev, vm_dev);
> + kfree(vm_dev);
> }
>
> /* Platform device */
> @@ -620,7 +619,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> unsigned long magic;
> int rc;
>
> - vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> + vm_dev = kzalloc(sizeof(*vm_dev), GFP_KERNEL);
> if (!vm_dev)
> return -ENOMEM;
>
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] virtio-mmio: don't break lifecycle of vm_dev
2023-07-03 2:11 ` Xuan Zhuo
@ 2023-07-03 20:43 ` Wolfram Sang
0 siblings, 0 replies; 3+ messages in thread
From: Wolfram Sang @ 2023-07-03 20:43 UTC (permalink / raw)
To: Xuan Zhuo
Cc: Michael S. Tsirkin, Jason Wang, weiping zhang, Cornelia Huck,
virtualization, linux-kernel, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 4455 bytes --]
> > Allocating the vm_dev struct with devres totally breaks this protection,
>
> device? or driver?
devres is the short name for 'managed devices'. That is, all the devm_*
functions.
> And why?
This is written in the below paragraph...
> > though. Instead of waiting for the vm_dev release callback, the memory
> > is freed when the platform_device is removed. Resulting in a
> > use-after-free when finally the callback is to be called.
... right here. Sadly, the video of my talk about device lifetime issues
is not online yet. But my slides are [1]. Please check pages 29-31.
[1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf
> Can we have the break stack?
Sure!
root@Lager:/sys/bus/platform/drivers/virtio-mmio# echo virtio-mmio.0 > unbind
[ 25.258132] kobject: 'virtio0' ((ptrval)): kobject_release, parent 00000000 (delayed 300)
[ 25.266592] ------------[ cut here ]------------
[ 25.271219] WARNING: CPU: 0 PID: 606 at lib/debugobjects.c:505 debug_print_object+0xa4/0xcc
[ 25.279595] ODEBUG: free active (active state 0) object: (ptrval) object type: timer_list hint: kobject_delayed_cleanup+0x0/0x68
[ 25.291186] Modules linked in:
[ 25.294250] CPU: 0 PID: 606 Comm: sh Tainted: G W 6.4.0-00001-g1279c634d910 #1215
[ 25.303134] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[ 25.309580] Backtrace:
[ 25.312032] dump_backtrace from show_stack+0x20/0x24
[ 25.317104] r7:00000009 r6:c0745e4a r5:60000093 r4:c073ddc2
[ 25.322768] show_stack from dump_stack_lvl+0x2c/0x34
[ 25.327836] dump_stack_lvl from dump_stack+0x14/0x1c
[ 25.332906] r5:c02fcc34 r4:c2db3880
[ 25.336484] dump_stack from __warn+0x90/0x118
[ 25.340944] __warn from warn_slowpath_fmt+0x88/0xcc
[ 25.345921] r10:c2607c78 r9:00000000 r8:c0745e02 r7:00000009 r6:c02fcc34 r5:000001f9
[ 25.353759] r4:c0745e4a
[ 25.356293] warn_slowpath_fmt from debug_print_object+0xa4/0xcc
[ 25.362314] r8:c0b1a3b4 r7:c06020f8 r6:c0735cc1 r5:c0a0492c r4:c1004a08
[ 25.369019] debug_print_object from debug_check_no_obj_freed+0x184/0x1bc
[ 25.375822] r8:c2607c00 r7:c06020f8 r6:c2db3880 r5:c2608000 r4:c2607000
[ 25.382527] debug_check_no_obj_freed from __kmem_cache_free+0x40/0x80
[ 25.389072] r10:00000004 r9:c2db3880 r8:c073257a r7:00000013 r6:c03952b0 r5:c2607c00
[ 25.396909] r4:c1000280
[ 25.399443] __kmem_cache_free from kfree+0x90/0x98
[ 25.404335] r7:f1581dec r6:c201e010 r5:c03952b0 r4:c2607c00
[ 25.409998] kfree from release_nodes+0x58/0x68
[ 25.414548] r7:f1581dec r6:c201e010 r5:f1581dec r4:c2607c00
[ 25.420210] release_nodes from devres_release_all+0xb8/0xe8
[ 25.425891] r9:c2db3880 r8:c0a9e1d0 r7:80000013 r6:c2db3880 r5:00000004 r4:c201e010
[ 25.433640] devres_release_all from device_unbind_cleanup+0x1c/0x70
[ 25.440022] r7:c0a99488 r6:00000000 r5:c0a99290 r4:c201e010
[ 25.445684] device_unbind_cleanup from device_release_driver_internal+0xb4/0x128
[ 25.453187] r5:c0a99290 r4:c201e010
[ 25.456764] device_release_driver_internal from device_driver_detach+0x20/0x24
[ 25.464088] r7:c0a99488 r6:c201e010 r5:c0a9e1d0 r4:0000000e
[ 25.469750] device_driver_detach from unbind_store+0x60/0x94
[ 25.475512] unbind_store from drv_attr_store+0x34/0x40
[ 25.480759] r9:c2db3880 r8:f1581f00 r7:c2d43a10 r6:c2d43a00 r5:c2d37e40 r4:c038ecfc
[ 25.488508] drv_attr_store from sysfs_kf_write+0x48/0x54
[ 25.493928] sysfs_kf_write from kernfs_fop_write_iter+0x160/0x1ac
[ 25.500128] r5:c2d37e40 r4:0000000e
[ 25.503705] kernfs_fop_write_iter from vfs_write+0x154/0x1b4
[ 25.509469] r8:00000000 r7:000f3070 r6:f1581f58 r5:c24a36c0 r4:0000000e
[ 25.516173] vfs_write from ksys_write+0x84/0xd8
[ 25.520802] r8:0000000e r7:f1581f64 r6:f1581f58 r5:000f3070 r4:c24a36c0
[ 25.527507] ksys_write from sys_write+0x18/0x1c
[ 25.532135] r8:c0100264 r7:00000004 r6:0000000e r5:000f3070 r4:00000001
[ 25.538839] sys_write from ret_fast_syscall+0x0/0x54
[ 25.543901] Exception stack(0xf1581fa8 to 0xf1581ff0)
[ 25.548960] 1fa0: 00000001 000f3070 00000001 000f3070 0000000e 00000000
[ 25.557147] 1fc0: 00000001 000f3070 0000000e 00000004 000f09f4 00000020 000f1284 000f1418
[ 25.565333] 1fe0: 000f0250 beaca868 00018ef0 b6f40ae8
[ 25.570387] ---[ end trace 0000000000000000 ]---
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-03 20:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-29 12:05 [PATCH] virtio-mmio: don't break lifecycle of vm_dev Wolfram Sang
2023-07-03 2:11 ` Xuan Zhuo
2023-07-03 20:43 ` Wolfram Sang
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).