* [PATCH v5 0/3] media: vimc: release vimc in release cb of v4l2_device
@ 2020-01-22 16:01 Dafna Hirschfeld
2020-01-22 16:01 ` [PATCH v5 1/3] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Dafna Hirschfeld @ 2020-01-22 16:01 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
I decided to split the patchset "race condition fixes" into
several patchsets since it deals with several bugs that are independent
and also, some of the fixes deal only with code in vimc and other fixes code in
the framework.
This patchset fixes only the first bug from the last patchset which is
a bug only in vimc:
Currently when the device is unbounded, the vimc entities are
released immediately. If the device is streaming,
the function `vimc_streamer_pipeline_terminate` is called when
unregistering the streaming capture device and it references the
already freed vimc entities.
The patchset solves this by deferring the release
to the release callback of v4l2_device. This ensures that
the vimc entities will be released after the last fh is closed
and so the streaming terminates before.
The detail of how to reproduce this bug are described in the patch:
"use-after-free fix - release vimc in the v4l_device release"
The bug is easily detected with a KASAN report.
Changes since v4:
- make the unregister callback optional and implement it only for vimc-capture entities.
This is because the subdevices are unregisterd anyway in v4l2_device_unregister.
Changes since v3:
- add a more precise description of the first bug in the third patch and replace the crash report with the KASAN report
Changes from v2:
in patch 3, in case of failure in the probe function the memory is released
directly from the probe function, and only on success path the release callback
of v4l2_device is assigned
Changes from v1:
v1 solved it by adding refcount to each entity.
(patch "media: vimc: crash fix - add refcount to vimc entities")
v2 is different solution due to comments from Hans Verkuil
Patches Summary:
- The first patch replaces the usage of vimc_device.pdev.dev with vimc.mdev.dev
- The second patch allocates the vimc_device dynamically.
This is needed since the release of the device is deferred
and might run after the device is initialized again.
- The third patch solves the use-after-free bug by moving the release of the vimc_device
and all the vimc entities to the release callback of v4l2_device.
Dafna Hirschfeld (3):
media: vimc: replace vimc->pdev.dev with vimc->mdev.dev
media: vimc: allocate vimc_device dynamically
media: vimc: use-after-free fix - release vimc in the v4l_device
release
drivers/media/platform/vimc/vimc-capture.c | 18 ++---
drivers/media/platform/vimc/vimc-common.c | 2 -
drivers/media/platform/vimc/vimc-common.h | 27 +++----
drivers/media/platform/vimc/vimc-core.c | 94 ++++++++++++++--------
drivers/media/platform/vimc/vimc-debayer.c | 21 +----
drivers/media/platform/vimc/vimc-scaler.c | 21 +----
drivers/media/platform/vimc/vimc-sensor.c | 20 +----
7 files changed, 96 insertions(+), 107 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v5 1/3] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev 2020-01-22 16:01 [PATCH v5 0/3] media: vimc: release vimc in release cb of v4l2_device Dafna Hirschfeld @ 2020-01-22 16:01 ` Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 2/3] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld 2 siblings, 0 replies; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-22 16:01 UTC (permalink / raw) To: linux-media Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3 replace 'vimc->pdev.dev' with 'vimc->mdev.dev' in debug prints and in assignment to vimc_ent_device.dev. This helps to unify the debug statements. This will also eliminate the need to use the pdev field in vimc_device in future patch. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/media/platform/vimc/vimc-capture.c | 6 +++--- drivers/media/platform/vimc/vimc-core.c | 4 ++-- drivers/media/platform/vimc/vimc-debayer.c | 2 +- drivers/media/platform/vimc/vimc-scaler.c | 2 +- drivers/media/platform/vimc/vimc-sensor.c | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c index 76c015898cfd..9a78bb7826a8 100644 --- a/drivers/media/platform/vimc/vimc-capture.c +++ b/drivers/media/platform/vimc/vimc-capture.c @@ -423,7 +423,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, ret = vb2_queue_init(q); if (ret) { - dev_err(&vimc->pdev.dev, "%s: vb2 queue init failed (err=%d)\n", + dev_err(vimc->mdev.dev, "%s: vb2 queue init failed (err=%d)\n", vcfg_name, ret); goto err_clean_m_ent; } @@ -443,7 +443,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, vcap->ved.ent = &vcap->vdev.entity; vcap->ved.process_frame = vimc_cap_process_frame; vcap->ved.vdev_get_format = vimc_cap_get_format; - vcap->ved.dev = &vimc->pdev.dev; + vcap->ved.dev = vimc->mdev.dev; /* Initialize the video_device struct */ vdev = &vcap->vdev; @@ -462,7 +462,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, /* Register the video_device with the v4l2 and the media framework */ ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1); if (ret) { - dev_err(&vimc->pdev.dev, "%s: video register failed (err=%d)\n", + dev_err(vimc->mdev.dev, "%s: video register failed (err=%d)\n", vcap->vdev.name, ret); goto err_release_queue; } diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c index 97a272f3350a..51c89fc79d90 100644 --- a/drivers/media/platform/vimc/vimc-core.c +++ b/drivers/media/platform/vimc/vimc-core.c @@ -162,12 +162,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) unsigned int i; for (i = 0; i < vimc->pipe_cfg->num_ents; i++) { - dev_dbg(&vimc->pdev.dev, "new entity for %s\n", + dev_dbg(vimc->mdev.dev, "new entity for %s\n", vimc->pipe_cfg->ents[i].name); vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc, vimc->pipe_cfg->ents[i].name); if (!vimc->ent_devs[i]) { - dev_err(&vimc->pdev.dev, "add new entity for %s\n", + dev_err(vimc->mdev.dev, "add new entity for %s\n", vimc->pipe_cfg->ents[i].name); return -EINVAL; } diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c index 5d1b67d684bb..34b98b235880 100644 --- a/drivers/media/platform/vimc/vimc-debayer.c +++ b/drivers/media/platform/vimc/vimc-debayer.c @@ -569,7 +569,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, goto err_free_hdl; vdeb->ved.process_frame = vimc_deb_process_frame; - vdeb->ved.dev = &vimc->pdev.dev; + vdeb->ved.dev = vimc->mdev.dev; vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def; /* Initialize the frame format */ diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c index e2e551bc3ded..cb2dc24c3e59 100644 --- a/drivers/media/platform/vimc/vimc-scaler.c +++ b/drivers/media/platform/vimc/vimc-scaler.c @@ -512,7 +512,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, } vsca->ved.process_frame = vimc_sca_process_frame; - vsca->ved.dev = &vimc->pdev.dev; + vsca->ved.dev = vimc->mdev.dev; /* Initialize the frame format */ vsca->sink_fmt = sink_fmt_default; diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c index 32380f504591..9788fe291193 100644 --- a/drivers/media/platform/vimc/vimc-sensor.c +++ b/drivers/media/platform/vimc/vimc-sensor.c @@ -370,7 +370,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, goto err_free_tpg; vsen->ved.process_frame = vimc_sen_process_frame; - vsen->ved.dev = &vimc->pdev.dev; + vsen->ved.dev = vimc->mdev.dev; /* Initialize the frame format */ vsen->mbus_format = fmt_default; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/3] media: vimc: allocate vimc_device dynamically 2020-01-22 16:01 [PATCH v5 0/3] media: vimc: release vimc in release cb of v4l2_device Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 1/3] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld @ 2020-01-22 16:01 ` Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld 2 siblings, 0 replies; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-22 16:01 UTC (permalink / raw) To: linux-media Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3 In future patch, the release of the device will move to the release callback of v4l2_device. Therefore the device will be released only when the last fh will be closed. Dynamic allocation will then be needed since when the device is unbounded and then bounded again, it might be that the probe callback will run before the release of the last device is finished. In that case both operations will run on the same memory concurrently and cause memory corruption. This patch also removes the pdev field of vimc_device since it is not needed anymore. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/media/platform/vimc/vimc-common.h | 2 -- drivers/media/platform/vimc/vimc-core.c | 31 +++++++++++++---------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h index 87eb8259c2a8..1b6ef7196f3c 100644 --- a/drivers/media/platform/vimc/vimc-common.h +++ b/drivers/media/platform/vimc/vimc-common.h @@ -106,14 +106,12 @@ struct vimc_ent_device { /** * struct vimc_device - main device for vimc driver * - * @pdev pointer to the platform device * @pipe_cfg pointer to the vimc pipeline configuration structure * @ent_devs array of vimc_ent_device pointers * @mdev the associated media_device parent * @v4l2_dev Internal v4l2 parent device */ struct vimc_device { - struct platform_device pdev; const struct vimc_pipeline_config *pipe_cfg; struct vimc_ent_device **ent_devs; struct media_device mdev; diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c index 51c89fc79d90..1c55e0382f09 100644 --- a/drivers/media/platform/vimc/vimc-core.c +++ b/drivers/media/platform/vimc/vimc-core.c @@ -252,16 +252,21 @@ static void vimc_unregister(struct vimc_device *vimc) media_device_cleanup(&vimc->mdev); v4l2_device_unregister(&vimc->v4l2_dev); kfree(vimc->ent_devs); + kfree(vimc); } static int vimc_probe(struct platform_device *pdev) { - struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev); + struct vimc_device *vimc; int ret; dev_dbg(&pdev->dev, "probe"); - memset(&vimc->mdev, 0, sizeof(vimc->mdev)); + vimc = kzalloc(sizeof(*vimc), GFP_KERNEL); + if (!vimc) + return -ENOMEM; + + vimc->pipe_cfg = &pipe_cfg; /* Link the media device within the v4l2_device */ vimc->v4l2_dev.mdev = &vimc->mdev; @@ -277,15 +282,17 @@ static int vimc_probe(struct platform_device *pdev) ret = vimc_register_devices(vimc); if (ret) { media_device_cleanup(&vimc->mdev); + kfree(vimc); return ret; } + platform_set_drvdata(pdev, vimc); return 0; } static int vimc_remove(struct platform_device *pdev) { - struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev); + struct vimc_device *vimc = platform_get_drvdata(pdev); dev_dbg(&pdev->dev, "remove"); @@ -299,12 +306,10 @@ static void vimc_dev_release(struct device *dev) { } -static struct vimc_device vimc_dev = { - .pipe_cfg = &pipe_cfg, - .pdev = { - .name = VIMC_PDEV_NAME, - .dev.release = vimc_dev_release, - } + +static struct platform_device vimc_pdev = { + .name = VIMC_PDEV_NAME, + .dev.release = vimc_dev_release, }; static struct platform_driver vimc_pdrv = { @@ -319,16 +324,16 @@ static int __init vimc_init(void) { int ret; - ret = platform_device_register(&vimc_dev.pdev); + ret = platform_device_register(&vimc_pdev); if (ret) { - dev_err(&vimc_dev.pdev.dev, + dev_err(&vimc_pdev.dev, "platform device registration failed (err=%d)\n", ret); return ret; } ret = platform_driver_register(&vimc_pdrv); if (ret) { - dev_err(&vimc_dev.pdev.dev, + dev_err(&vimc_pdev.dev, "platform driver registration failed (err=%d)\n", ret); platform_driver_unregister(&vimc_pdrv); return ret; @@ -341,7 +346,7 @@ static void __exit vimc_exit(void) { platform_driver_unregister(&vimc_pdrv); - platform_device_unregister(&vimc_dev.pdev); + platform_device_unregister(&vimc_pdev); } module_init(vimc_init); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-22 16:01 [PATCH v5 0/3] media: vimc: release vimc in release cb of v4l2_device Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 1/3] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 2/3] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld @ 2020-01-22 16:01 ` Dafna Hirschfeld 2020-01-22 16:39 ` Helen Koike 2 siblings, 1 reply; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-22 16:01 UTC (permalink / raw) To: linux-media Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3 A use-after-free bug occures when unbinding the device while it streams. The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed when calling the sensor's 'rm' callback but the freed pointer is later accessed in the function 'vimc_streamer_pipeline_terminate'. To fix this bug, move the release callback of the vimc entities and vimc_device to the release callback of v4l2_device. The .rm callback of vimc_ent_config is replaced by two callbacks: .unregister - this is called upon removing the device and it unregisters the entity. This is an optional callback since subdevices don't need to implement it because they are already unregistered in v4l2_device_unregister. .release - this is called from the release callback of v4l2_device and it frees the entity. This ensures that the entities will be released when the last fh of any of the devices is closed. The commands that cause the crash and the KASAN report: media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & sleep 1 echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 [ 188.421800] [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 [ 188.425938] Call Trace: [ 188.426610] dump_stack+0x75/0xa0 [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.429057] print_address_description.constprop.6+0x16/0x220 [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.433455] __kasan_report.cold.9+0x1a/0x40 [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.436010] kasan_report+0xe/0x20 [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] [ 188.446179] vimc_remove+0x19/0x70 [vimc] [ 188.447301] platform_drv_remove+0x2f/0x50 [ 188.448468] device_release_driver_internal+0x133/0x260 [ 188.449814] unbind_store+0x121/0x150 [ 188.450726] kernfs_fop_write+0x142/0x230 [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 [ 188.452826] vfs_write+0xdc/0x230 [ 188.453760] ksys_write+0xaf/0x140 [ 188.454702] ? __ia32_sys_read+0x40/0x40 [ 188.455773] ? __do_page_fault+0x473/0x620 [ 188.456780] do_syscall_64+0x5e/0x1a0 [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 188.459079] RIP: 0033:0x7f80f1f13504 [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53 [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 [ 188.475107] [ 188.475500] Allocated by task 473: [ 188.476351] save_stack+0x19/0x80 [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] [ 188.480776] platform_drv_probe+0x46/0xa0 [ 188.481829] really_probe+0x16c/0x520 [ 188.482732] driver_probe_device+0x114/0x170 [ 188.483783] device_driver_attach+0x85/0x90 [ 188.484800] __driver_attach+0xa8/0x190 [ 188.485734] bus_for_each_dev+0xe4/0x140 [ 188.486702] bus_add_driver+0x223/0x2d0 [ 188.487715] driver_register+0xca/0x140 [ 188.488767] 0xffffffffc037003d [ 188.489635] do_one_initcall+0x86/0x28f [ 188.490702] do_init_module+0xf8/0x340 [ 188.491773] load_module+0x3766/0x3a10 [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 [ 188.494059] do_syscall_64+0x5e/0x1a0 [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 188.496481] [ 188.496893] Freed by task 185: [ 188.497670] save_stack+0x19/0x80 [ 188.498493] __kasan_slab_free+0x125/0x170 [ 188.499486] kfree+0x8c/0x230 [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] [ 188.502976] device_release+0x3c/0xd0 [ 188.503867] kobject_put+0xf4/0x240 [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] [ 188.508846] vimc_remove+0x19/0x70 [vimc] [ 188.509792] platform_drv_remove+0x2f/0x50 [ 188.510752] device_release_driver_internal+0x133/0x260 [ 188.512006] unbind_store+0x121/0x150 [ 188.512899] kernfs_fop_write+0x142/0x230 [ 188.513874] vfs_write+0xdc/0x230 [ 188.514698] ksys_write+0xaf/0x140 [ 188.515523] do_syscall_64+0x5e/0x1a0 [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 188.517710] [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 [ 188.520528] The buggy address is located 8 bytes inside of [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) [ 188.523015] The buggy address belongs to the page: [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 [ 188.530883] page dumped because: kasan: bad access detected [ 188.532336] [ 188.532720] Memory state around the buggy address: [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [ 188.538996] ^ [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/media/platform/vimc/vimc-capture.c | 12 ++--- drivers/media/platform/vimc/vimc-common.c | 2 - drivers/media/platform/vimc/vimc-common.h | 25 ++++----- drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- 7 files changed, 71 insertions(+), 85 deletions(-) diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c index 9a78bb7826a8..c5a645f98c66 100644 --- a/drivers/media/platform/vimc/vimc-capture.c +++ b/drivers/media/platform/vimc/vimc-capture.c @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { .link_validate = vimc_vdev_link_validate, }; -static void vimc_cap_release(struct video_device *vdev) +void vimc_cap_release(struct vimc_ent_device *ved) { struct vimc_cap_device *vcap = - container_of(vdev, struct vimc_cap_device, vdev); + container_of(ved, struct vimc_cap_device, ved); media_entity_cleanup(vcap->ved.ent); kfree(vcap); } -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) +void vimc_cap_unregister(struct vimc_ent_device *ved) { - struct vimc_cap_device *vcap; + struct vimc_cap_device *vcap = + container_of(ved, struct vimc_cap_device, ved); - vcap = container_of(ved, struct vimc_cap_device, ved); vb2_queue_release(&vcap->queue); video_unregister_device(&vcap->vdev); } @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, vdev = &vcap->vdev; vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; vdev->entity.ops = &vimc_cap_mops; - vdev->release = vimc_cap_release; + vdev->release = video_device_release_empty; vdev->fops = &vimc_cap_fops; vdev->ioctl_ops = &vimc_cap_ioctl_ops; vdev->lock = &vcap->lock; diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c index 16ce9f3b7c75..c95c17c048f2 100644 --- a/drivers/media/platform/vimc/vimc-common.c +++ b/drivers/media/platform/vimc/vimc-common.c @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, u32 function, u16 num_pads, struct media_pad *pads, - const struct v4l2_subdev_internal_ops *sd_int_ops, const struct v4l2_subdev_ops *sd_ops) { int ret; @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, /* Initialize the subdev */ v4l2_subdev_init(sd, sd_ops); - sd->internal_ops = sd_int_ops; sd->entity.function = function; sd->entity.ops = &vimc_ent_sd_mops; sd->owner = THIS_MODULE; diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h index 1b6ef7196f3c..616d5a6b0754 100644 --- a/drivers/media/platform/vimc/vimc-common.h +++ b/drivers/media/platform/vimc/vimc-common.h @@ -125,16 +125,18 @@ struct vimc_device { * @name entity name * @ved pointer to vimc_ent_device (a node in the * topology) - * @add subdev add hook - initializes and registers - * subdev called from vimc-core - * @rm subdev rm hook - unregisters and frees - * subdev called from vimc-core + * @add initializes and registers + * vim entity - called from vimc-core + * @unregister unregisters vimc entity - called from vimc-core + * @release releases vimc entity - called from the v4l2_dev + * release callback */ struct vimc_ent_config { const char *name; struct vimc_ent_device *(*add)(struct vimc_device *vimc, const char *vcfg_name); - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); + void (*unregister)(struct vimc_ent_device *ved); + void (*release)(struct vimc_ent_device *ved); }; /** @@ -145,22 +147,23 @@ struct vimc_ent_config { */ bool vimc_is_source(struct media_entity *ent); -/* prototypes for vimc_ent_config add and rm hooks */ +/* prototypes for vimc_ent_config hooks */ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, const char *vcfg_name); -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); +void vimc_cap_unregister(struct vimc_ent_device *ved); +void vimc_cap_release(struct vimc_ent_device *ved); struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, const char *vcfg_name); -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); +void vimc_deb_release(struct vimc_ent_device *ved); struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, const char *vcfg_name); -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); +void vimc_sca_release(struct vimc_ent_device *ved); struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, const char *vcfg_name); -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); +void vimc_sen_release(struct vimc_ent_device *ved); /** * vimc_pix_map_by_index - get vimc_pix_map struct by its index @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); * @num_pads: number of pads to initialize * @pads: the array of pads of the entity, the caller should set the flags of the pads - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops * @sd_ops: pointer to &struct v4l2_subdev_ops. * * Helper function initialize and register the struct vimc_ent_device and struct @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, u32 function, u16 num_pads, struct media_pad *pads, - const struct v4l2_subdev_internal_ops *sd_int_ops, const struct v4l2_subdev_ops *sd_ops); /** diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c index 1c55e0382f09..306f9c293628 100644 --- a/drivers/media/platform/vimc/vimc-core.c +++ b/drivers/media/platform/vimc/vimc-core.c @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { { .name = "Sensor A", .add = vimc_sen_add, - .rm = vimc_sen_rm, + .release = vimc_sen_release, }, { .name = "Sensor B", .add = vimc_sen_add, - .rm = vimc_sen_rm, + .release = vimc_sen_release, }, { .name = "Debayer A", .add = vimc_deb_add, - .rm = vimc_deb_rm, + .release = vimc_deb_release, }, { .name = "Debayer B", .add = vimc_deb_add, - .rm = vimc_deb_rm, + .release = vimc_deb_release, }, { .name = "Raw Capture 0", .add = vimc_cap_add, - .rm = vimc_cap_rm, + .unregister = vimc_cap_unregister, + .release = vimc_cap_release, }, { .name = "Raw Capture 1", .add = vimc_cap_add, - .rm = vimc_cap_rm, + .unregister = vimc_cap_unregister, + .release = vimc_cap_release, }, { /* TODO: change this to vimc-input when it is implemented */ .name = "RGB/YUV Input", .add = vimc_sen_add, - .rm = vimc_sen_rm, + .release = vimc_sen_release, }, { .name = "Scaler", .add = vimc_sca_add, - .rm = vimc_sca_rm, + .release = vimc_sca_release, }, { .name = "RGB/YUV Capture", .add = vimc_cap_add, - .rm = vimc_cap_rm, + .unregister = vimc_cap_unregister, + .release = vimc_cap_release, }, }; @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) return 0; } -static void vimc_rm_subdevs(struct vimc_device *vimc) +static void vimc_release_subdevs(struct vimc_device *vimc) { unsigned int i; for (i = 0; i < vimc->pipe_cfg->num_ents; i++) if (vimc->ent_devs[i]) - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); +} + +static void vimc_unregister_subdevs(struct vimc_device *vimc) +{ + unsigned int i; + + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); +} + +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) +{ + struct vimc_device *vimc = + container_of(v4l2_dev, struct vimc_device, v4l2_dev); + + vimc_release_subdevs(vimc); + media_device_cleanup(&vimc->mdev); + kfree(vimc->ent_devs); + kfree(vimc); } static int vimc_register_devices(struct vimc_device *vimc) @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) "v4l2 device register failed (err=%d)\n", ret); return ret; } - /* allocate ent_devs */ vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, sizeof(*vimc->ent_devs), GFP_KERNEL); @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) err_mdev_unregister: media_device_unregister(&vimc->mdev); - media_device_cleanup(&vimc->mdev); err_rm_subdevs: - vimc_rm_subdevs(vimc); + vimc_unregister_subdevs(vimc); + vimc_release_subdevs(vimc); kfree(vimc->ent_devs); err_v4l2_unregister: v4l2_device_unregister(&vimc->v4l2_dev); @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) static void vimc_unregister(struct vimc_device *vimc) { + vimc_unregister_subdevs(vimc); media_device_unregister(&vimc->mdev); - media_device_cleanup(&vimc->mdev); v4l2_device_unregister(&vimc->v4l2_dev); - kfree(vimc->ent_devs); - kfree(vimc); } static int vimc_probe(struct platform_device *pdev) @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) kfree(vimc); return ret; } + /* + * the release cb is set only after successful registration. + * if the registration fails, we release directly from probe + */ + vimc->v4l2_dev.release = vimc_v4l2_dev_release; platform_set_drvdata(pdev, vimc); return 0; } @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) dev_dbg(&pdev->dev, "remove"); - vimc_rm_subdevs(vimc); vimc_unregister(vimc); + v4l2_device_put(&vimc->v4l2_dev); return 0; } diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c index 34b98b235880..baf6bf9f65b5 100644 --- a/drivers/media/platform/vimc/vimc-debayer.c +++ b/drivers/media/platform/vimc/vimc-debayer.c @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { .s_ctrl = vimc_deb_s_ctrl, }; -static void vimc_deb_release(struct v4l2_subdev *sd) +void vimc_deb_release(struct vimc_ent_device *ved) { struct vimc_deb_device *vdeb = - container_of(sd, struct vimc_deb_device, sd); + container_of(ved, struct vimc_deb_device, ved); v4l2_ctrl_handler_free(&vdeb->hdl); media_entity_cleanup(vdeb->ved.ent); kfree(vdeb); } -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { - .release = vimc_deb_release, -}; - -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) -{ - struct vimc_deb_device *vdeb; - - vdeb = container_of(ved, struct vimc_deb_device, ved); - v4l2_device_unregister_subdev(&vdeb->sd); -} - static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, .id = VIMC_CID_VIMC_CLASS, @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, vcfg_name, MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, - vdeb->pads, - &vimc_deb_int_ops, &vimc_deb_ops); + vdeb->pads, &vimc_deb_ops); if (ret) goto err_free_hdl; diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c index cb2dc24c3e59..7521439747c5 100644 --- a/drivers/media/platform/vimc/vimc-scaler.c +++ b/drivers/media/platform/vimc/vimc-scaler.c @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, return vsca->src_frame; }; -static void vimc_sca_release(struct v4l2_subdev *sd) +void vimc_sca_release(struct vimc_ent_device *ved) { struct vimc_sca_device *vsca = - container_of(sd, struct vimc_sca_device, sd); + container_of(ved, struct vimc_sca_device, ved); media_entity_cleanup(vsca->ved.ent); kfree(vsca); } -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { - .release = vimc_sca_release, -}; - -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) -{ - struct vimc_sca_device *vsca; - - vsca = container_of(ved, struct vimc_sca_device, ved); - v4l2_device_unregister_subdev(&vsca->sd); -} - struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, const char *vcfg_name) { @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, vcfg_name, MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, - vsca->pads, - &vimc_sca_int_ops, &vimc_sca_ops); + vsca->pads, &vimc_sca_ops); if (ret) { kfree(vsca); return NULL; diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c index 9788fe291193..92daee58209e 100644 --- a/drivers/media/platform/vimc/vimc-sensor.c +++ b/drivers/media/platform/vimc/vimc-sensor.c @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { .s_ctrl = vimc_sen_s_ctrl, }; -static void vimc_sen_release(struct v4l2_subdev *sd) +void vimc_sen_release(struct vimc_ent_device *ved) { struct vimc_sen_device *vsen = - container_of(sd, struct vimc_sen_device, sd); + container_of(ved, struct vimc_sen_device, ved); v4l2_ctrl_handler_free(&vsen->hdl); tpg_free(&vsen->tpg); @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) kfree(vsen); } -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { - .release = vimc_sen_release, -}; - -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) -{ - struct vimc_sen_device *vsen; - - vsen = container_of(ved, struct vimc_sen_device, ved); - v4l2_device_unregister_subdev(&vsen->sd); -} - /* Image Processing Controls */ static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, vcfg_name, MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, - &vimc_sen_int_ops, &vimc_sen_ops); + &vimc_sen_ops); if (ret) goto err_free_tpg; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-22 16:01 ` [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld @ 2020-01-22 16:39 ` Helen Koike 2020-01-23 11:36 ` Dafna Hirschfeld 0 siblings, 1 reply; 10+ messages in thread From: Helen Koike @ 2020-01-22 16:39 UTC (permalink / raw) To: Dafna Hirschfeld, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3 Hi Dafna, Thanks for the patch. On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: > A use-after-free bug occures when unbinding the device while it streams. > The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed > when calling the sensor's 'rm' callback but the freed pointer is > later accessed in the function 'vimc_streamer_pipeline_terminate'. > To fix this bug, move the release callback of the vimc entities > and vimc_device to the release callback of v4l2_device. > The .rm callback of vimc_ent_config is replaced by two callbacks: > > .unregister - this is called upon removing the device and > it unregisters the entity. This is an optional callback since > subdevices don't need to implement it because they are already > unregistered in v4l2_device_unregister. > > .release - this is called from the release callback of v4l2_device > and it frees the entity. > > This ensures that the entities will be released when the last fh > of any of the devices is closed. > > The commands that cause the crash and the KASAN report: > > media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' > media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 > v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 > v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & > sleep 1 > echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind > > [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 > [ 188.421800] > [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 > [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 > [ 188.425938] Call Trace: > [ 188.426610] dump_stack+0x75/0xa0 > [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.429057] print_address_description.constprop.6+0x16/0x220 > [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.433455] __kasan_report.cold.9+0x1a/0x40 > [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.436010] kasan_report+0xe/0x20 > [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] > [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] > [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] > [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] > [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] > [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] > [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] > [ 188.446179] vimc_remove+0x19/0x70 [vimc] > [ 188.447301] platform_drv_remove+0x2f/0x50 > [ 188.448468] device_release_driver_internal+0x133/0x260 > [ 188.449814] unbind_store+0x121/0x150 > [ 188.450726] kernfs_fop_write+0x142/0x230 > [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 > [ 188.452826] vfs_write+0xdc/0x230 > [ 188.453760] ksys_write+0xaf/0x140 > [ 188.454702] ? __ia32_sys_read+0x40/0x40 > [ 188.455773] ? __do_page_fault+0x473/0x620 > [ 188.456780] do_syscall_64+0x5e/0x1a0 > [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 188.459079] RIP: 0033:0x7f80f1f13504 > [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53 > [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 > [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 > [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 > [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 > [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 > [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 > [ 188.475107] > [ 188.475500] Allocated by task 473: > [ 188.476351] save_stack+0x19/0x80 > [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 > [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] > [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] > [ 188.480776] platform_drv_probe+0x46/0xa0 > [ 188.481829] really_probe+0x16c/0x520 > [ 188.482732] driver_probe_device+0x114/0x170 > [ 188.483783] device_driver_attach+0x85/0x90 > [ 188.484800] __driver_attach+0xa8/0x190 > [ 188.485734] bus_for_each_dev+0xe4/0x140 > [ 188.486702] bus_add_driver+0x223/0x2d0 > [ 188.487715] driver_register+0xca/0x140 > [ 188.488767] 0xffffffffc037003d > [ 188.489635] do_one_initcall+0x86/0x28f > [ 188.490702] do_init_module+0xf8/0x340 > [ 188.491773] load_module+0x3766/0x3a10 > [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 > [ 188.494059] do_syscall_64+0x5e/0x1a0 > [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 188.496481] > [ 188.496893] Freed by task 185: > [ 188.497670] save_stack+0x19/0x80 > [ 188.498493] __kasan_slab_free+0x125/0x170 > [ 188.499486] kfree+0x8c/0x230 > [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] > [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] > [ 188.502976] device_release+0x3c/0xd0 > [ 188.503867] kobject_put+0xf4/0x240 > [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] > [ 188.508846] vimc_remove+0x19/0x70 [vimc] > [ 188.509792] platform_drv_remove+0x2f/0x50 > [ 188.510752] device_release_driver_internal+0x133/0x260 > [ 188.512006] unbind_store+0x121/0x150 > [ 188.512899] kernfs_fop_write+0x142/0x230 > [ 188.513874] vfs_write+0xdc/0x230 > [ 188.514698] ksys_write+0xaf/0x140 > [ 188.515523] do_syscall_64+0x5e/0x1a0 > [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 188.517710] > [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 > [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 > [ 188.520528] The buggy address is located 8 bytes inside of > [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) > [ 188.523015] The buggy address belongs to the page: > [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 > [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 > [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 > [ 188.530883] page dumped because: kasan: bad access detected > [ 188.532336] > [ 188.532720] Memory state around the buggy address: > [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [ 188.538996] ^ > [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/media/platform/vimc/vimc-capture.c | 12 ++--- > drivers/media/platform/vimc/vimc-common.c | 2 - > drivers/media/platform/vimc/vimc-common.h | 25 ++++----- > drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- > drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- > drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- > drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- > 7 files changed, 71 insertions(+), 85 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c > index 9a78bb7826a8..c5a645f98c66 100644 > --- a/drivers/media/platform/vimc/vimc-capture.c > +++ b/drivers/media/platform/vimc/vimc-capture.c > @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { > .link_validate = vimc_vdev_link_validate, > }; > > -static void vimc_cap_release(struct video_device *vdev) > +void vimc_cap_release(struct vimc_ent_device *ved) > { > struct vimc_cap_device *vcap = > - container_of(vdev, struct vimc_cap_device, vdev); > + container_of(ved, struct vimc_cap_device, ved); > > media_entity_cleanup(vcap->ved.ent); > kfree(vcap); > } > > -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) > +void vimc_cap_unregister(struct vimc_ent_device *ved) > { > - struct vimc_cap_device *vcap; > + struct vimc_cap_device *vcap = > + container_of(ved, struct vimc_cap_device, ved); > > - vcap = container_of(ved, struct vimc_cap_device, ved); > vb2_queue_release(&vcap->queue); > video_unregister_device(&vcap->vdev); > } > @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, > vdev = &vcap->vdev; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > vdev->entity.ops = &vimc_cap_mops; > - vdev->release = vimc_cap_release; > + vdev->release = video_device_release_empty; > vdev->fops = &vimc_cap_fops; > vdev->ioctl_ops = &vimc_cap_ioctl_ops; > vdev->lock = &vcap->lock; > diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c > index 16ce9f3b7c75..c95c17c048f2 100644 > --- a/drivers/media/platform/vimc/vimc-common.c > +++ b/drivers/media/platform/vimc/vimc-common.c > @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, > u32 function, > u16 num_pads, > struct media_pad *pads, > - const struct v4l2_subdev_internal_ops *sd_int_ops, > const struct v4l2_subdev_ops *sd_ops) > { > int ret; > @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, > > /* Initialize the subdev */ > v4l2_subdev_init(sd, sd_ops); > - sd->internal_ops = sd_int_ops; > sd->entity.function = function; > sd->entity.ops = &vimc_ent_sd_mops; > sd->owner = THIS_MODULE; > diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h > index 1b6ef7196f3c..616d5a6b0754 100644 > --- a/drivers/media/platform/vimc/vimc-common.h > +++ b/drivers/media/platform/vimc/vimc-common.h > @@ -125,16 +125,18 @@ struct vimc_device { > * @name entity name > * @ved pointer to vimc_ent_device (a node in the > * topology) > - * @add subdev add hook - initializes and registers > - * subdev called from vimc-core > - * @rm subdev rm hook - unregisters and frees > - * subdev called from vimc-core > + * @add initializes and registers > + * vim entity - called from vimc-core > + * @unregister unregisters vimc entity - called from vimc-core > + * @release releases vimc entity - called from the v4l2_dev > + * release callback > */ Did you test the suggestion I gave in last patch? That instead of adding another callback here, to just remove the release functions of the subdevices, and add a check if the release cb isn't null just before calling it. With this, the release functions of the video nodes would be called first. I followed the discussion regarding fhs in media node and video, not that I'm against your changes, but I would like to understand a bit better why the simple fix suggested isn't enough. Thanks Helen > struct vimc_ent_config { > const char *name; > struct vimc_ent_device *(*add)(struct vimc_device *vimc, > const char *vcfg_name); > - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); > + void (*unregister)(struct vimc_ent_device *ved); > + void (*release)(struct vimc_ent_device *ved); > }; > > /** > @@ -145,22 +147,23 @@ struct vimc_ent_config { > */ > bool vimc_is_source(struct media_entity *ent); > > -/* prototypes for vimc_ent_config add and rm hooks */ > +/* prototypes for vimc_ent_config hooks */ > struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, > const char *vcfg_name); > -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); > +void vimc_cap_unregister(struct vimc_ent_device *ved); > +void vimc_cap_release(struct vimc_ent_device *ved); > > struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, > const char *vcfg_name); > -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); > +void vimc_deb_release(struct vimc_ent_device *ved); > > struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, > const char *vcfg_name); > -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); > +void vimc_sca_release(struct vimc_ent_device *ved); > > struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > const char *vcfg_name); > -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); > +void vimc_sen_release(struct vimc_ent_device *ved); > > /** > * vimc_pix_map_by_index - get vimc_pix_map struct by its index > @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); > * @num_pads: number of pads to initialize > * @pads: the array of pads of the entity, the caller should set the > flags of the pads > - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops > * @sd_ops: pointer to &struct v4l2_subdev_ops. > * > * Helper function initialize and register the struct vimc_ent_device and struct > @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, > u32 function, > u16 num_pads, > struct media_pad *pads, > - const struct v4l2_subdev_internal_ops *sd_int_ops, > const struct v4l2_subdev_ops *sd_ops); > > /** > diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c > index 1c55e0382f09..306f9c293628 100644 > --- a/drivers/media/platform/vimc/vimc-core.c > +++ b/drivers/media/platform/vimc/vimc-core.c > @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { > { > .name = "Sensor A", > .add = vimc_sen_add, > - .rm = vimc_sen_rm, > + .release = vimc_sen_release, > }, > { > .name = "Sensor B", > .add = vimc_sen_add, > - .rm = vimc_sen_rm, > + .release = vimc_sen_release, > }, > { > .name = "Debayer A", > .add = vimc_deb_add, > - .rm = vimc_deb_rm, > + .release = vimc_deb_release, > }, > { > .name = "Debayer B", > .add = vimc_deb_add, > - .rm = vimc_deb_rm, > + .release = vimc_deb_release, > }, > { > .name = "Raw Capture 0", > .add = vimc_cap_add, > - .rm = vimc_cap_rm, > + .unregister = vimc_cap_unregister, > + .release = vimc_cap_release, > }, > { > .name = "Raw Capture 1", > .add = vimc_cap_add, > - .rm = vimc_cap_rm, > + .unregister = vimc_cap_unregister, > + .release = vimc_cap_release, > }, > { > /* TODO: change this to vimc-input when it is implemented */ > .name = "RGB/YUV Input", > .add = vimc_sen_add, > - .rm = vimc_sen_rm, > + .release = vimc_sen_release, > }, > { > .name = "Scaler", > .add = vimc_sca_add, > - .rm = vimc_sca_rm, > + .release = vimc_sca_release, > }, > { > .name = "RGB/YUV Capture", > .add = vimc_cap_add, > - .rm = vimc_cap_rm, > + .unregister = vimc_cap_unregister, > + .release = vimc_cap_release, > }, > }; > > @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > return 0; > } > > -static void vimc_rm_subdevs(struct vimc_device *vimc) > +static void vimc_release_subdevs(struct vimc_device *vimc) > { > unsigned int i; > > for (i = 0; i < vimc->pipe_cfg->num_ents; i++) > if (vimc->ent_devs[i]) > - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); > + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); > +} > + > +static void vimc_unregister_subdevs(struct vimc_device *vimc) > +{ > + unsigned int i; > + > + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) > + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) > + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); > +} > + > +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > +{ > + struct vimc_device *vimc = > + container_of(v4l2_dev, struct vimc_device, v4l2_dev); > + > + vimc_release_subdevs(vimc); > + media_device_cleanup(&vimc->mdev); > + kfree(vimc->ent_devs); > + kfree(vimc); > } > > static int vimc_register_devices(struct vimc_device *vimc) > @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) > "v4l2 device register failed (err=%d)\n", ret); > return ret; > } > - > /* allocate ent_devs */ > vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, > sizeof(*vimc->ent_devs), GFP_KERNEL); > @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) > > err_mdev_unregister: > media_device_unregister(&vimc->mdev); > - media_device_cleanup(&vimc->mdev); > err_rm_subdevs: > - vimc_rm_subdevs(vimc); > + vimc_unregister_subdevs(vimc); > + vimc_release_subdevs(vimc); > kfree(vimc->ent_devs); > err_v4l2_unregister: > v4l2_device_unregister(&vimc->v4l2_dev); > @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) > > static void vimc_unregister(struct vimc_device *vimc) > { > + vimc_unregister_subdevs(vimc); > media_device_unregister(&vimc->mdev); > - media_device_cleanup(&vimc->mdev); > v4l2_device_unregister(&vimc->v4l2_dev); > - kfree(vimc->ent_devs); > - kfree(vimc); > } > > static int vimc_probe(struct platform_device *pdev) > @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) > kfree(vimc); > return ret; > } > + /* > + * the release cb is set only after successful registration. > + * if the registration fails, we release directly from probe > + */ > > + vimc->v4l2_dev.release = vimc_v4l2_dev_release; > platform_set_drvdata(pdev, vimc); > return 0; > } > @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) > > dev_dbg(&pdev->dev, "remove"); > > - vimc_rm_subdevs(vimc); > vimc_unregister(vimc); > + v4l2_device_put(&vimc->v4l2_dev); > > return 0; > } > diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c > index 34b98b235880..baf6bf9f65b5 100644 > --- a/drivers/media/platform/vimc/vimc-debayer.c > +++ b/drivers/media/platform/vimc/vimc-debayer.c > @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { > .s_ctrl = vimc_deb_s_ctrl, > }; > > -static void vimc_deb_release(struct v4l2_subdev *sd) > +void vimc_deb_release(struct vimc_ent_device *ved) > { > struct vimc_deb_device *vdeb = > - container_of(sd, struct vimc_deb_device, sd); > + container_of(ved, struct vimc_deb_device, ved); > > v4l2_ctrl_handler_free(&vdeb->hdl); > media_entity_cleanup(vdeb->ved.ent); > kfree(vdeb); > } > > -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { > - .release = vimc_deb_release, > -}; > - > -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) > -{ > - struct vimc_deb_device *vdeb; > - > - vdeb = container_of(ved, struct vimc_deb_device, ved); > - v4l2_device_unregister_subdev(&vdeb->sd); > -} > - > static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { > .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, > .id = VIMC_CID_VIMC_CLASS, > @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, > ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, > vcfg_name, > MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, > - vdeb->pads, > - &vimc_deb_int_ops, &vimc_deb_ops); > + vdeb->pads, &vimc_deb_ops); > if (ret) > goto err_free_hdl; > > diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c > index cb2dc24c3e59..7521439747c5 100644 > --- a/drivers/media/platform/vimc/vimc-scaler.c > +++ b/drivers/media/platform/vimc/vimc-scaler.c > @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, > return vsca->src_frame; > }; > > -static void vimc_sca_release(struct v4l2_subdev *sd) > +void vimc_sca_release(struct vimc_ent_device *ved) > { > struct vimc_sca_device *vsca = > - container_of(sd, struct vimc_sca_device, sd); > + container_of(ved, struct vimc_sca_device, ved); > > media_entity_cleanup(vsca->ved.ent); > kfree(vsca); > } > > -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { > - .release = vimc_sca_release, > -}; > - > -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) > -{ > - struct vimc_sca_device *vsca; > - > - vsca = container_of(ved, struct vimc_sca_device, ved); > - v4l2_device_unregister_subdev(&vsca->sd); > -} > - > struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, > const char *vcfg_name) > { > @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, > ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, > vcfg_name, > MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, > - vsca->pads, > - &vimc_sca_int_ops, &vimc_sca_ops); > + vsca->pads, &vimc_sca_ops); > if (ret) { > kfree(vsca); > return NULL; > diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c > index 9788fe291193..92daee58209e 100644 > --- a/drivers/media/platform/vimc/vimc-sensor.c > +++ b/drivers/media/platform/vimc/vimc-sensor.c > @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { > .s_ctrl = vimc_sen_s_ctrl, > }; > > -static void vimc_sen_release(struct v4l2_subdev *sd) > +void vimc_sen_release(struct vimc_ent_device *ved) > { > struct vimc_sen_device *vsen = > - container_of(sd, struct vimc_sen_device, sd); > + container_of(ved, struct vimc_sen_device, ved); > > v4l2_ctrl_handler_free(&vsen->hdl); > tpg_free(&vsen->tpg); > @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) > kfree(vsen); > } > > -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { > - .release = vimc_sen_release, > -}; > - > -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) > -{ > - struct vimc_sen_device *vsen; > - > - vsen = container_of(ved, struct vimc_sen_device, ved); > - v4l2_device_unregister_subdev(&vsen->sd); > -} > - > /* Image Processing Controls */ > static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { > .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, > @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, > vcfg_name, > MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, > - &vimc_sen_int_ops, &vimc_sen_ops); > + &vimc_sen_ops); > if (ret) > goto err_free_tpg; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-22 16:39 ` Helen Koike @ 2020-01-23 11:36 ` Dafna Hirschfeld 2020-01-24 11:31 ` Hans Verkuil 0 siblings, 1 reply; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-23 11:36 UTC (permalink / raw) To: Helen Koike, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3 Hi On 22.01.20 17:39, Helen Koike wrote: > Hi Dafna, > > Thanks for the patch. > > On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: >> A use-after-free bug occures when unbinding the device while it streams. >> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed >> when calling the sensor's 'rm' callback but the freed pointer is >> later accessed in the function 'vimc_streamer_pipeline_terminate'. >> To fix this bug, move the release callback of the vimc entities >> and vimc_device to the release callback of v4l2_device. >> The .rm callback of vimc_ent_config is replaced by two callbacks: >> >> .unregister - this is called upon removing the device and >> it unregisters the entity. This is an optional callback since >> subdevices don't need to implement it because they are already >> unregistered in v4l2_device_unregister. >> >> .release - this is called from the release callback of v4l2_device >> and it frees the entity. >> >> This ensures that the entities will be released when the last fh >> of any of the devices is closed. >> >> The commands that cause the crash and the KASAN report: >> >> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' >> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' >> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 >> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & >> sleep 1 >> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind >> >> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 >> [ 188.421800] >> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 >> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 >> [ 188.425938] Call Trace: >> [ 188.426610] dump_stack+0x75/0xa0 >> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.429057] print_address_description.constprop.6+0x16/0x220 >> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.433455] __kasan_report.cold.9+0x1a/0x40 >> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.436010] kasan_report+0xe/0x20 >> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] >> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] >> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] >> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] >> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] >> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] >> [ 188.446179] vimc_remove+0x19/0x70 [vimc] >> [ 188.447301] platform_drv_remove+0x2f/0x50 >> [ 188.448468] device_release_driver_internal+0x133/0x260 >> [ 188.449814] unbind_store+0x121/0x150 >> [ 188.450726] kernfs_fop_write+0x142/0x230 >> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 >> [ 188.452826] vfs_write+0xdc/0x230 >> [ 188.453760] ksys_write+0xaf/0x140 >> [ 188.454702] ? __ia32_sys_read+0x40/0x40 >> [ 188.455773] ? __do_page_fault+0x473/0x620 >> [ 188.456780] do_syscall_64+0x5e/0x1a0 >> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> [ 188.459079] RIP: 0033:0x7f80f1f13504 >> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53 >> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 >> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 >> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 >> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 >> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 >> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 >> [ 188.475107] >> [ 188.475500] Allocated by task 473: >> [ 188.476351] save_stack+0x19/0x80 >> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 >> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] >> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] >> [ 188.480776] platform_drv_probe+0x46/0xa0 >> [ 188.481829] really_probe+0x16c/0x520 >> [ 188.482732] driver_probe_device+0x114/0x170 >> [ 188.483783] device_driver_attach+0x85/0x90 >> [ 188.484800] __driver_attach+0xa8/0x190 >> [ 188.485734] bus_for_each_dev+0xe4/0x140 >> [ 188.486702] bus_add_driver+0x223/0x2d0 >> [ 188.487715] driver_register+0xca/0x140 >> [ 188.488767] 0xffffffffc037003d >> [ 188.489635] do_one_initcall+0x86/0x28f >> [ 188.490702] do_init_module+0xf8/0x340 >> [ 188.491773] load_module+0x3766/0x3a10 >> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 >> [ 188.494059] do_syscall_64+0x5e/0x1a0 >> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> [ 188.496481] >> [ 188.496893] Freed by task 185: >> [ 188.497670] save_stack+0x19/0x80 >> [ 188.498493] __kasan_slab_free+0x125/0x170 >> [ 188.499486] kfree+0x8c/0x230 >> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] >> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] >> [ 188.502976] device_release+0x3c/0xd0 >> [ 188.503867] kobject_put+0xf4/0x240 >> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] >> [ 188.508846] vimc_remove+0x19/0x70 [vimc] >> [ 188.509792] platform_drv_remove+0x2f/0x50 >> [ 188.510752] device_release_driver_internal+0x133/0x260 >> [ 188.512006] unbind_store+0x121/0x150 >> [ 188.512899] kernfs_fop_write+0x142/0x230 >> [ 188.513874] vfs_write+0xdc/0x230 >> [ 188.514698] ksys_write+0xaf/0x140 >> [ 188.515523] do_syscall_64+0x5e/0x1a0 >> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> [ 188.517710] >> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 >> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 >> [ 188.520528] The buggy address is located 8 bytes inside of >> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) >> [ 188.523015] The buggy address belongs to the page: >> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 >> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 >> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 >> [ 188.530883] page dumped because: kasan: bad access detected >> [ 188.532336] >> [ 188.532720] Memory state around the buggy address: >> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> [ 188.538996] ^ >> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/media/platform/vimc/vimc-capture.c | 12 ++--- >> drivers/media/platform/vimc/vimc-common.c | 2 - >> drivers/media/platform/vimc/vimc-common.h | 25 ++++----- >> drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- >> drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- >> drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- >> drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- >> 7 files changed, 71 insertions(+), 85 deletions(-) >> >> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >> index 9a78bb7826a8..c5a645f98c66 100644 >> --- a/drivers/media/platform/vimc/vimc-capture.c >> +++ b/drivers/media/platform/vimc/vimc-capture.c >> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { >> .link_validate = vimc_vdev_link_validate, >> }; >> >> -static void vimc_cap_release(struct video_device *vdev) >> +void vimc_cap_release(struct vimc_ent_device *ved) >> { >> struct vimc_cap_device *vcap = >> - container_of(vdev, struct vimc_cap_device, vdev); >> + container_of(ved, struct vimc_cap_device, ved); >> >> media_entity_cleanup(vcap->ved.ent); >> kfree(vcap); >> } >> >> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >> +void vimc_cap_unregister(struct vimc_ent_device *ved) >> { >> - struct vimc_cap_device *vcap; >> + struct vimc_cap_device *vcap = >> + container_of(ved, struct vimc_cap_device, ved); >> >> - vcap = container_of(ved, struct vimc_cap_device, ved); >> vb2_queue_release(&vcap->queue); >> video_unregister_device(&vcap->vdev); >> } >> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >> vdev = &vcap->vdev; >> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >> vdev->entity.ops = &vimc_cap_mops; >> - vdev->release = vimc_cap_release; >> + vdev->release = video_device_release_empty; >> vdev->fops = &vimc_cap_fops; >> vdev->ioctl_ops = &vimc_cap_ioctl_ops; >> vdev->lock = &vcap->lock; >> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >> index 16ce9f3b7c75..c95c17c048f2 100644 >> --- a/drivers/media/platform/vimc/vimc-common.c >> +++ b/drivers/media/platform/vimc/vimc-common.c >> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >> u32 function, >> u16 num_pads, >> struct media_pad *pads, >> - const struct v4l2_subdev_internal_ops *sd_int_ops, >> const struct v4l2_subdev_ops *sd_ops) >> { >> int ret; >> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >> >> /* Initialize the subdev */ >> v4l2_subdev_init(sd, sd_ops); >> - sd->internal_ops = sd_int_ops; >> sd->entity.function = function; >> sd->entity.ops = &vimc_ent_sd_mops; >> sd->owner = THIS_MODULE; >> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >> index 1b6ef7196f3c..616d5a6b0754 100644 >> --- a/drivers/media/platform/vimc/vimc-common.h >> +++ b/drivers/media/platform/vimc/vimc-common.h >> @@ -125,16 +125,18 @@ struct vimc_device { >> * @name entity name >> * @ved pointer to vimc_ent_device (a node in the >> * topology) >> - * @add subdev add hook - initializes and registers >> - * subdev called from vimc-core >> - * @rm subdev rm hook - unregisters and frees >> - * subdev called from vimc-core >> + * @add initializes and registers >> + * vim entity - called from vimc-core >> + * @unregister unregisters vimc entity - called from vimc-core >> + * @release releases vimc entity - called from the v4l2_dev >> + * release callback >> */ > > Did you test the suggestion I gave in last patch? > That instead of adding another callback here, to just remove the release functions of the subdevices, > and add a check if the release cb isn't null just before calling it. > With this, the release functions of the video nodes would be called first. > I just tested you suggestion, here is the patch: https://gitlab.collabora.com/dafna/linux/commit/f59bc7c6643ce8f88aa33eb5e9f413a1aca99589 I get a kasan report: [ 37.736909] BUG: KASAN: use-after-free in v4l2_device_unregister+0xa7/0xf0 [videodev] [ 37.738110] Read of size 4 at addr ffff8881eb3f00bc by task bash/227 ... What happen is that the function v4l2_device_unregister loops over the subdevices and unregister them: /* Unregister subdevs */ list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) { v4l2_device_unregister_subdev(sd); if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) v4l2_i2c_subdev_unregister(sd); else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) v4l2_spi_subdev_unregister(sd); } when unregistering the first subdevice it also immediately calls the release callback because no one holds a fh for /dev/v4l2-subdev* , but the release cb releases the memory where the 'sd' in the above loop resides, then when referencing it in sd->flags it is after it is already freed. Dafna > I followed the discussion regarding fhs in media node and video, not that I'm against your changes, > but I would like to understand a bit better why the simple fix suggested isn't enough. > > Thanks > Helen > >> struct vimc_ent_config { >> const char *name; >> struct vimc_ent_device *(*add)(struct vimc_device *vimc, >> const char *vcfg_name); >> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); >> + void (*unregister)(struct vimc_ent_device *ved); >> + void (*release)(struct vimc_ent_device *ved); >> }; >> >> /** >> @@ -145,22 +147,23 @@ struct vimc_ent_config { >> */ >> bool vimc_is_source(struct media_entity *ent); >> >> -/* prototypes for vimc_ent_config add and rm hooks */ >> +/* prototypes for vimc_ent_config hooks */ >> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >> const char *vcfg_name); >> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >> +void vimc_cap_unregister(struct vimc_ent_device *ved); >> +void vimc_cap_release(struct vimc_ent_device *ved); >> >> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >> const char *vcfg_name); >> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >> +void vimc_deb_release(struct vimc_ent_device *ved); >> >> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >> const char *vcfg_name); >> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >> +void vimc_sca_release(struct vimc_ent_device *ved); >> >> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >> const char *vcfg_name); >> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >> +void vimc_sen_release(struct vimc_ent_device *ved); >> >> /** >> * vimc_pix_map_by_index - get vimc_pix_map struct by its index >> @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); >> * @num_pads: number of pads to initialize >> * @pads: the array of pads of the entity, the caller should set the >> flags of the pads >> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops >> * @sd_ops: pointer to &struct v4l2_subdev_ops. >> * >> * Helper function initialize and register the struct vimc_ent_device and struct >> @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >> u32 function, >> u16 num_pads, >> struct media_pad *pads, >> - const struct v4l2_subdev_internal_ops *sd_int_ops, >> const struct v4l2_subdev_ops *sd_ops); >> >> /** >> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c >> index 1c55e0382f09..306f9c293628 100644 >> --- a/drivers/media/platform/vimc/vimc-core.c >> +++ b/drivers/media/platform/vimc/vimc-core.c >> @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { >> { >> .name = "Sensor A", >> .add = vimc_sen_add, >> - .rm = vimc_sen_rm, >> + .release = vimc_sen_release, >> }, >> { >> .name = "Sensor B", >> .add = vimc_sen_add, >> - .rm = vimc_sen_rm, >> + .release = vimc_sen_release, >> }, >> { >> .name = "Debayer A", >> .add = vimc_deb_add, >> - .rm = vimc_deb_rm, >> + .release = vimc_deb_release, >> }, >> { >> .name = "Debayer B", >> .add = vimc_deb_add, >> - .rm = vimc_deb_rm, >> + .release = vimc_deb_release, >> }, >> { >> .name = "Raw Capture 0", >> .add = vimc_cap_add, >> - .rm = vimc_cap_rm, >> + .unregister = vimc_cap_unregister, >> + .release = vimc_cap_release, >> }, >> { >> .name = "Raw Capture 1", >> .add = vimc_cap_add, >> - .rm = vimc_cap_rm, >> + .unregister = vimc_cap_unregister, >> + .release = vimc_cap_release, >> }, >> { >> /* TODO: change this to vimc-input when it is implemented */ >> .name = "RGB/YUV Input", >> .add = vimc_sen_add, >> - .rm = vimc_sen_rm, >> + .release = vimc_sen_release, >> }, >> { >> .name = "Scaler", >> .add = vimc_sca_add, >> - .rm = vimc_sca_rm, >> + .release = vimc_sca_release, >> }, >> { >> .name = "RGB/YUV Capture", >> .add = vimc_cap_add, >> - .rm = vimc_cap_rm, >> + .unregister = vimc_cap_unregister, >> + .release = vimc_cap_release, >> }, >> }; >> >> @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >> return 0; >> } >> >> -static void vimc_rm_subdevs(struct vimc_device *vimc) >> +static void vimc_release_subdevs(struct vimc_device *vimc) >> { >> unsigned int i; >> >> for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >> if (vimc->ent_devs[i]) >> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); >> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); >> +} >> + >> +static void vimc_unregister_subdevs(struct vimc_device *vimc) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) >> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); >> +} >> + >> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >> +{ >> + struct vimc_device *vimc = >> + container_of(v4l2_dev, struct vimc_device, v4l2_dev); >> + >> + vimc_release_subdevs(vimc); >> + media_device_cleanup(&vimc->mdev); >> + kfree(vimc->ent_devs); >> + kfree(vimc); >> } >> >> static int vimc_register_devices(struct vimc_device *vimc) >> @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) >> "v4l2 device register failed (err=%d)\n", ret); >> return ret; >> } >> - >> /* allocate ent_devs */ >> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, >> sizeof(*vimc->ent_devs), GFP_KERNEL); >> @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >> >> err_mdev_unregister: >> media_device_unregister(&vimc->mdev); >> - media_device_cleanup(&vimc->mdev); >> err_rm_subdevs: >> - vimc_rm_subdevs(vimc); >> + vimc_unregister_subdevs(vimc); >> + vimc_release_subdevs(vimc); >> kfree(vimc->ent_devs); >> err_v4l2_unregister: >> v4l2_device_unregister(&vimc->v4l2_dev); >> @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >> >> static void vimc_unregister(struct vimc_device *vimc) >> { >> + vimc_unregister_subdevs(vimc); >> media_device_unregister(&vimc->mdev); >> - media_device_cleanup(&vimc->mdev); >> v4l2_device_unregister(&vimc->v4l2_dev); >> - kfree(vimc->ent_devs); >> - kfree(vimc); >> } >> >> static int vimc_probe(struct platform_device *pdev) >> @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) >> kfree(vimc); >> return ret; >> } >> + /* >> + * the release cb is set only after successful registration. >> + * if the registration fails, we release directly from probe >> + */ >> >> + vimc->v4l2_dev.release = vimc_v4l2_dev_release; >> platform_set_drvdata(pdev, vimc); >> return 0; >> } >> @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) >> >> dev_dbg(&pdev->dev, "remove"); >> >> - vimc_rm_subdevs(vimc); >> vimc_unregister(vimc); >> + v4l2_device_put(&vimc->v4l2_dev); >> >> return 0; >> } >> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >> index 34b98b235880..baf6bf9f65b5 100644 >> --- a/drivers/media/platform/vimc/vimc-debayer.c >> +++ b/drivers/media/platform/vimc/vimc-debayer.c >> @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { >> .s_ctrl = vimc_deb_s_ctrl, >> }; >> >> -static void vimc_deb_release(struct v4l2_subdev *sd) >> +void vimc_deb_release(struct vimc_ent_device *ved) >> { >> struct vimc_deb_device *vdeb = >> - container_of(sd, struct vimc_deb_device, sd); >> + container_of(ved, struct vimc_deb_device, ved); >> >> v4l2_ctrl_handler_free(&vdeb->hdl); >> media_entity_cleanup(vdeb->ved.ent); >> kfree(vdeb); >> } >> >> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { >> - .release = vimc_deb_release, >> -}; >> - >> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >> -{ >> - struct vimc_deb_device *vdeb; >> - >> - vdeb = container_of(ved, struct vimc_deb_device, ved); >> - v4l2_device_unregister_subdev(&vdeb->sd); >> -} >> - >> static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { >> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >> .id = VIMC_CID_VIMC_CLASS, >> @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, >> vcfg_name, >> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, >> - vdeb->pads, >> - &vimc_deb_int_ops, &vimc_deb_ops); >> + vdeb->pads, &vimc_deb_ops); >> if (ret) >> goto err_free_hdl; >> >> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >> index cb2dc24c3e59..7521439747c5 100644 >> --- a/drivers/media/platform/vimc/vimc-scaler.c >> +++ b/drivers/media/platform/vimc/vimc-scaler.c >> @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, >> return vsca->src_frame; >> }; >> >> -static void vimc_sca_release(struct v4l2_subdev *sd) >> +void vimc_sca_release(struct vimc_ent_device *ved) >> { >> struct vimc_sca_device *vsca = >> - container_of(sd, struct vimc_sca_device, sd); >> + container_of(ved, struct vimc_sca_device, ved); >> >> media_entity_cleanup(vsca->ved.ent); >> kfree(vsca); >> } >> >> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { >> - .release = vimc_sca_release, >> -}; >> - >> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >> -{ >> - struct vimc_sca_device *vsca; >> - >> - vsca = container_of(ved, struct vimc_sca_device, ved); >> - v4l2_device_unregister_subdev(&vsca->sd); >> -} >> - >> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >> const char *vcfg_name) >> { >> @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, >> vcfg_name, >> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, >> - vsca->pads, >> - &vimc_sca_int_ops, &vimc_sca_ops); >> + vsca->pads, &vimc_sca_ops); >> if (ret) { >> kfree(vsca); >> return NULL; >> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >> index 9788fe291193..92daee58209e 100644 >> --- a/drivers/media/platform/vimc/vimc-sensor.c >> +++ b/drivers/media/platform/vimc/vimc-sensor.c >> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { >> .s_ctrl = vimc_sen_s_ctrl, >> }; >> >> -static void vimc_sen_release(struct v4l2_subdev *sd) >> +void vimc_sen_release(struct vimc_ent_device *ved) >> { >> struct vimc_sen_device *vsen = >> - container_of(sd, struct vimc_sen_device, sd); >> + container_of(ved, struct vimc_sen_device, ved); >> >> v4l2_ctrl_handler_free(&vsen->hdl); >> tpg_free(&vsen->tpg); >> @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) >> kfree(vsen); >> } >> >> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { >> - .release = vimc_sen_release, >> -}; >> - >> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >> -{ >> - struct vimc_sen_device *vsen; >> - >> - vsen = container_of(ved, struct vimc_sen_device, ved); >> - v4l2_device_unregister_subdev(&vsen->sd); >> -} >> - >> /* Image Processing Controls */ >> static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { >> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >> @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, >> vcfg_name, >> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, >> - &vimc_sen_int_ops, &vimc_sen_ops); >> + &vimc_sen_ops); >> if (ret) >> goto err_free_tpg; >> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-23 11:36 ` Dafna Hirschfeld @ 2020-01-24 11:31 ` Hans Verkuil 2020-01-24 12:18 ` Dafna Hirschfeld 0 siblings, 1 reply; 10+ messages in thread From: Hans Verkuil @ 2020-01-24 11:31 UTC (permalink / raw) To: Dafna Hirschfeld, Helen Koike, linux-media Cc: ezequiel, skhan, kernel, dafna3 On 1/23/20 12:36 PM, Dafna Hirschfeld wrote: > Hi > > On 22.01.20 17:39, Helen Koike wrote: >> Hi Dafna, >> >> Thanks for the patch. >> >> On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: >>> A use-after-free bug occures when unbinding the device while it streams. >>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed >>> when calling the sensor's 'rm' callback but the freed pointer is >>> later accessed in the function 'vimc_streamer_pipeline_terminate'. >>> To fix this bug, move the release callback of the vimc entities >>> and vimc_device to the release callback of v4l2_device. >>> The .rm callback of vimc_ent_config is replaced by two callbacks: >>> >>> .unregister - this is called upon removing the device and >>> it unregisters the entity. This is an optional callback since >>> subdevices don't need to implement it because they are already >>> unregistered in v4l2_device_unregister. >>> >>> .release - this is called from the release callback of v4l2_device >>> and it frees the entity. >>> >>> This ensures that the entities will be released when the last fh >>> of any of the devices is closed. >>> >>> The commands that cause the crash and the KASAN report: >>> >>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' >>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' >>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 >>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & >>> sleep 1 >>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind >>> >>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 >>> [ 188.421800] >>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 >>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 >>> [ 188.425938] Call Trace: >>> [ 188.426610] dump_stack+0x75/0xa0 >>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.429057] print_address_description.constprop.6+0x16/0x220 >>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40 >>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.436010] kasan_report+0xe/0x20 >>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] >>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] >>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] >>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] >>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] >>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>> [ 188.446179] vimc_remove+0x19/0x70 [vimc] >>> [ 188.447301] platform_drv_remove+0x2f/0x50 >>> [ 188.448468] device_release_driver_internal+0x133/0x260 >>> [ 188.449814] unbind_store+0x121/0x150 >>> [ 188.450726] kernfs_fop_write+0x142/0x230 >>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 >>> [ 188.452826] vfs_write+0xdc/0x230 >>> [ 188.453760] ksys_write+0xaf/0x140 >>> [ 188.454702] ? __ia32_sys_read+0x40/0x40 >>> [ 188.455773] ? __do_page_fault+0x473/0x620 >>> [ 188.456780] do_syscall_64+0x5e/0x1a0 >>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> [ 188.459079] RIP: 0033:0x7f80f1f13504 >>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 >>> d4 55 48 89 f5 53 >>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 >>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 >>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 >>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 >>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 >>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 >>> [ 188.475107] >>> [ 188.475500] Allocated by task 473: >>> [ 188.476351] save_stack+0x19/0x80 >>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 >>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] >>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] >>> [ 188.480776] platform_drv_probe+0x46/0xa0 >>> [ 188.481829] really_probe+0x16c/0x520 >>> [ 188.482732] driver_probe_device+0x114/0x170 >>> [ 188.483783] device_driver_attach+0x85/0x90 >>> [ 188.484800] __driver_attach+0xa8/0x190 >>> [ 188.485734] bus_for_each_dev+0xe4/0x140 >>> [ 188.486702] bus_add_driver+0x223/0x2d0 >>> [ 188.487715] driver_register+0xca/0x140 >>> [ 188.488767] 0xffffffffc037003d >>> [ 188.489635] do_one_initcall+0x86/0x28f >>> [ 188.490702] do_init_module+0xf8/0x340 >>> [ 188.491773] load_module+0x3766/0x3a10 >>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 >>> [ 188.494059] do_syscall_64+0x5e/0x1a0 >>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> [ 188.496481] >>> [ 188.496893] Freed by task 185: >>> [ 188.497670] save_stack+0x19/0x80 >>> [ 188.498493] __kasan_slab_free+0x125/0x170 >>> [ 188.499486] kfree+0x8c/0x230 >>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] >>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] >>> [ 188.502976] device_release+0x3c/0xd0 >>> [ 188.503867] kobject_put+0xf4/0x240 >>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>> [ 188.508846] vimc_remove+0x19/0x70 [vimc] >>> [ 188.509792] platform_drv_remove+0x2f/0x50 >>> [ 188.510752] device_release_driver_internal+0x133/0x260 >>> [ 188.512006] unbind_store+0x121/0x150 >>> [ 188.512899] kernfs_fop_write+0x142/0x230 >>> [ 188.513874] vfs_write+0xdc/0x230 >>> [ 188.514698] ksys_write+0xaf/0x140 >>> [ 188.515523] do_syscall_64+0x5e/0x1a0 >>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> [ 188.517710] >>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 >>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 >>> [ 188.520528] The buggy address is located 8 bytes inside of >>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) >>> [ 188.523015] The buggy address belongs to the page: >>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 >>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 >>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 >>> [ 188.530883] page dumped because: kasan: bad access detected >>> [ 188.532336] >>> [ 188.532720] Memory state around the buggy address: >>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>> [ 188.538996] ^ >>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>> --- >>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--- >>> drivers/media/platform/vimc/vimc-common.c | 2 - >>> drivers/media/platform/vimc/vimc-common.h | 25 ++++----- >>> drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- >>> drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- >>> drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- >>> drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- >>> 7 files changed, 71 insertions(+), 85 deletions(-) >>> >>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>> index 9a78bb7826a8..c5a645f98c66 100644 >>> --- a/drivers/media/platform/vimc/vimc-capture.c >>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { >>> .link_validate = vimc_vdev_link_validate, >>> }; >>> -static void vimc_cap_release(struct video_device *vdev) >>> +void vimc_cap_release(struct vimc_ent_device *ved) >>> { >>> struct vimc_cap_device *vcap = >>> - container_of(vdev, struct vimc_cap_device, vdev); >>> + container_of(ved, struct vimc_cap_device, ved); >>> media_entity_cleanup(vcap->ved.ent); >>> kfree(vcap); >>> } >>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>> +void vimc_cap_unregister(struct vimc_ent_device *ved) >>> { >>> - struct vimc_cap_device *vcap; >>> + struct vimc_cap_device *vcap = >>> + container_of(ved, struct vimc_cap_device, ved); >>> - vcap = container_of(ved, struct vimc_cap_device, ved); >>> vb2_queue_release(&vcap->queue); >>> video_unregister_device(&vcap->vdev); >>> } >>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>> vdev = &vcap->vdev; >>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>> vdev->entity.ops = &vimc_cap_mops; >>> - vdev->release = vimc_cap_release; >>> + vdev->release = video_device_release_empty; >>> vdev->fops = &vimc_cap_fops; >>> vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>> vdev->lock = &vcap->lock; >>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >>> index 16ce9f3b7c75..c95c17c048f2 100644 >>> --- a/drivers/media/platform/vimc/vimc-common.c >>> +++ b/drivers/media/platform/vimc/vimc-common.c >>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>> u32 function, >>> u16 num_pads, >>> struct media_pad *pads, >>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>> const struct v4l2_subdev_ops *sd_ops) >>> { >>> int ret; >>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>> /* Initialize the subdev */ >>> v4l2_subdev_init(sd, sd_ops); >>> - sd->internal_ops = sd_int_ops; >>> sd->entity.function = function; >>> sd->entity.ops = &vimc_ent_sd_mops; >>> sd->owner = THIS_MODULE; >>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >>> index 1b6ef7196f3c..616d5a6b0754 100644 >>> --- a/drivers/media/platform/vimc/vimc-common.h >>> +++ b/drivers/media/platform/vimc/vimc-common.h >>> @@ -125,16 +125,18 @@ struct vimc_device { >>> * @name entity name >>> * @ved pointer to vimc_ent_device (a node in the >>> * topology) >>> - * @add subdev add hook - initializes and registers >>> - * subdev called from vimc-core >>> - * @rm subdev rm hook - unregisters and frees >>> - * subdev called from vimc-core >>> + * @add initializes and registers >>> + * vim entity - called from vimc-core >>> + * @unregister unregisters vimc entity - called from vimc-core >>> + * @release releases vimc entity - called from the v4l2_dev >>> + * release callback >>> */ >> >> Did you test the suggestion I gave in last patch? >> That instead of adding another callback here, to just remove the release functions of the subdevices, >> and add a check if the release cb isn't null just before calling it. >> With this, the release functions of the video nodes would be called first. >> > I just tested you suggestion, here is the patch: https://gitlab.collabora.com/dafna/linux/commit/f59bc7c6643ce8f88aa33eb5e9f413a1aca99589 > I get a kasan report: > > [ 37.736909] BUG: KASAN: use-after-free in v4l2_device_unregister+0xa7/0xf0 [videodev] > [ 37.738110] Read of size 4 at addr ffff8881eb3f00bc by task bash/227 > ... > > What happen is that the function v4l2_device_unregister loops over the subdevices and unregister them: > > /* Unregister subdevs */ > list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) { > v4l2_device_unregister_subdev(sd); > if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) > v4l2_i2c_subdev_unregister(sd); > else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) > v4l2_spi_subdev_unregister(sd); > } > > when unregistering the first subdevice it also immediately calls the release callback because no one holds a fh for > /dev/v4l2-subdev* , but the release cb releases the memory where the 'sd' in the above loop resides, then when > referencing it in sd->flags it is after it is already freed. Urgh. That's a bug in that function. I think the if...else if bit can be moved to v4l2_subdev_release(). I'm surprised that this hasn't been seen before. Regards, Hans > > Dafna > >> I followed the discussion regarding fhs in media node and video, not that I'm against your changes, >> but I would like to understand a bit better why the simple fix suggested isn't enough. >> >> Thanks >> Helen >> >>> struct vimc_ent_config { >>> const char *name; >>> struct vimc_ent_device *(*add)(struct vimc_device *vimc, >>> const char *vcfg_name); >>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); >>> + void (*unregister)(struct vimc_ent_device *ved); >>> + void (*release)(struct vimc_ent_device *ved); >>> }; >>> /** >>> @@ -145,22 +147,23 @@ struct vimc_ent_config { >>> */ >>> bool vimc_is_source(struct media_entity *ent); >>> -/* prototypes for vimc_ent_config add and rm hooks */ >>> +/* prototypes for vimc_ent_config hooks */ >>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>> const char *vcfg_name); >>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>> +void vimc_cap_unregister(struct vimc_ent_device *ved); >>> +void vimc_cap_release(struct vimc_ent_device *ved); >>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>> const char *vcfg_name); >>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>> +void vimc_deb_release(struct vimc_ent_device *ved); >>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>> const char *vcfg_name); >>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>> +void vimc_sca_release(struct vimc_ent_device *ved); >>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>> const char *vcfg_name); >>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>> +void vimc_sen_release(struct vimc_ent_device *ved); >>> /** >>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index >>> @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); >>> * @num_pads: number of pads to initialize >>> * @pads: the array of pads of the entity, the caller should set the >>> flags of the pads >>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops >>> * @sd_ops: pointer to &struct v4l2_subdev_ops. >>> * >>> * Helper function initialize and register the struct vimc_ent_device and struct >>> @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>> u32 function, >>> u16 num_pads, >>> struct media_pad *pads, >>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>> const struct v4l2_subdev_ops *sd_ops); >>> /** >>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c >>> index 1c55e0382f09..306f9c293628 100644 >>> --- a/drivers/media/platform/vimc/vimc-core.c >>> +++ b/drivers/media/platform/vimc/vimc-core.c >>> @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { >>> { >>> .name = "Sensor A", >>> .add = vimc_sen_add, >>> - .rm = vimc_sen_rm, >>> + .release = vimc_sen_release, >>> }, >>> { >>> .name = "Sensor B", >>> .add = vimc_sen_add, >>> - .rm = vimc_sen_rm, >>> + .release = vimc_sen_release, >>> }, >>> { >>> .name = "Debayer A", >>> .add = vimc_deb_add, >>> - .rm = vimc_deb_rm, >>> + .release = vimc_deb_release, >>> }, >>> { >>> .name = "Debayer B", >>> .add = vimc_deb_add, >>> - .rm = vimc_deb_rm, >>> + .release = vimc_deb_release, >>> }, >>> { >>> .name = "Raw Capture 0", >>> .add = vimc_cap_add, >>> - .rm = vimc_cap_rm, >>> + .unregister = vimc_cap_unregister, >>> + .release = vimc_cap_release, >>> }, >>> { >>> .name = "Raw Capture 1", >>> .add = vimc_cap_add, >>> - .rm = vimc_cap_rm, >>> + .unregister = vimc_cap_unregister, >>> + .release = vimc_cap_release, >>> }, >>> { >>> /* TODO: change this to vimc-input when it is implemented */ >>> .name = "RGB/YUV Input", >>> .add = vimc_sen_add, >>> - .rm = vimc_sen_rm, >>> + .release = vimc_sen_release, >>> }, >>> { >>> .name = "Scaler", >>> .add = vimc_sca_add, >>> - .rm = vimc_sca_rm, >>> + .release = vimc_sca_release, >>> }, >>> { >>> .name = "RGB/YUV Capture", >>> .add = vimc_cap_add, >>> - .rm = vimc_cap_rm, >>> + .unregister = vimc_cap_unregister, >>> + .release = vimc_cap_release, >>> }, >>> }; >>> @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>> return 0; >>> } >>> -static void vimc_rm_subdevs(struct vimc_device *vimc) >>> +static void vimc_release_subdevs(struct vimc_device *vimc) >>> { >>> unsigned int i; >>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>> if (vimc->ent_devs[i]) >>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); >>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); >>> +} >>> + >>> +static void vimc_unregister_subdevs(struct vimc_device *vimc) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) >>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); >>> +} >>> + >>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>> +{ >>> + struct vimc_device *vimc = >>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>> + >>> + vimc_release_subdevs(vimc); >>> + media_device_cleanup(&vimc->mdev); >>> + kfree(vimc->ent_devs); >>> + kfree(vimc); >>> } >>> static int vimc_register_devices(struct vimc_device *vimc) >>> @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) >>> "v4l2 device register failed (err=%d)\n", ret); >>> return ret; >>> } >>> - >>> /* allocate ent_devs */ >>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, >>> sizeof(*vimc->ent_devs), GFP_KERNEL); >>> @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>> err_mdev_unregister: >>> media_device_unregister(&vimc->mdev); >>> - media_device_cleanup(&vimc->mdev); >>> err_rm_subdevs: >>> - vimc_rm_subdevs(vimc); >>> + vimc_unregister_subdevs(vimc); >>> + vimc_release_subdevs(vimc); >>> kfree(vimc->ent_devs); >>> err_v4l2_unregister: >>> v4l2_device_unregister(&vimc->v4l2_dev); >>> @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>> static void vimc_unregister(struct vimc_device *vimc) >>> { >>> + vimc_unregister_subdevs(vimc); >>> media_device_unregister(&vimc->mdev); >>> - media_device_cleanup(&vimc->mdev); >>> v4l2_device_unregister(&vimc->v4l2_dev); >>> - kfree(vimc->ent_devs); >>> - kfree(vimc); >>> } >>> static int vimc_probe(struct platform_device *pdev) >>> @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) >>> kfree(vimc); >>> return ret; >>> } >>> + /* >>> + * the release cb is set only after successful registration. >>> + * if the registration fails, we release directly from probe >>> + */ >>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release; >>> platform_set_drvdata(pdev, vimc); >>> return 0; >>> } >>> @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) >>> dev_dbg(&pdev->dev, "remove"); >>> - vimc_rm_subdevs(vimc); >>> vimc_unregister(vimc); >>> + v4l2_device_put(&vimc->v4l2_dev); >>> return 0; >>> } >>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>> index 34b98b235880..baf6bf9f65b5 100644 >>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>> @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { >>> .s_ctrl = vimc_deb_s_ctrl, >>> }; >>> -static void vimc_deb_release(struct v4l2_subdev *sd) >>> +void vimc_deb_release(struct vimc_ent_device *ved) >>> { >>> struct vimc_deb_device *vdeb = >>> - container_of(sd, struct vimc_deb_device, sd); >>> + container_of(ved, struct vimc_deb_device, ved); >>> v4l2_ctrl_handler_free(&vdeb->hdl); >>> media_entity_cleanup(vdeb->ved.ent); >>> kfree(vdeb); >>> } >>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { >>> - .release = vimc_deb_release, >>> -}; >>> - >>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>> -{ >>> - struct vimc_deb_device *vdeb; >>> - >>> - vdeb = container_of(ved, struct vimc_deb_device, ved); >>> - v4l2_device_unregister_subdev(&vdeb->sd); >>> -} >>> - >>> static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { >>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>> .id = VIMC_CID_VIMC_CLASS, >>> @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, >>> vcfg_name, >>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, >>> - vdeb->pads, >>> - &vimc_deb_int_ops, &vimc_deb_ops); >>> + vdeb->pads, &vimc_deb_ops); >>> if (ret) >>> goto err_free_hdl; >>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >>> index cb2dc24c3e59..7521439747c5 100644 >>> --- a/drivers/media/platform/vimc/vimc-scaler.c >>> +++ b/drivers/media/platform/vimc/vimc-scaler.c >>> @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, >>> return vsca->src_frame; >>> }; >>> -static void vimc_sca_release(struct v4l2_subdev *sd) >>> +void vimc_sca_release(struct vimc_ent_device *ved) >>> { >>> struct vimc_sca_device *vsca = >>> - container_of(sd, struct vimc_sca_device, sd); >>> + container_of(ved, struct vimc_sca_device, ved); >>> media_entity_cleanup(vsca->ved.ent); >>> kfree(vsca); >>> } >>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { >>> - .release = vimc_sca_release, >>> -}; >>> - >>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>> -{ >>> - struct vimc_sca_device *vsca; >>> - >>> - vsca = container_of(ved, struct vimc_sca_device, ved); >>> - v4l2_device_unregister_subdev(&vsca->sd); >>> -} >>> - >>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>> const char *vcfg_name) >>> { >>> @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, >>> vcfg_name, >>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, >>> - vsca->pads, >>> - &vimc_sca_int_ops, &vimc_sca_ops); >>> + vsca->pads, &vimc_sca_ops); >>> if (ret) { >>> kfree(vsca); >>> return NULL; >>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >>> index 9788fe291193..92daee58209e 100644 >>> --- a/drivers/media/platform/vimc/vimc-sensor.c >>> +++ b/drivers/media/platform/vimc/vimc-sensor.c >>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { >>> .s_ctrl = vimc_sen_s_ctrl, >>> }; >>> -static void vimc_sen_release(struct v4l2_subdev *sd) >>> +void vimc_sen_release(struct vimc_ent_device *ved) >>> { >>> struct vimc_sen_device *vsen = >>> - container_of(sd, struct vimc_sen_device, sd); >>> + container_of(ved, struct vimc_sen_device, ved); >>> v4l2_ctrl_handler_free(&vsen->hdl); >>> tpg_free(&vsen->tpg); >>> @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) >>> kfree(vsen); >>> } >>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { >>> - .release = vimc_sen_release, >>> -}; >>> - >>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>> -{ >>> - struct vimc_sen_device *vsen; >>> - >>> - vsen = container_of(ved, struct vimc_sen_device, ved); >>> - v4l2_device_unregister_subdev(&vsen->sd); >>> -} >>> - >>> /* Image Processing Controls */ >>> static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { >>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>> @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, >>> vcfg_name, >>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, >>> - &vimc_sen_int_ops, &vimc_sen_ops); >>> + &vimc_sen_ops); >>> if (ret) >>> goto err_free_tpg; >>> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-24 11:31 ` Hans Verkuil @ 2020-01-24 12:18 ` Dafna Hirschfeld 2020-01-24 13:21 ` Hans Verkuil 0 siblings, 1 reply; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-24 12:18 UTC (permalink / raw) To: Hans Verkuil, Helen Koike, linux-media; +Cc: ezequiel, skhan, kernel, dafna3 On 24.01.20 12:31, Hans Verkuil wrote: > On 1/23/20 12:36 PM, Dafna Hirschfeld wrote: >> Hi >> >> On 22.01.20 17:39, Helen Koike wrote: >>> Hi Dafna, >>> >>> Thanks for the patch. >>> >>> On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: >>>> A use-after-free bug occures when unbinding the device while it streams. >>>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed >>>> when calling the sensor's 'rm' callback but the freed pointer is >>>> later accessed in the function 'vimc_streamer_pipeline_terminate'. >>>> To fix this bug, move the release callback of the vimc entities >>>> and vimc_device to the release callback of v4l2_device. >>>> The .rm callback of vimc_ent_config is replaced by two callbacks: >>>> >>>> .unregister - this is called upon removing the device and >>>> it unregisters the entity. This is an optional callback since >>>> subdevices don't need to implement it because they are already >>>> unregistered in v4l2_device_unregister. >>>> >>>> .release - this is called from the release callback of v4l2_device >>>> and it frees the entity. >>>> >>>> This ensures that the entities will be released when the last fh >>>> of any of the devices is closed. >>>> >>>> The commands that cause the crash and the KASAN report: >>>> >>>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' >>>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' >>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 >>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >>>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & >>>> sleep 1 >>>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind >>>> >>>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 >>>> [ 188.421800] >>>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 >>>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 >>>> [ 188.425938] Call Trace: >>>> [ 188.426610] dump_stack+0x75/0xa0 >>>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.429057] print_address_description.constprop.6+0x16/0x220 >>>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40 >>>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.436010] kasan_report+0xe/0x20 >>>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] >>>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] >>>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] >>>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] >>>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] >>>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>> [ 188.446179] vimc_remove+0x19/0x70 [vimc] >>>> [ 188.447301] platform_drv_remove+0x2f/0x50 >>>> [ 188.448468] device_release_driver_internal+0x133/0x260 >>>> [ 188.449814] unbind_store+0x121/0x150 >>>> [ 188.450726] kernfs_fop_write+0x142/0x230 >>>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 >>>> [ 188.452826] vfs_write+0xdc/0x230 >>>> [ 188.453760] ksys_write+0xaf/0x140 >>>> [ 188.454702] ? __ia32_sys_read+0x40/0x40 >>>> [ 188.455773] ? __do_page_fault+0x473/0x620 >>>> [ 188.456780] do_syscall_64+0x5e/0x1a0 >>>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>> [ 188.459079] RIP: 0033:0x7f80f1f13504 >>>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 >>>> d4 55 48 89 f5 53 >>>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 >>>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 >>>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 >>>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 >>>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 >>>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 >>>> [ 188.475107] >>>> [ 188.475500] Allocated by task 473: >>>> [ 188.476351] save_stack+0x19/0x80 >>>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 >>>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] >>>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] >>>> [ 188.480776] platform_drv_probe+0x46/0xa0 >>>> [ 188.481829] really_probe+0x16c/0x520 >>>> [ 188.482732] driver_probe_device+0x114/0x170 >>>> [ 188.483783] device_driver_attach+0x85/0x90 >>>> [ 188.484800] __driver_attach+0xa8/0x190 >>>> [ 188.485734] bus_for_each_dev+0xe4/0x140 >>>> [ 188.486702] bus_add_driver+0x223/0x2d0 >>>> [ 188.487715] driver_register+0xca/0x140 >>>> [ 188.488767] 0xffffffffc037003d >>>> [ 188.489635] do_one_initcall+0x86/0x28f >>>> [ 188.490702] do_init_module+0xf8/0x340 >>>> [ 188.491773] load_module+0x3766/0x3a10 >>>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 >>>> [ 188.494059] do_syscall_64+0x5e/0x1a0 >>>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>> [ 188.496481] >>>> [ 188.496893] Freed by task 185: >>>> [ 188.497670] save_stack+0x19/0x80 >>>> [ 188.498493] __kasan_slab_free+0x125/0x170 >>>> [ 188.499486] kfree+0x8c/0x230 >>>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] >>>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] >>>> [ 188.502976] device_release+0x3c/0xd0 >>>> [ 188.503867] kobject_put+0xf4/0x240 >>>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>> [ 188.508846] vimc_remove+0x19/0x70 [vimc] >>>> [ 188.509792] platform_drv_remove+0x2f/0x50 >>>> [ 188.510752] device_release_driver_internal+0x133/0x260 >>>> [ 188.512006] unbind_store+0x121/0x150 >>>> [ 188.512899] kernfs_fop_write+0x142/0x230 >>>> [ 188.513874] vfs_write+0xdc/0x230 >>>> [ 188.514698] ksys_write+0xaf/0x140 >>>> [ 188.515523] do_syscall_64+0x5e/0x1a0 >>>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>> [ 188.517710] >>>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 >>>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 >>>> [ 188.520528] The buggy address is located 8 bytes inside of >>>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) >>>> [ 188.523015] The buggy address belongs to the page: >>>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 >>>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 >>>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 >>>> [ 188.530883] page dumped because: kasan: bad access detected >>>> [ 188.532336] >>>> [ 188.532720] Memory state around the buggy address: >>>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>> [ 188.538996] ^ >>>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> --- >>>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--- >>>> drivers/media/platform/vimc/vimc-common.c | 2 - >>>> drivers/media/platform/vimc/vimc-common.h | 25 ++++----- >>>> drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- >>>> drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- >>>> drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- >>>> drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- >>>> 7 files changed, 71 insertions(+), 85 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>>> index 9a78bb7826a8..c5a645f98c66 100644 >>>> --- a/drivers/media/platform/vimc/vimc-capture.c >>>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { >>>> .link_validate = vimc_vdev_link_validate, >>>> }; >>>> -static void vimc_cap_release(struct video_device *vdev) >>>> +void vimc_cap_release(struct vimc_ent_device *ved) >>>> { >>>> struct vimc_cap_device *vcap = >>>> - container_of(vdev, struct vimc_cap_device, vdev); >>>> + container_of(ved, struct vimc_cap_device, ved); >>>> media_entity_cleanup(vcap->ved.ent); >>>> kfree(vcap); >>>> } >>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>> +void vimc_cap_unregister(struct vimc_ent_device *ved) >>>> { >>>> - struct vimc_cap_device *vcap; >>>> + struct vimc_cap_device *vcap = >>>> + container_of(ved, struct vimc_cap_device, ved); >>>> - vcap = container_of(ved, struct vimc_cap_device, ved); >>>> vb2_queue_release(&vcap->queue); >>>> video_unregister_device(&vcap->vdev); >>>> } >>>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>> vdev = &vcap->vdev; >>>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>>> vdev->entity.ops = &vimc_cap_mops; >>>> - vdev->release = vimc_cap_release; >>>> + vdev->release = video_device_release_empty; >>>> vdev->fops = &vimc_cap_fops; >>>> vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>>> vdev->lock = &vcap->lock; >>>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >>>> index 16ce9f3b7c75..c95c17c048f2 100644 >>>> --- a/drivers/media/platform/vimc/vimc-common.c >>>> +++ b/drivers/media/platform/vimc/vimc-common.c >>>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>> u32 function, >>>> u16 num_pads, >>>> struct media_pad *pads, >>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>> const struct v4l2_subdev_ops *sd_ops) >>>> { >>>> int ret; >>>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>> /* Initialize the subdev */ >>>> v4l2_subdev_init(sd, sd_ops); >>>> - sd->internal_ops = sd_int_ops; >>>> sd->entity.function = function; >>>> sd->entity.ops = &vimc_ent_sd_mops; >>>> sd->owner = THIS_MODULE; >>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >>>> index 1b6ef7196f3c..616d5a6b0754 100644 >>>> --- a/drivers/media/platform/vimc/vimc-common.h >>>> +++ b/drivers/media/platform/vimc/vimc-common.h >>>> @@ -125,16 +125,18 @@ struct vimc_device { >>>> * @name entity name >>>> * @ved pointer to vimc_ent_device (a node in the >>>> * topology) >>>> - * @add subdev add hook - initializes and registers >>>> - * subdev called from vimc-core >>>> - * @rm subdev rm hook - unregisters and frees >>>> - * subdev called from vimc-core >>>> + * @add initializes and registers >>>> + * vim entity - called from vimc-core >>>> + * @unregister unregisters vimc entity - called from vimc-core >>>> + * @release releases vimc entity - called from the v4l2_dev >>>> + * release callback >>>> */ >>> >>> Did you test the suggestion I gave in last patch? >>> That instead of adding another callback here, to just remove the release functions of the subdevices, >>> and add a check if the release cb isn't null just before calling it. >>> With this, the release functions of the video nodes would be called first. >>> >> I just tested you suggestion, here is the patch: https://gitlab.collabora.com/dafna/linux/commit/f59bc7c6643ce8f88aa33eb5e9f413a1aca99589 >> I get a kasan report: >> >> [ 37.736909] BUG: KASAN: use-after-free in v4l2_device_unregister+0xa7/0xf0 [videodev] >> [ 37.738110] Read of size 4 at addr ffff8881eb3f00bc by task bash/227 >> ... >> >> What happen is that the function v4l2_device_unregister loops over the subdevices and unregister them: >> >> /* Unregister subdevs */ >> list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) { >> v4l2_device_unregister_subdev(sd); >> if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) >> v4l2_i2c_subdev_unregister(sd); >> else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) >> v4l2_spi_subdev_unregister(sd); >> } >> >> when unregistering the first subdevice it also immediately calls the release callback because no one holds a fh for >> /dev/v4l2-subdev* , but the release cb releases the memory where the 'sd' in the above loop resides, then when >> referencing it in sd->flags it is after it is already freed. > > Urgh. That's a bug in that function. I think the if...else if bit can be moved to > v4l2_subdev_release(). > > I'm surprised that this hasn't been seen before. > I thought that this is not a bug, this happen in vimc only when implementing the release callback of subdevices which we agreed should not be implemented, or I am missing something? Dafna > Regards, > > Hans > >> >> Dafna >> >>> I followed the discussion regarding fhs in media node and video, not that I'm against your changes, >>> but I would like to understand a bit better why the simple fix suggested isn't enough. >>> >>> Thanks >>> Helen >>> >>>> struct vimc_ent_config { >>>> const char *name; >>>> struct vimc_ent_device *(*add)(struct vimc_device *vimc, >>>> const char *vcfg_name); >>>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>> + void (*unregister)(struct vimc_ent_device *ved); >>>> + void (*release)(struct vimc_ent_device *ved); >>>> }; >>>> /** >>>> @@ -145,22 +147,23 @@ struct vimc_ent_config { >>>> */ >>>> bool vimc_is_source(struct media_entity *ent); >>>> -/* prototypes for vimc_ent_config add and rm hooks */ >>>> +/* prototypes for vimc_ent_config hooks */ >>>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>> const char *vcfg_name); >>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>> +void vimc_cap_unregister(struct vimc_ent_device *ved); >>>> +void vimc_cap_release(struct vimc_ent_device *ved); >>>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>> const char *vcfg_name); >>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>> +void vimc_deb_release(struct vimc_ent_device *ved); >>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>> const char *vcfg_name); >>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>> +void vimc_sca_release(struct vimc_ent_device *ved); >>>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>> const char *vcfg_name); >>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>> +void vimc_sen_release(struct vimc_ent_device *ved); >>>> /** >>>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index >>>> @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); >>>> * @num_pads: number of pads to initialize >>>> * @pads: the array of pads of the entity, the caller should set the >>>> flags of the pads >>>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops >>>> * @sd_ops: pointer to &struct v4l2_subdev_ops. >>>> * >>>> * Helper function initialize and register the struct vimc_ent_device and struct >>>> @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>> u32 function, >>>> u16 num_pads, >>>> struct media_pad *pads, >>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>> const struct v4l2_subdev_ops *sd_ops); >>>> /** >>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c >>>> index 1c55e0382f09..306f9c293628 100644 >>>> --- a/drivers/media/platform/vimc/vimc-core.c >>>> +++ b/drivers/media/platform/vimc/vimc-core.c >>>> @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { >>>> { >>>> .name = "Sensor A", >>>> .add = vimc_sen_add, >>>> - .rm = vimc_sen_rm, >>>> + .release = vimc_sen_release, >>>> }, >>>> { >>>> .name = "Sensor B", >>>> .add = vimc_sen_add, >>>> - .rm = vimc_sen_rm, >>>> + .release = vimc_sen_release, >>>> }, >>>> { >>>> .name = "Debayer A", >>>> .add = vimc_deb_add, >>>> - .rm = vimc_deb_rm, >>>> + .release = vimc_deb_release, >>>> }, >>>> { >>>> .name = "Debayer B", >>>> .add = vimc_deb_add, >>>> - .rm = vimc_deb_rm, >>>> + .release = vimc_deb_release, >>>> }, >>>> { >>>> .name = "Raw Capture 0", >>>> .add = vimc_cap_add, >>>> - .rm = vimc_cap_rm, >>>> + .unregister = vimc_cap_unregister, >>>> + .release = vimc_cap_release, >>>> }, >>>> { >>>> .name = "Raw Capture 1", >>>> .add = vimc_cap_add, >>>> - .rm = vimc_cap_rm, >>>> + .unregister = vimc_cap_unregister, >>>> + .release = vimc_cap_release, >>>> }, >>>> { >>>> /* TODO: change this to vimc-input when it is implemented */ >>>> .name = "RGB/YUV Input", >>>> .add = vimc_sen_add, >>>> - .rm = vimc_sen_rm, >>>> + .release = vimc_sen_release, >>>> }, >>>> { >>>> .name = "Scaler", >>>> .add = vimc_sca_add, >>>> - .rm = vimc_sca_rm, >>>> + .release = vimc_sca_release, >>>> }, >>>> { >>>> .name = "RGB/YUV Capture", >>>> .add = vimc_cap_add, >>>> - .rm = vimc_cap_rm, >>>> + .unregister = vimc_cap_unregister, >>>> + .release = vimc_cap_release, >>>> }, >>>> }; >>>> @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>>> return 0; >>>> } >>>> -static void vimc_rm_subdevs(struct vimc_device *vimc) >>>> +static void vimc_release_subdevs(struct vimc_device *vimc) >>>> { >>>> unsigned int i; >>>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>> if (vimc->ent_devs[i]) >>>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); >>>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); >>>> +} >>>> + >>>> +static void vimc_unregister_subdevs(struct vimc_device *vimc) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) >>>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); >>>> +} >>>> + >>>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>>> +{ >>>> + struct vimc_device *vimc = >>>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>>> + >>>> + vimc_release_subdevs(vimc); >>>> + media_device_cleanup(&vimc->mdev); >>>> + kfree(vimc->ent_devs); >>>> + kfree(vimc); >>>> } >>>> static int vimc_register_devices(struct vimc_device *vimc) >>>> @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>> "v4l2 device register failed (err=%d)\n", ret); >>>> return ret; >>>> } >>>> - >>>> /* allocate ent_devs */ >>>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, >>>> sizeof(*vimc->ent_devs), GFP_KERNEL); >>>> @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>> err_mdev_unregister: >>>> media_device_unregister(&vimc->mdev); >>>> - media_device_cleanup(&vimc->mdev); >>>> err_rm_subdevs: >>>> - vimc_rm_subdevs(vimc); >>>> + vimc_unregister_subdevs(vimc); >>>> + vimc_release_subdevs(vimc); >>>> kfree(vimc->ent_devs); >>>> err_v4l2_unregister: >>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>> @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>> static void vimc_unregister(struct vimc_device *vimc) >>>> { >>>> + vimc_unregister_subdevs(vimc); >>>> media_device_unregister(&vimc->mdev); >>>> - media_device_cleanup(&vimc->mdev); >>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>> - kfree(vimc->ent_devs); >>>> - kfree(vimc); >>>> } >>>> static int vimc_probe(struct platform_device *pdev) >>>> @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) >>>> kfree(vimc); >>>> return ret; >>>> } >>>> + /* >>>> + * the release cb is set only after successful registration. >>>> + * if the registration fails, we release directly from probe >>>> + */ >>>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release; >>>> platform_set_drvdata(pdev, vimc); >>>> return 0; >>>> } >>>> @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) >>>> dev_dbg(&pdev->dev, "remove"); >>>> - vimc_rm_subdevs(vimc); >>>> vimc_unregister(vimc); >>>> + v4l2_device_put(&vimc->v4l2_dev); >>>> return 0; >>>> } >>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>>> index 34b98b235880..baf6bf9f65b5 100644 >>>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>>> @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { >>>> .s_ctrl = vimc_deb_s_ctrl, >>>> }; >>>> -static void vimc_deb_release(struct v4l2_subdev *sd) >>>> +void vimc_deb_release(struct vimc_ent_device *ved) >>>> { >>>> struct vimc_deb_device *vdeb = >>>> - container_of(sd, struct vimc_deb_device, sd); >>>> + container_of(ved, struct vimc_deb_device, ved); >>>> v4l2_ctrl_handler_free(&vdeb->hdl); >>>> media_entity_cleanup(vdeb->ved.ent); >>>> kfree(vdeb); >>>> } >>>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { >>>> - .release = vimc_deb_release, >>>> -}; >>>> - >>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>> -{ >>>> - struct vimc_deb_device *vdeb; >>>> - >>>> - vdeb = container_of(ved, struct vimc_deb_device, ved); >>>> - v4l2_device_unregister_subdev(&vdeb->sd); >>>> -} >>>> - >>>> static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { >>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>> .id = VIMC_CID_VIMC_CLASS, >>>> @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, >>>> vcfg_name, >>>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, >>>> - vdeb->pads, >>>> - &vimc_deb_int_ops, &vimc_deb_ops); >>>> + vdeb->pads, &vimc_deb_ops); >>>> if (ret) >>>> goto err_free_hdl; >>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >>>> index cb2dc24c3e59..7521439747c5 100644 >>>> --- a/drivers/media/platform/vimc/vimc-scaler.c >>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c >>>> @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, >>>> return vsca->src_frame; >>>> }; >>>> -static void vimc_sca_release(struct v4l2_subdev *sd) >>>> +void vimc_sca_release(struct vimc_ent_device *ved) >>>> { >>>> struct vimc_sca_device *vsca = >>>> - container_of(sd, struct vimc_sca_device, sd); >>>> + container_of(ved, struct vimc_sca_device, ved); >>>> media_entity_cleanup(vsca->ved.ent); >>>> kfree(vsca); >>>> } >>>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { >>>> - .release = vimc_sca_release, >>>> -}; >>>> - >>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>> -{ >>>> - struct vimc_sca_device *vsca; >>>> - >>>> - vsca = container_of(ved, struct vimc_sca_device, ved); >>>> - v4l2_device_unregister_subdev(&vsca->sd); >>>> -} >>>> - >>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>> const char *vcfg_name) >>>> { >>>> @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, >>>> vcfg_name, >>>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, >>>> - vsca->pads, >>>> - &vimc_sca_int_ops, &vimc_sca_ops); >>>> + vsca->pads, &vimc_sca_ops); >>>> if (ret) { >>>> kfree(vsca); >>>> return NULL; >>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >>>> index 9788fe291193..92daee58209e 100644 >>>> --- a/drivers/media/platform/vimc/vimc-sensor.c >>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c >>>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { >>>> .s_ctrl = vimc_sen_s_ctrl, >>>> }; >>>> -static void vimc_sen_release(struct v4l2_subdev *sd) >>>> +void vimc_sen_release(struct vimc_ent_device *ved) >>>> { >>>> struct vimc_sen_device *vsen = >>>> - container_of(sd, struct vimc_sen_device, sd); >>>> + container_of(ved, struct vimc_sen_device, ved); >>>> v4l2_ctrl_handler_free(&vsen->hdl); >>>> tpg_free(&vsen->tpg); >>>> @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) >>>> kfree(vsen); >>>> } >>>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { >>>> - .release = vimc_sen_release, >>>> -}; >>>> - >>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>> -{ >>>> - struct vimc_sen_device *vsen; >>>> - >>>> - vsen = container_of(ved, struct vimc_sen_device, ved); >>>> - v4l2_device_unregister_subdev(&vsen->sd); >>>> -} >>>> - >>>> /* Image Processing Controls */ >>>> static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { >>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>> @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, >>>> vcfg_name, >>>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, >>>> - &vimc_sen_int_ops, &vimc_sen_ops); >>>> + &vimc_sen_ops); >>>> if (ret) >>>> goto err_free_tpg; >>>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-24 12:18 ` Dafna Hirschfeld @ 2020-01-24 13:21 ` Hans Verkuil 2020-01-31 17:41 ` Dafna Hirschfeld 0 siblings, 1 reply; 10+ messages in thread From: Hans Verkuil @ 2020-01-24 13:21 UTC (permalink / raw) To: Dafna Hirschfeld, Helen Koike, linux-media Cc: ezequiel, skhan, kernel, dafna3 On 1/24/20 1:18 PM, Dafna Hirschfeld wrote: > > > On 24.01.20 12:31, Hans Verkuil wrote: >> On 1/23/20 12:36 PM, Dafna Hirschfeld wrote: >>> Hi >>> >>> On 22.01.20 17:39, Helen Koike wrote: >>>> Hi Dafna, >>>> >>>> Thanks for the patch. >>>> >>>> On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: >>>>> A use-after-free bug occures when unbinding the device while it streams. >>>>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed >>>>> when calling the sensor's 'rm' callback but the freed pointer is >>>>> later accessed in the function 'vimc_streamer_pipeline_terminate'. >>>>> To fix this bug, move the release callback of the vimc entities >>>>> and vimc_device to the release callback of v4l2_device. >>>>> The .rm callback of vimc_ent_config is replaced by two callbacks: >>>>> >>>>> .unregister - this is called upon removing the device and >>>>> it unregisters the entity. This is an optional callback since >>>>> subdevices don't need to implement it because they are already >>>>> unregistered in v4l2_device_unregister. >>>>> >>>>> .release - this is called from the release callback of v4l2_device >>>>> and it frees the entity. >>>>> >>>>> This ensures that the entities will be released when the last fh >>>>> of any of the devices is closed. >>>>> >>>>> The commands that cause the crash and the KASAN report: >>>>> >>>>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' >>>>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' >>>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 >>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >>>>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & >>>>> sleep 1 >>>>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind >>>>> >>>>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 >>>>> [ 188.421800] >>>>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 >>>>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 >>>>> [ 188.425938] Call Trace: >>>>> [ 188.426610] dump_stack+0x75/0xa0 >>>>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.429057] print_address_description.constprop.6+0x16/0x220 >>>>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40 >>>>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.436010] kasan_report+0xe/0x20 >>>>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] >>>>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] >>>>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] >>>>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] >>>>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] >>>>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>>> [ 188.446179] vimc_remove+0x19/0x70 [vimc] >>>>> [ 188.447301] platform_drv_remove+0x2f/0x50 >>>>> [ 188.448468] device_release_driver_internal+0x133/0x260 >>>>> [ 188.449814] unbind_store+0x121/0x150 >>>>> [ 188.450726] kernfs_fop_write+0x142/0x230 >>>>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 >>>>> [ 188.452826] vfs_write+0xdc/0x230 >>>>> [ 188.453760] ksys_write+0xaf/0x140 >>>>> [ 188.454702] ? __ia32_sys_read+0x40/0x40 >>>>> [ 188.455773] ? __do_page_fault+0x473/0x620 >>>>> [ 188.456780] do_syscall_64+0x5e/0x1a0 >>>>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>> [ 188.459079] RIP: 0033:0x7f80f1f13504 >>>>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 >>>>> d4 55 48 89 f5 53 >>>>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 >>>>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 >>>>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 >>>>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 >>>>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 >>>>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 >>>>> [ 188.475107] >>>>> [ 188.475500] Allocated by task 473: >>>>> [ 188.476351] save_stack+0x19/0x80 >>>>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 >>>>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] >>>>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] >>>>> [ 188.480776] platform_drv_probe+0x46/0xa0 >>>>> [ 188.481829] really_probe+0x16c/0x520 >>>>> [ 188.482732] driver_probe_device+0x114/0x170 >>>>> [ 188.483783] device_driver_attach+0x85/0x90 >>>>> [ 188.484800] __driver_attach+0xa8/0x190 >>>>> [ 188.485734] bus_for_each_dev+0xe4/0x140 >>>>> [ 188.486702] bus_add_driver+0x223/0x2d0 >>>>> [ 188.487715] driver_register+0xca/0x140 >>>>> [ 188.488767] 0xffffffffc037003d >>>>> [ 188.489635] do_one_initcall+0x86/0x28f >>>>> [ 188.490702] do_init_module+0xf8/0x340 >>>>> [ 188.491773] load_module+0x3766/0x3a10 >>>>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 >>>>> [ 188.494059] do_syscall_64+0x5e/0x1a0 >>>>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>> [ 188.496481] >>>>> [ 188.496893] Freed by task 185: >>>>> [ 188.497670] save_stack+0x19/0x80 >>>>> [ 188.498493] __kasan_slab_free+0x125/0x170 >>>>> [ 188.499486] kfree+0x8c/0x230 >>>>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] >>>>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] >>>>> [ 188.502976] device_release+0x3c/0xd0 >>>>> [ 188.503867] kobject_put+0xf4/0x240 >>>>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>>> [ 188.508846] vimc_remove+0x19/0x70 [vimc] >>>>> [ 188.509792] platform_drv_remove+0x2f/0x50 >>>>> [ 188.510752] device_release_driver_internal+0x133/0x260 >>>>> [ 188.512006] unbind_store+0x121/0x150 >>>>> [ 188.512899] kernfs_fop_write+0x142/0x230 >>>>> [ 188.513874] vfs_write+0xdc/0x230 >>>>> [ 188.514698] ksys_write+0xaf/0x140 >>>>> [ 188.515523] do_syscall_64+0x5e/0x1a0 >>>>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>> [ 188.517710] >>>>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 >>>>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 >>>>> [ 188.520528] The buggy address is located 8 bytes inside of >>>>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) >>>>> [ 188.523015] The buggy address belongs to the page: >>>>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 >>>>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 >>>>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 >>>>> [ 188.530883] page dumped because: kasan: bad access detected >>>>> [ 188.532336] >>>>> [ 188.532720] Memory state around the buggy address: >>>>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>> [ 188.538996] ^ >>>>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>> --- >>>>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--- >>>>> drivers/media/platform/vimc/vimc-common.c | 2 - >>>>> drivers/media/platform/vimc/vimc-common.h | 25 ++++----- >>>>> drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- >>>>> drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- >>>>> drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- >>>>> drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- >>>>> 7 files changed, 71 insertions(+), 85 deletions(-) >>>>> >>>>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>>>> index 9a78bb7826a8..c5a645f98c66 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-capture.c >>>>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>>>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { >>>>> .link_validate = vimc_vdev_link_validate, >>>>> }; >>>>> -static void vimc_cap_release(struct video_device *vdev) >>>>> +void vimc_cap_release(struct vimc_ent_device *ved) >>>>> { >>>>> struct vimc_cap_device *vcap = >>>>> - container_of(vdev, struct vimc_cap_device, vdev); >>>>> + container_of(ved, struct vimc_cap_device, ved); >>>>> media_entity_cleanup(vcap->ved.ent); >>>>> kfree(vcap); >>>>> } >>>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>> +void vimc_cap_unregister(struct vimc_ent_device *ved) >>>>> { >>>>> - struct vimc_cap_device *vcap; >>>>> + struct vimc_cap_device *vcap = >>>>> + container_of(ved, struct vimc_cap_device, ved); >>>>> - vcap = container_of(ved, struct vimc_cap_device, ved); >>>>> vb2_queue_release(&vcap->queue); >>>>> video_unregister_device(&vcap->vdev); >>>>> } >>>>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>>> vdev = &vcap->vdev; >>>>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>>>> vdev->entity.ops = &vimc_cap_mops; >>>>> - vdev->release = vimc_cap_release; >>>>> + vdev->release = video_device_release_empty; >>>>> vdev->fops = &vimc_cap_fops; >>>>> vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>>>> vdev->lock = &vcap->lock; >>>>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >>>>> index 16ce9f3b7c75..c95c17c048f2 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-common.c >>>>> +++ b/drivers/media/platform/vimc/vimc-common.c >>>>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>> u32 function, >>>>> u16 num_pads, >>>>> struct media_pad *pads, >>>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>>> const struct v4l2_subdev_ops *sd_ops) >>>>> { >>>>> int ret; >>>>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>> /* Initialize the subdev */ >>>>> v4l2_subdev_init(sd, sd_ops); >>>>> - sd->internal_ops = sd_int_ops; >>>>> sd->entity.function = function; >>>>> sd->entity.ops = &vimc_ent_sd_mops; >>>>> sd->owner = THIS_MODULE; >>>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >>>>> index 1b6ef7196f3c..616d5a6b0754 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-common.h >>>>> +++ b/drivers/media/platform/vimc/vimc-common.h >>>>> @@ -125,16 +125,18 @@ struct vimc_device { >>>>> * @name entity name >>>>> * @ved pointer to vimc_ent_device (a node in the >>>>> * topology) >>>>> - * @add subdev add hook - initializes and registers >>>>> - * subdev called from vimc-core >>>>> - * @rm subdev rm hook - unregisters and frees >>>>> - * subdev called from vimc-core >>>>> + * @add initializes and registers >>>>> + * vim entity - called from vimc-core >>>>> + * @unregister unregisters vimc entity - called from vimc-core >>>>> + * @release releases vimc entity - called from the v4l2_dev >>>>> + * release callback >>>>> */ >>>> >>>> Did you test the suggestion I gave in last patch? >>>> That instead of adding another callback here, to just remove the release functions of the subdevices, >>>> and add a check if the release cb isn't null just before calling it. >>>> With this, the release functions of the video nodes would be called first. >>>> >>> I just tested you suggestion, here is the patch: https://gitlab.collabora.com/dafna/linux/commit/f59bc7c6643ce8f88aa33eb5e9f413a1aca99589 >>> I get a kasan report: >>> >>> [ 37.736909] BUG: KASAN: use-after-free in v4l2_device_unregister+0xa7/0xf0 [videodev] >>> [ 37.738110] Read of size 4 at addr ffff8881eb3f00bc by task bash/227 >>> ... >>> >>> What happen is that the function v4l2_device_unregister loops over the subdevices and unregister them: >>> >>> /* Unregister subdevs */ >>> list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) { >>> v4l2_device_unregister_subdev(sd); >>> if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) >>> v4l2_i2c_subdev_unregister(sd); >>> else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) >>> v4l2_spi_subdev_unregister(sd); >>> } >>> >>> when unregistering the first subdevice it also immediately calls the release callback because no one holds a fh for >>> /dev/v4l2-subdev* , but the release cb releases the memory where the 'sd' in the above loop resides, then when >>> referencing it in sd->flags it is after it is already freed. >> >> Urgh. That's a bug in that function. I think the if...else if bit can be moved to >> v4l2_subdev_release(). >> >> I'm surprised that this hasn't been seen before. >> I thought that this is not a bug, this happen in vimc only when implementing the release callback > of subdevices which we agreed should not be implemented, or I am missing something? That too, but in general it is just a bad idea to unregister a subdev and keep using the sd. I think it makes sense to move this bit to v4l2_subdev_release(). Regards, Hans > > Dafna > >> Regards, >> >> Hans >> >>> >>> Dafna >>> >>>> I followed the discussion regarding fhs in media node and video, not that I'm against your changes, >>>> but I would like to understand a bit better why the simple fix suggested isn't enough. >>>> >>>> Thanks >>>> Helen >>>> >>>>> struct vimc_ent_config { >>>>> const char *name; >>>>> struct vimc_ent_device *(*add)(struct vimc_device *vimc, >>>>> const char *vcfg_name); >>>>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>> + void (*unregister)(struct vimc_ent_device *ved); >>>>> + void (*release)(struct vimc_ent_device *ved); >>>>> }; >>>>> /** >>>>> @@ -145,22 +147,23 @@ struct vimc_ent_config { >>>>> */ >>>>> bool vimc_is_source(struct media_entity *ent); >>>>> -/* prototypes for vimc_ent_config add and rm hooks */ >>>>> +/* prototypes for vimc_ent_config hooks */ >>>>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>>> const char *vcfg_name); >>>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>> +void vimc_cap_unregister(struct vimc_ent_device *ved); >>>>> +void vimc_cap_release(struct vimc_ent_device *ved); >>>>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>>> const char *vcfg_name); >>>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>> +void vimc_deb_release(struct vimc_ent_device *ved); >>>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>> const char *vcfg_name); >>>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>> +void vimc_sca_release(struct vimc_ent_device *ved); >>>>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>>> const char *vcfg_name); >>>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>> +void vimc_sen_release(struct vimc_ent_device *ved); >>>>> /** >>>>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index >>>>> @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); >>>>> * @num_pads: number of pads to initialize >>>>> * @pads: the array of pads of the entity, the caller should set the >>>>> flags of the pads >>>>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops >>>>> * @sd_ops: pointer to &struct v4l2_subdev_ops. >>>>> * >>>>> * Helper function initialize and register the struct vimc_ent_device and struct >>>>> @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>> u32 function, >>>>> u16 num_pads, >>>>> struct media_pad *pads, >>>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>>> const struct v4l2_subdev_ops *sd_ops); >>>>> /** >>>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c >>>>> index 1c55e0382f09..306f9c293628 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-core.c >>>>> +++ b/drivers/media/platform/vimc/vimc-core.c >>>>> @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { >>>>> { >>>>> .name = "Sensor A", >>>>> .add = vimc_sen_add, >>>>> - .rm = vimc_sen_rm, >>>>> + .release = vimc_sen_release, >>>>> }, >>>>> { >>>>> .name = "Sensor B", >>>>> .add = vimc_sen_add, >>>>> - .rm = vimc_sen_rm, >>>>> + .release = vimc_sen_release, >>>>> }, >>>>> { >>>>> .name = "Debayer A", >>>>> .add = vimc_deb_add, >>>>> - .rm = vimc_deb_rm, >>>>> + .release = vimc_deb_release, >>>>> }, >>>>> { >>>>> .name = "Debayer B", >>>>> .add = vimc_deb_add, >>>>> - .rm = vimc_deb_rm, >>>>> + .release = vimc_deb_release, >>>>> }, >>>>> { >>>>> .name = "Raw Capture 0", >>>>> .add = vimc_cap_add, >>>>> - .rm = vimc_cap_rm, >>>>> + .unregister = vimc_cap_unregister, >>>>> + .release = vimc_cap_release, >>>>> }, >>>>> { >>>>> .name = "Raw Capture 1", >>>>> .add = vimc_cap_add, >>>>> - .rm = vimc_cap_rm, >>>>> + .unregister = vimc_cap_unregister, >>>>> + .release = vimc_cap_release, >>>>> }, >>>>> { >>>>> /* TODO: change this to vimc-input when it is implemented */ >>>>> .name = "RGB/YUV Input", >>>>> .add = vimc_sen_add, >>>>> - .rm = vimc_sen_rm, >>>>> + .release = vimc_sen_release, >>>>> }, >>>>> { >>>>> .name = "Scaler", >>>>> .add = vimc_sca_add, >>>>> - .rm = vimc_sca_rm, >>>>> + .release = vimc_sca_release, >>>>> }, >>>>> { >>>>> .name = "RGB/YUV Capture", >>>>> .add = vimc_cap_add, >>>>> - .rm = vimc_cap_rm, >>>>> + .unregister = vimc_cap_unregister, >>>>> + .release = vimc_cap_release, >>>>> }, >>>>> }; >>>>> @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>>>> return 0; >>>>> } >>>>> -static void vimc_rm_subdevs(struct vimc_device *vimc) >>>>> +static void vimc_release_subdevs(struct vimc_device *vimc) >>>>> { >>>>> unsigned int i; >>>>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>>> if (vimc->ent_devs[i]) >>>>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); >>>>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); >>>>> +} >>>>> + >>>>> +static void vimc_unregister_subdevs(struct vimc_device *vimc) >>>>> +{ >>>>> + unsigned int i; >>>>> + >>>>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>>> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) >>>>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); >>>>> +} >>>>> + >>>>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>>>> +{ >>>>> + struct vimc_device *vimc = >>>>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>>>> + >>>>> + vimc_release_subdevs(vimc); >>>>> + media_device_cleanup(&vimc->mdev); >>>>> + kfree(vimc->ent_devs); >>>>> + kfree(vimc); >>>>> } >>>>> static int vimc_register_devices(struct vimc_device *vimc) >>>>> @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>> "v4l2 device register failed (err=%d)\n", ret); >>>>> return ret; >>>>> } >>>>> - >>>>> /* allocate ent_devs */ >>>>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, >>>>> sizeof(*vimc->ent_devs), GFP_KERNEL); >>>>> @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>> err_mdev_unregister: >>>>> media_device_unregister(&vimc->mdev); >>>>> - media_device_cleanup(&vimc->mdev); >>>>> err_rm_subdevs: >>>>> - vimc_rm_subdevs(vimc); >>>>> + vimc_unregister_subdevs(vimc); >>>>> + vimc_release_subdevs(vimc); >>>>> kfree(vimc->ent_devs); >>>>> err_v4l2_unregister: >>>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>>> @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>> static void vimc_unregister(struct vimc_device *vimc) >>>>> { >>>>> + vimc_unregister_subdevs(vimc); >>>>> media_device_unregister(&vimc->mdev); >>>>> - media_device_cleanup(&vimc->mdev); >>>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>>> - kfree(vimc->ent_devs); >>>>> - kfree(vimc); >>>>> } >>>>> static int vimc_probe(struct platform_device *pdev) >>>>> @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) >>>>> kfree(vimc); >>>>> return ret; >>>>> } >>>>> + /* >>>>> + * the release cb is set only after successful registration. >>>>> + * if the registration fails, we release directly from probe >>>>> + */ >>>>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release; >>>>> platform_set_drvdata(pdev, vimc); >>>>> return 0; >>>>> } >>>>> @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) >>>>> dev_dbg(&pdev->dev, "remove"); >>>>> - vimc_rm_subdevs(vimc); >>>>> vimc_unregister(vimc); >>>>> + v4l2_device_put(&vimc->v4l2_dev); >>>>> return 0; >>>>> } >>>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>>>> index 34b98b235880..baf6bf9f65b5 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>>>> @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { >>>>> .s_ctrl = vimc_deb_s_ctrl, >>>>> }; >>>>> -static void vimc_deb_release(struct v4l2_subdev *sd) >>>>> +void vimc_deb_release(struct vimc_ent_device *ved) >>>>> { >>>>> struct vimc_deb_device *vdeb = >>>>> - container_of(sd, struct vimc_deb_device, sd); >>>>> + container_of(ved, struct vimc_deb_device, ved); >>>>> v4l2_ctrl_handler_free(&vdeb->hdl); >>>>> media_entity_cleanup(vdeb->ved.ent); >>>>> kfree(vdeb); >>>>> } >>>>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { >>>>> - .release = vimc_deb_release, >>>>> -}; >>>>> - >>>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>> -{ >>>>> - struct vimc_deb_device *vdeb; >>>>> - >>>>> - vdeb = container_of(ved, struct vimc_deb_device, ved); >>>>> - v4l2_device_unregister_subdev(&vdeb->sd); >>>>> -} >>>>> - >>>>> static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { >>>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>>> .id = VIMC_CID_VIMC_CLASS, >>>>> @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, >>>>> vcfg_name, >>>>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, >>>>> - vdeb->pads, >>>>> - &vimc_deb_int_ops, &vimc_deb_ops); >>>>> + vdeb->pads, &vimc_deb_ops); >>>>> if (ret) >>>>> goto err_free_hdl; >>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >>>>> index cb2dc24c3e59..7521439747c5 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c >>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c >>>>> @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, >>>>> return vsca->src_frame; >>>>> }; >>>>> -static void vimc_sca_release(struct v4l2_subdev *sd) >>>>> +void vimc_sca_release(struct vimc_ent_device *ved) >>>>> { >>>>> struct vimc_sca_device *vsca = >>>>> - container_of(sd, struct vimc_sca_device, sd); >>>>> + container_of(ved, struct vimc_sca_device, ved); >>>>> media_entity_cleanup(vsca->ved.ent); >>>>> kfree(vsca); >>>>> } >>>>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { >>>>> - .release = vimc_sca_release, >>>>> -}; >>>>> - >>>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>> -{ >>>>> - struct vimc_sca_device *vsca; >>>>> - >>>>> - vsca = container_of(ved, struct vimc_sca_device, ved); >>>>> - v4l2_device_unregister_subdev(&vsca->sd); >>>>> -} >>>>> - >>>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>> const char *vcfg_name) >>>>> { >>>>> @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, >>>>> vcfg_name, >>>>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, >>>>> - vsca->pads, >>>>> - &vimc_sca_int_ops, &vimc_sca_ops); >>>>> + vsca->pads, &vimc_sca_ops); >>>>> if (ret) { >>>>> kfree(vsca); >>>>> return NULL; >>>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >>>>> index 9788fe291193..92daee58209e 100644 >>>>> --- a/drivers/media/platform/vimc/vimc-sensor.c >>>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c >>>>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { >>>>> .s_ctrl = vimc_sen_s_ctrl, >>>>> }; >>>>> -static void vimc_sen_release(struct v4l2_subdev *sd) >>>>> +void vimc_sen_release(struct vimc_ent_device *ved) >>>>> { >>>>> struct vimc_sen_device *vsen = >>>>> - container_of(sd, struct vimc_sen_device, sd); >>>>> + container_of(ved, struct vimc_sen_device, ved); >>>>> v4l2_ctrl_handler_free(&vsen->hdl); >>>>> tpg_free(&vsen->tpg); >>>>> @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) >>>>> kfree(vsen); >>>>> } >>>>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { >>>>> - .release = vimc_sen_release, >>>>> -}; >>>>> - >>>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>> -{ >>>>> - struct vimc_sen_device *vsen; >>>>> - >>>>> - vsen = container_of(ved, struct vimc_sen_device, ved); >>>>> - v4l2_device_unregister_subdev(&vsen->sd); >>>>> -} >>>>> - >>>>> /* Image Processing Controls */ >>>>> static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { >>>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>>> @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, >>>>> vcfg_name, >>>>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, >>>>> - &vimc_sen_int_ops, &vimc_sen_ops); >>>>> + &vimc_sen_ops); >>>>> if (ret) >>>>> goto err_free_tpg; >>>>> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release 2020-01-24 13:21 ` Hans Verkuil @ 2020-01-31 17:41 ` Dafna Hirschfeld 0 siblings, 0 replies; 10+ messages in thread From: Dafna Hirschfeld @ 2020-01-31 17:41 UTC (permalink / raw) To: Hans Verkuil, Helen Koike, linux-media; +Cc: ezequiel, skhan, kernel, dafna3 On 24.01.20 14:21, Hans Verkuil wrote: > On 1/24/20 1:18 PM, Dafna Hirschfeld wrote: >> >> >> On 24.01.20 12:31, Hans Verkuil wrote: >>> On 1/23/20 12:36 PM, Dafna Hirschfeld wrote: >>>> Hi >>>> >>>> On 22.01.20 17:39, Helen Koike wrote: >>>>> Hi Dafna, >>>>> >>>>> Thanks for the patch. >>>>> >>>>> On 1/22/20 2:01 PM, Dafna Hirschfeld wrote: >>>>>> A use-after-free bug occures when unbinding the device while it streams. >>>>>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed >>>>>> when calling the sensor's 'rm' callback but the freed pointer is >>>>>> later accessed in the function 'vimc_streamer_pipeline_terminate'. >>>>>> To fix this bug, move the release callback of the vimc entities >>>>>> and vimc_device to the release callback of v4l2_device. >>>>>> The .rm callback of vimc_ent_config is replaced by two callbacks: >>>>>> >>>>>> .unregister - this is called upon removing the device and >>>>>> it unregisters the entity. This is an optional callback since >>>>>> subdevices don't need to implement it because they are already >>>>>> unregistered in v4l2_device_unregister. >>>>>> >>>>>> .release - this is called from the release callback of v4l2_device >>>>>> and it frees the entity. >>>>>> >>>>>> This ensures that the entities will be released when the last fh >>>>>> of any of the devices is closed. >>>>>> >>>>>> The commands that cause the crash and the KASAN report: >>>>>> >>>>>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]' >>>>>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]' >>>>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 >>>>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81 >>>>>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 & >>>>>> sleep 1 >>>>>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind >>>>>> >>>>>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185 >>>>>> [ 188.421800] >>>>>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1 >>>>>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 >>>>>> [ 188.425938] Call Trace: >>>>>> [ 188.426610] dump_stack+0x75/0xa0 >>>>>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.429057] print_address_description.constprop.6+0x16/0x220 >>>>>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40 >>>>>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.436010] kasan_report+0xe/0x20 >>>>>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc] >>>>>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc] >>>>>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc] >>>>>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common] >>>>>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common] >>>>>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc] >>>>>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>>>> [ 188.446179] vimc_remove+0x19/0x70 [vimc] >>>>>> [ 188.447301] platform_drv_remove+0x2f/0x50 >>>>>> [ 188.448468] device_release_driver_internal+0x133/0x260 >>>>>> [ 188.449814] unbind_store+0x121/0x150 >>>>>> [ 188.450726] kernfs_fop_write+0x142/0x230 >>>>>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100 >>>>>> [ 188.452826] vfs_write+0xdc/0x230 >>>>>> [ 188.453760] ksys_write+0xaf/0x140 >>>>>> [ 188.454702] ? __ia32_sys_read+0x40/0x40 >>>>>> [ 188.455773] ? __do_page_fault+0x473/0x620 >>>>>> [ 188.456780] do_syscall_64+0x5e/0x1a0 >>>>>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>>> [ 188.459079] RIP: 0033:0x7f80f1f13504 >>>>>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 >>>>>> d4 55 48 89 f5 53 >>>>>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 >>>>>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504 >>>>>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001 >>>>>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740 >>>>>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760 >>>>>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006 >>>>>> [ 188.475107] >>>>>> [ 188.475500] Allocated by task 473: >>>>>> [ 188.476351] save_stack+0x19/0x80 >>>>>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0 >>>>>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc] >>>>>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc] >>>>>> [ 188.480776] platform_drv_probe+0x46/0xa0 >>>>>> [ 188.481829] really_probe+0x16c/0x520 >>>>>> [ 188.482732] driver_probe_device+0x114/0x170 >>>>>> [ 188.483783] device_driver_attach+0x85/0x90 >>>>>> [ 188.484800] __driver_attach+0xa8/0x190 >>>>>> [ 188.485734] bus_for_each_dev+0xe4/0x140 >>>>>> [ 188.486702] bus_add_driver+0x223/0x2d0 >>>>>> [ 188.487715] driver_register+0xca/0x140 >>>>>> [ 188.488767] 0xffffffffc037003d >>>>>> [ 188.489635] do_one_initcall+0x86/0x28f >>>>>> [ 188.490702] do_init_module+0xf8/0x340 >>>>>> [ 188.491773] load_module+0x3766/0x3a10 >>>>>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0 >>>>>> [ 188.494059] do_syscall_64+0x5e/0x1a0 >>>>>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>>> [ 188.496481] >>>>>> [ 188.496893] Freed by task 185: >>>>>> [ 188.497670] save_stack+0x19/0x80 >>>>>> [ 188.498493] __kasan_slab_free+0x125/0x170 >>>>>> [ 188.499486] kfree+0x8c/0x230 >>>>>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev] >>>>>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev] >>>>>> [ 188.502976] device_release+0x3c/0xd0 >>>>>> [ 188.503867] kobject_put+0xf4/0x240 >>>>>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc] >>>>>> [ 188.508846] vimc_remove+0x19/0x70 [vimc] >>>>>> [ 188.509792] platform_drv_remove+0x2f/0x50 >>>>>> [ 188.510752] device_release_driver_internal+0x133/0x260 >>>>>> [ 188.512006] unbind_store+0x121/0x150 >>>>>> [ 188.512899] kernfs_fop_write+0x142/0x230 >>>>>> [ 188.513874] vfs_write+0xdc/0x230 >>>>>> [ 188.514698] ksys_write+0xaf/0x140 >>>>>> [ 188.515523] do_syscall_64+0x5e/0x1a0 >>>>>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>>>>> [ 188.517710] >>>>>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000 >>>>>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096 >>>>>> [ 188.520528] The buggy address is located 8 bytes inside of >>>>>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000) >>>>>> [ 188.523015] The buggy address belongs to the page: >>>>>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0 >>>>>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140 >>>>>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 >>>>>> [ 188.530883] page dumped because: kasan: bad access detected >>>>>> [ 188.532336] >>>>>> [ 188.532720] Memory state around the buggy address: >>>>>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>>>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >>>>>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>>> [ 188.538996] ^ >>>>>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>>>>> >>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>>> --- >>>>>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--- >>>>>> drivers/media/platform/vimc/vimc-common.c | 2 - >>>>>> drivers/media/platform/vimc/vimc-common.h | 25 ++++----- >>>>>> drivers/media/platform/vimc/vimc-core.c | 61 +++++++++++++++------- >>>>>> drivers/media/platform/vimc/vimc-debayer.c | 19 ++----- >>>>>> drivers/media/platform/vimc/vimc-scaler.c | 19 ++----- >>>>>> drivers/media/platform/vimc/vimc-sensor.c | 18 ++----- >>>>>> 7 files changed, 71 insertions(+), 85 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >>>>>> index 9a78bb7826a8..c5a645f98c66 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-capture.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-capture.c >>>>>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = { >>>>>> .link_validate = vimc_vdev_link_validate, >>>>>> }; >>>>>> -static void vimc_cap_release(struct video_device *vdev) >>>>>> +void vimc_cap_release(struct vimc_ent_device *ved) >>>>>> { >>>>>> struct vimc_cap_device *vcap = >>>>>> - container_of(vdev, struct vimc_cap_device, vdev); >>>>>> + container_of(ved, struct vimc_cap_device, ved); >>>>>> media_entity_cleanup(vcap->ved.ent); >>>>>> kfree(vcap); >>>>>> } >>>>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>>> +void vimc_cap_unregister(struct vimc_ent_device *ved) >>>>>> { >>>>>> - struct vimc_cap_device *vcap; >>>>>> + struct vimc_cap_device *vcap = >>>>>> + container_of(ved, struct vimc_cap_device, ved); >>>>>> - vcap = container_of(ved, struct vimc_cap_device, ved); >>>>>> vb2_queue_release(&vcap->queue); >>>>>> video_unregister_device(&vcap->vdev); >>>>>> } >>>>>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>>>> vdev = &vcap->vdev; >>>>>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; >>>>>> vdev->entity.ops = &vimc_cap_mops; >>>>>> - vdev->release = vimc_cap_release; >>>>>> + vdev->release = video_device_release_empty; >>>>>> vdev->fops = &vimc_cap_fops; >>>>>> vdev->ioctl_ops = &vimc_cap_ioctl_ops; >>>>>> vdev->lock = &vcap->lock; >>>>>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c >>>>>> index 16ce9f3b7c75..c95c17c048f2 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-common.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-common.c >>>>>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>>> u32 function, >>>>>> u16 num_pads, >>>>>> struct media_pad *pads, >>>>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>>>> const struct v4l2_subdev_ops *sd_ops) >>>>>> { >>>>>> int ret; >>>>>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>>> /* Initialize the subdev */ >>>>>> v4l2_subdev_init(sd, sd_ops); >>>>>> - sd->internal_ops = sd_int_ops; >>>>>> sd->entity.function = function; >>>>>> sd->entity.ops = &vimc_ent_sd_mops; >>>>>> sd->owner = THIS_MODULE; >>>>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h >>>>>> index 1b6ef7196f3c..616d5a6b0754 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-common.h >>>>>> +++ b/drivers/media/platform/vimc/vimc-common.h >>>>>> @@ -125,16 +125,18 @@ struct vimc_device { >>>>>> * @name entity name >>>>>> * @ved pointer to vimc_ent_device (a node in the >>>>>> * topology) >>>>>> - * @add subdev add hook - initializes and registers >>>>>> - * subdev called from vimc-core >>>>>> - * @rm subdev rm hook - unregisters and frees >>>>>> - * subdev called from vimc-core >>>>>> + * @add initializes and registers >>>>>> + * vim entity - called from vimc-core >>>>>> + * @unregister unregisters vimc entity - called from vimc-core >>>>>> + * @release releases vimc entity - called from the v4l2_dev >>>>>> + * release callback >>>>>> */ >>>>> >>>>> Did you test the suggestion I gave in last patch? >>>>> That instead of adding another callback here, to just remove the release functions of the subdevices, >>>>> and add a check if the release cb isn't null just before calling it. >>>>> With this, the release functions of the video nodes would be called first. >>>>> >>>> I just tested you suggestion, here is the patch: https://gitlab.collabora.com/dafna/linux/commit/f59bc7c6643ce8f88aa33eb5e9f413a1aca99589 >>>> I get a kasan report: >>>> >>>> [ 37.736909] BUG: KASAN: use-after-free in v4l2_device_unregister+0xa7/0xf0 [videodev] >>>> [ 37.738110] Read of size 4 at addr ffff8881eb3f00bc by task bash/227 >>>> ... >>>> >>>> What happen is that the function v4l2_device_unregister loops over the subdevices and unregister them: >>>> >>>> /* Unregister subdevs */ >>>> list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) { >>>> v4l2_device_unregister_subdev(sd); >>>> if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) >>>> v4l2_i2c_subdev_unregister(sd); >>>> else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) >>>> v4l2_spi_subdev_unregister(sd); >>>> } >>>> >>>> when unregistering the first subdevice it also immediately calls the release callback because no one holds a fh for >>>> /dev/v4l2-subdev* , but the release cb releases the memory where the 'sd' in the above loop resides, then when >>>> referencing it in sd->flags it is after it is already freed. >>> >>> Urgh. That's a bug in that function. I think the if...else if bit can be moved to >>> v4l2_subdev_release(). >>> >>> I'm surprised that this hasn't been seen before. >>> I thought that this is not a bug, this happen in vimc only when implementing the release callback >> of subdevices which we agreed should not be implemented, or I am missing something? > > That too, but in general it is just a bad idea to unregister a subdev and keep using the sd. > > I think it makes sense to move this bit to v4l2_subdev_release(). > But v4l2_subdev_release is also called after v4l2_device_unregister, so I don't see the difference. Dafna > Regards, > > Hans > >> >> Dafna >> >>> Regards, >>> >>> Hans >>> >>>> >>>> Dafna >>>> >>>>> I followed the discussion regarding fhs in media node and video, not that I'm against your changes, >>>>> but I would like to understand a bit better why the simple fix suggested isn't enough. >>>>> >>>>> Thanks >>>>> Helen >>>>> >>>>>> struct vimc_ent_config { >>>>>> const char *name; >>>>>> struct vimc_ent_device *(*add)(struct vimc_device *vimc, >>>>>> const char *vcfg_name); >>>>>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>>> + void (*unregister)(struct vimc_ent_device *ved); >>>>>> + void (*release)(struct vimc_ent_device *ved); >>>>>> }; >>>>>> /** >>>>>> @@ -145,22 +147,23 @@ struct vimc_ent_config { >>>>>> */ >>>>>> bool vimc_is_source(struct media_entity *ent); >>>>>> -/* prototypes for vimc_ent_config add and rm hooks */ >>>>>> +/* prototypes for vimc_ent_config hooks */ >>>>>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc, >>>>>> const char *vcfg_name); >>>>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>>> +void vimc_cap_unregister(struct vimc_ent_device *ved); >>>>>> +void vimc_cap_release(struct vimc_ent_device *ved); >>>>>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>>>> const char *vcfg_name); >>>>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>>> +void vimc_deb_release(struct vimc_ent_device *ved); >>>>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>>> const char *vcfg_name); >>>>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>>> +void vimc_sca_release(struct vimc_ent_device *ved); >>>>>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>>>> const char *vcfg_name); >>>>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved); >>>>>> +void vimc_sen_release(struct vimc_ent_device *ved); >>>>>> /** >>>>>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index >>>>>> @@ -195,7 +198,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat); >>>>>> * @num_pads: number of pads to initialize >>>>>> * @pads: the array of pads of the entity, the caller should set the >>>>>> flags of the pads >>>>>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops >>>>>> * @sd_ops: pointer to &struct v4l2_subdev_ops. >>>>>> * >>>>>> * Helper function initialize and register the struct vimc_ent_device and struct >>>>>> @@ -208,7 +210,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved, >>>>>> u32 function, >>>>>> u16 num_pads, >>>>>> struct media_pad *pads, >>>>>> - const struct v4l2_subdev_internal_ops *sd_int_ops, >>>>>> const struct v4l2_subdev_ops *sd_ops); >>>>>> /** >>>>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c >>>>>> index 1c55e0382f09..306f9c293628 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-core.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-core.c >>>>>> @@ -48,48 +48,51 @@ static struct vimc_ent_config ent_config[] = { >>>>>> { >>>>>> .name = "Sensor A", >>>>>> .add = vimc_sen_add, >>>>>> - .rm = vimc_sen_rm, >>>>>> + .release = vimc_sen_release, >>>>>> }, >>>>>> { >>>>>> .name = "Sensor B", >>>>>> .add = vimc_sen_add, >>>>>> - .rm = vimc_sen_rm, >>>>>> + .release = vimc_sen_release, >>>>>> }, >>>>>> { >>>>>> .name = "Debayer A", >>>>>> .add = vimc_deb_add, >>>>>> - .rm = vimc_deb_rm, >>>>>> + .release = vimc_deb_release, >>>>>> }, >>>>>> { >>>>>> .name = "Debayer B", >>>>>> .add = vimc_deb_add, >>>>>> - .rm = vimc_deb_rm, >>>>>> + .release = vimc_deb_release, >>>>>> }, >>>>>> { >>>>>> .name = "Raw Capture 0", >>>>>> .add = vimc_cap_add, >>>>>> - .rm = vimc_cap_rm, >>>>>> + .unregister = vimc_cap_unregister, >>>>>> + .release = vimc_cap_release, >>>>>> }, >>>>>> { >>>>>> .name = "Raw Capture 1", >>>>>> .add = vimc_cap_add, >>>>>> - .rm = vimc_cap_rm, >>>>>> + .unregister = vimc_cap_unregister, >>>>>> + .release = vimc_cap_release, >>>>>> }, >>>>>> { >>>>>> /* TODO: change this to vimc-input when it is implemented */ >>>>>> .name = "RGB/YUV Input", >>>>>> .add = vimc_sen_add, >>>>>> - .rm = vimc_sen_rm, >>>>>> + .release = vimc_sen_release, >>>>>> }, >>>>>> { >>>>>> .name = "Scaler", >>>>>> .add = vimc_sca_add, >>>>>> - .rm = vimc_sca_rm, >>>>>> + .release = vimc_sca_release, >>>>>> }, >>>>>> { >>>>>> .name = "RGB/YUV Capture", >>>>>> .add = vimc_cap_add, >>>>>> - .rm = vimc_cap_rm, >>>>>> + .unregister = vimc_cap_unregister, >>>>>> + .release = vimc_cap_release, >>>>>> }, >>>>>> }; >>>>>> @@ -175,13 +178,33 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>>>>> return 0; >>>>>> } >>>>>> -static void vimc_rm_subdevs(struct vimc_device *vimc) >>>>>> +static void vimc_release_subdevs(struct vimc_device *vimc) >>>>>> { >>>>>> unsigned int i; >>>>>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>>>> if (vimc->ent_devs[i]) >>>>>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]); >>>>>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]); >>>>>> +} >>>>>> + >>>>>> +static void vimc_unregister_subdevs(struct vimc_device *vimc) >>>>>> +{ >>>>>> + unsigned int i; >>>>>> + >>>>>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++) >>>>>> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister) >>>>>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]); >>>>>> +} >>>>>> + >>>>>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>>>>> +{ >>>>>> + struct vimc_device *vimc = >>>>>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>>>>> + >>>>>> + vimc_release_subdevs(vimc); >>>>>> + media_device_cleanup(&vimc->mdev); >>>>>> + kfree(vimc->ent_devs); >>>>>> + kfree(vimc); >>>>>> } >>>>>> static int vimc_register_devices(struct vimc_device *vimc) >>>>>> @@ -195,7 +218,6 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>>> "v4l2 device register failed (err=%d)\n", ret); >>>>>> return ret; >>>>>> } >>>>>> - >>>>>> /* allocate ent_devs */ >>>>>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents, >>>>>> sizeof(*vimc->ent_devs), GFP_KERNEL); >>>>>> @@ -236,9 +258,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>>> err_mdev_unregister: >>>>>> media_device_unregister(&vimc->mdev); >>>>>> - media_device_cleanup(&vimc->mdev); >>>>>> err_rm_subdevs: >>>>>> - vimc_rm_subdevs(vimc); >>>>>> + vimc_unregister_subdevs(vimc); >>>>>> + vimc_release_subdevs(vimc); >>>>>> kfree(vimc->ent_devs); >>>>>> err_v4l2_unregister: >>>>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>>>> @@ -248,11 +270,9 @@ static int vimc_register_devices(struct vimc_device *vimc) >>>>>> static void vimc_unregister(struct vimc_device *vimc) >>>>>> { >>>>>> + vimc_unregister_subdevs(vimc); >>>>>> media_device_unregister(&vimc->mdev); >>>>>> - media_device_cleanup(&vimc->mdev); >>>>>> v4l2_device_unregister(&vimc->v4l2_dev); >>>>>> - kfree(vimc->ent_devs); >>>>>> - kfree(vimc); >>>>>> } >>>>>> static int vimc_probe(struct platform_device *pdev) >>>>>> @@ -285,7 +305,12 @@ static int vimc_probe(struct platform_device *pdev) >>>>>> kfree(vimc); >>>>>> return ret; >>>>>> } >>>>>> + /* >>>>>> + * the release cb is set only after successful registration. >>>>>> + * if the registration fails, we release directly from probe >>>>>> + */ >>>>>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release; >>>>>> platform_set_drvdata(pdev, vimc); >>>>>> return 0; >>>>>> } >>>>>> @@ -296,8 +321,8 @@ static int vimc_remove(struct platform_device *pdev) >>>>>> dev_dbg(&pdev->dev, "remove"); >>>>>> - vimc_rm_subdevs(vimc); >>>>>> vimc_unregister(vimc); >>>>>> + v4l2_device_put(&vimc->v4l2_dev); >>>>>> return 0; >>>>>> } >>>>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c >>>>>> index 34b98b235880..baf6bf9f65b5 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-debayer.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c >>>>>> @@ -494,28 +494,16 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = { >>>>>> .s_ctrl = vimc_deb_s_ctrl, >>>>>> }; >>>>>> -static void vimc_deb_release(struct v4l2_subdev *sd) >>>>>> +void vimc_deb_release(struct vimc_ent_device *ved) >>>>>> { >>>>>> struct vimc_deb_device *vdeb = >>>>>> - container_of(sd, struct vimc_deb_device, sd); >>>>>> + container_of(ved, struct vimc_deb_device, ved); >>>>>> v4l2_ctrl_handler_free(&vdeb->hdl); >>>>>> media_entity_cleanup(vdeb->ved.ent); >>>>>> kfree(vdeb); >>>>>> } >>>>>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = { >>>>>> - .release = vimc_deb_release, >>>>>> -}; >>>>>> - >>>>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>>> -{ >>>>>> - struct vimc_deb_device *vdeb; >>>>>> - >>>>>> - vdeb = container_of(ved, struct vimc_deb_device, ved); >>>>>> - v4l2_device_unregister_subdev(&vdeb->sd); >>>>>> -} >>>>>> - >>>>>> static const struct v4l2_ctrl_config vimc_deb_ctrl_class = { >>>>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>>>> .id = VIMC_CID_VIMC_CLASS, >>>>>> @@ -563,8 +551,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc, >>>>>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev, >>>>>> vcfg_name, >>>>>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2, >>>>>> - vdeb->pads, >>>>>> - &vimc_deb_int_ops, &vimc_deb_ops); >>>>>> + vdeb->pads, &vimc_deb_ops); >>>>>> if (ret) >>>>>> goto err_free_hdl; >>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c >>>>>> index cb2dc24c3e59..7521439747c5 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c >>>>>> @@ -464,27 +464,15 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved, >>>>>> return vsca->src_frame; >>>>>> }; >>>>>> -static void vimc_sca_release(struct v4l2_subdev *sd) >>>>>> +void vimc_sca_release(struct vimc_ent_device *ved) >>>>>> { >>>>>> struct vimc_sca_device *vsca = >>>>>> - container_of(sd, struct vimc_sca_device, sd); >>>>>> + container_of(ved, struct vimc_sca_device, ved); >>>>>> media_entity_cleanup(vsca->ved.ent); >>>>>> kfree(vsca); >>>>>> } >>>>>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = { >>>>>> - .release = vimc_sca_release, >>>>>> -}; >>>>>> - >>>>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>>> -{ >>>>>> - struct vimc_sca_device *vsca; >>>>>> - >>>>>> - vsca = container_of(ved, struct vimc_sca_device, ved); >>>>>> - v4l2_device_unregister_subdev(&vsca->sd); >>>>>> -} >>>>>> - >>>>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>>> const char *vcfg_name) >>>>>> { >>>>>> @@ -504,8 +492,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc, >>>>>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev, >>>>>> vcfg_name, >>>>>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2, >>>>>> - vsca->pads, >>>>>> - &vimc_sca_int_ops, &vimc_sca_ops); >>>>>> + vsca->pads, &vimc_sca_ops); >>>>>> if (ret) { >>>>>> kfree(vsca); >>>>>> return NULL; >>>>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c >>>>>> index 9788fe291193..92daee58209e 100644 >>>>>> --- a/drivers/media/platform/vimc/vimc-sensor.c >>>>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c >>>>>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = { >>>>>> .s_ctrl = vimc_sen_s_ctrl, >>>>>> }; >>>>>> -static void vimc_sen_release(struct v4l2_subdev *sd) >>>>>> +void vimc_sen_release(struct vimc_ent_device *ved) >>>>>> { >>>>>> struct vimc_sen_device *vsen = >>>>>> - container_of(sd, struct vimc_sen_device, sd); >>>>>> + container_of(ved, struct vimc_sen_device, ved); >>>>>> v4l2_ctrl_handler_free(&vsen->hdl); >>>>>> tpg_free(&vsen->tpg); >>>>>> @@ -290,18 +290,6 @@ static void vimc_sen_release(struct v4l2_subdev *sd) >>>>>> kfree(vsen); >>>>>> } >>>>>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = { >>>>>> - .release = vimc_sen_release, >>>>>> -}; >>>>>> - >>>>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved) >>>>>> -{ >>>>>> - struct vimc_sen_device *vsen; >>>>>> - >>>>>> - vsen = container_of(ved, struct vimc_sen_device, ved); >>>>>> - v4l2_device_unregister_subdev(&vsen->sd); >>>>>> -} >>>>>> - >>>>>> /* Image Processing Controls */ >>>>>> static const struct v4l2_ctrl_config vimc_sen_ctrl_class = { >>>>>> .flags = V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY, >>>>>> @@ -365,7 +353,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>>>>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev, >>>>>> vcfg_name, >>>>>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad, >>>>>> - &vimc_sen_int_ops, &vimc_sen_ops); >>>>>> + &vimc_sen_ops); >>>>>> if (ret) >>>>>> goto err_free_tpg; >>>>>> >>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-31 17:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-22 16:01 [PATCH v5 0/3] media: vimc: release vimc in release cb of v4l2_device Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 1/3] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 2/3] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld 2020-01-22 16:01 ` [PATCH v5 3/3] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld 2020-01-22 16:39 ` Helen Koike 2020-01-23 11:36 ` Dafna Hirschfeld 2020-01-24 11:31 ` Hans Verkuil 2020-01-24 12:18 ` Dafna Hirschfeld 2020-01-24 13:21 ` Hans Verkuil 2020-01-31 17:41 ` Dafna Hirschfeld
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).