Messages in this thread | | | Date | Tue, 31 Oct 2023 16:29:43 +0800 | Subject | Re: [PATCH] Devcoredump: fix use-after-free issue when releasing devcd device | From | Yu Wang <> |
| |
Thanks for your comments :)
On 10/27/2023 8:45 PM, Johannes Berg wrote: > On Fri, 2023-10-27 at 13:11 +0200, Greg KH wrote: >>> >>> static void my_coredump() >>> { >>> struct my_coredump_state dump_state; >>> struct device *new_device = >>> kzalloc(sizeof(*new_device), GFP_KERNEL); >>> >>> ... >>> new_device->release = my_dev_release; >>> device_initialize(new_device); >>> ... >>> device_add(new_device); >>> ... >>> init_completion(&dump_state.dump_done); >>> dev_coredumpm(new_device, NULL, &dump_state, datalen, GFP_KERNEL, >>> my_coredump_read, my_coredump_free); >>> wait_for_completion(&dump_state.dump_done); >>> device_del(new_device); >>> put_device(new_device); >>> } >> >> Is there any in-kernel user like this? If so, why not fix them up to >> not do this?
In this case, the device is temporarily added for dump only, so we need to delete it when dump is completed. The other users doesn't add/delete the device like this.
>> > > Maybe this is only a simplified scenario and whenever you remove a > device when a coredump is still pending this can happen?
It removes the device when the @free function has been called, I think the @free function should be considered as a completion signal, and so we need to put @free at the end of falling-device-related-clean-up in devcoredump framework (the change in the patch).
> > Actually, no, wait, what is this doing??? Why is there a completion and > all that stuff there? You shouldn't really care about the dump once you > have created it, and not pass NULL for the struct module pointer > either?!
Yes, I know we don't need to care about the dump data, but as mentioned upon, the device is locally and temporarily created for this one-time dump only, we need to free it when dump is completed, so introduce this completion. Refer to drivers/remoteproc/remoteproc_coredump.c.
Regarding NULL for the struct module pointer, looks it's allowed for this API (<remoteproc_coredump.c> also pass NULL)? But yes, it's not nice indeed, we need this to get a reference of the calling module for safety. Will correct in the next patch set.
> > johannes
Best Regards, Yu
| |