Re: [PATCH] Devcoredump: fix use-after-free issue when releasing devcd device

[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