From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from pandora.armlinux.org.uk ([78.32.30.218]:51696 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752255AbdI2Vdp (ORCPT ); Fri, 29 Sep 2017 17:33:45 -0400 From: Russell King To: Mauro Carvalho Chehab Cc: linux-media@vger.kernel.org Subject: [PATCH RFC] [media] v4l: async: don't bomb out on ->complete failure MIME-Version: 1.0 Content-Disposition: inline Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" Message-Id: Date: Fri, 29 Sep 2017 22:33:36 +0100 Sender: linux-media-owner@vger.kernel.org List-ID: When a subdev is being registered via v4l2_async_register_subdev(), we test to see if we have all the components, and if so, we call the ->complete() handler. However, the error handling is broken - if notifier->complete() returns an error, we propagate the error out of v4l2_async_register_subdev(), but leaving the subdev bound and on the notifier's "done" list. Drivers calling v4l2_async_register_subdev() assume that this means that v4l2_async_register_subdev() failed, which causes probe failure and the memory backing the subdev to be freed. At this point, we now have a subdev on the notifier's done list which has been freed. If we then try to remove the notifier via v4l2_async_notifier_unregister(), we walk the notifier's done list, cleaning up the subdevs - and hit the freed subdev, causing a kernel oops such as: Unable to handle kernel paging request at virtual address 6e6f6994 pgd = d039c000 [6e6f6994] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: mux_mmio caam_jr snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card snd_soc_imx_spdif imx_media_csi(C) ci_hdrc_imx snd_soc_imx_audmux imx6_mipi_csi2(C) imx219 snd_soc_sgtl5000 ci_hdrc usbmisc_imx udc_core caam video_mux mux_core imx_media_ic(C) imx_sdma imx_media_vdic(C) imx_media_capture(C) imx2_wdt snd_soc_fsl_ssi snd_soc_fsl_spdif imx_pcm_dma coda v4l2_mem2mem videobuf2_v4l2 imx_vdoa videobuf2_dma_contig videobuf2_core imx_thermal videobuf2_vmalloc videobuf2_memops imx_media(C-) imx_media_common(C) v4l2_fwnode nfsd rc_pinnacle_pctv_hd dw_hdmi_ahb_audio dw_hdmi_cec etnaviv CPU: 1 PID: 8039 Comm: rmmod Tainted: G C 4.14.0-rc1+ #2194 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) task: ee522700 task.stack: e5f68000 PC is at __lock_acquire+0x98/0x13f4 LR is at lock_acquire+0xd8/0x250 pc : [] lr : [] psr: 200e0093 sp : e5f69d28 ip : e5f68000 fp : e5f69da4 r10: 00000000 r9 : 00000000 r8 : 6e6f6994 r7 : 00000000 r6 : c0aa4f64 r5 : ee522700 r4 : c13f4abc r3 : 00000000 r2 : 00000001 r1 : 00000000 r0 : 6e6f6994 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 2039c04a DAC: 00000051 Process rmmod (pid: 8039, stack limit = 0xe5f68210) Backtrace: [] (__lock_acquire) from [] (lock_acquire+0xd8/0x250) [] (lock_acquire) from [] (_raw_spin_lock+0x34/0x44) [] (_raw_spin_lock) from [] (v4l2_device_unregister_subdev+0x2c/0xb0) [] (v4l2_device_unregister_subdev) from [] (v4l2_async_cleanup+0x14/0x40) [] (v4l2_async_cleanup) from [] (v4l2_async_notifier_unregister+0xa8/0x1ec) [] (v4l2_async_notifier_unregister) from [] (imx_media_remove+0x2c/0x54 [imx_media]) [] (imx_media_remove [imx_media]) from [] (platform_drv_remove+0x2c/0x44) [] (platform_drv_remove) from [] (device_release_driver_internal+0x158/0x1f8) [] (device_release_driver_internal) from [] (driver_detach+0x40/0x74) [] (driver_detach) from [] (bus_remove_driver+0x54/0x98) [] (bus_remove_driver) from [] (driver_unregister+0x30/0x50) [] (driver_unregister) from [] (platform_driver_unregister+0x14/0x18) [] (platform_driver_unregister) from [] (imx_media_pdrv_exit+0x14/0x1c [imx_media]) [] (imx_media_pdrv_exit [imx_media]) from [] (SyS_delete_module+0x170/0x1c0) [] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x1c) Code: 1a00000e e3a00000 e24bd028 e89daff0 (e5980000) ---[ end trace 97732329ac63e5ae ]--- Avoid this by reporting an error to the kernel message log about the failure, rather than silently propagating the error from ->complete() and causing this latent use-after-free oops. Signed-off-by: Russell King --- drivers/media/v4l2-core/v4l2-async.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index d741a8e0fdac..8d221f4a8a8d 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -122,8 +122,12 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, /* Move from the global subdevice list to notifier's done */ list_move(&sd->async_list, ¬ifier->done); - if (list_empty(¬ifier->waiting) && notifier->complete) - return notifier->complete(notifier); + if (list_empty(¬ifier->waiting) && notifier->complete) { + int ret = notifier->complete(notifier); + if (ret) + dev_err(notifier->v4l2_dev->dev, + "complete notifier failed: %d\n", ret); + } return 0; } -- 2.7.4