Messages in this thread | | | From | Tzung-Bi Shih <> | Date | Tue, 9 Nov 2021 14:49:34 +0800 | Subject | Re: [PATCH v2] platform/chrome: cros_ec_debugfs: detach log reader wq from devm |
| |
Hi Benson and Guenter,
On Mon, Nov 1, 2021 at 4:36 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > Debugfs console_log uses devm memory (e.g. debug_info in > cros_ec_console_log_poll()). However, lifecycles of device and debugfs > are independent. An use-after-free issue is observed if userland > program operates the debugfs after the memory has been freed. > > The call trace: > do_raw_spin_lock > _raw_spin_lock_irqsave > remove_wait_queue > ep_unregister_pollwait > ep_remove > do_epoll_ctl > > A Python example to reproduce the issue: > ... import select > ... p = select.epoll() > ... f = open('/sys/kernel/debug/cros_scp/console_log') > ... p.register(f, select.POLLIN) > ... p.poll(1) > [(4, 1)] # 4=fd, 1=select.POLLIN > > [ shutdown cros_scp at the point ] > > ... p.poll(1) > [(4, 16)] # 4=fd, 16=select.POLLHUP > ... p.unregister(f) > > An use-after-free issue raises here. It called epoll_ctl with > EPOLL_CTL_DEL which in turn to use the workqueue in the devm (i.e. > log_wq). > > Detaches log reader's workqueue from devm to make sure it is persistent > even if the device has been removed. > > Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> > --- > As for 2 other related cases I could image: > > Case 1. userland program opens the debugfs after the device has been removed > > ENOENT. cros_ec_debugfs_remove() calls debugfs_remove_recursive(). > > Case 2. userland program is reading when the device is removing > > If the userland program is waiting in cros_ec_console_log_read(), the device > removal will wait for it. > > See the calling stack for the case: > wait_for_completion > __debugfs_file_removed > remove_one > simple_recursive_removal > debugfs_remove > cros_ec_debugfs_remove > platform_drv_remove > device_release_driver_internal > device_release_driver > bus_remove_device > device_del > platform_device_del > platform_device_unregister
Would you mind to share your thoughts on the patch?
| |