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

[Yu Wang]

On 10/27/2023 7:12 PM, Greg KH wrote:
> On Thu, Oct 26, 2023 at 10:55:21PM -0700, Yu Wang wrote:
>> With sample code as below, it may hit use-after-free issue when
>> releasing devcd device.
>>
>>     struct my_coredump_state {
>>         struct completion dump_done;
>>         ...
>>     };
>>
>>     static void my_coredump_free(void *data)
>>     {
>>         struct my_coredump_state *dump_state = data;
>>         ...
>>         complete(&dump_state->dump_done);
>>     }
>>
>>     static void my_dev_release(struct device *dev)
>>     {
>>         kfree(dev);
>>     }
>>
>>     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);
>>     }
>>
>> In devcoredump framework, devcd_dev_release() will be called when
>> releasing the devcd device, it will call the free() callback first
>> and try to delete the symlink in sysfs directory of the failing device.
>> Eventhough it has checked 'devcd->failing_dev->kobj.sd' before that,
>> there is no mechanism to ensure it's still available when accessing
>> it in kernfs_find_ns(), refer to the diagram as below:
>>
>>     Thread A was waiting for 'dump_state.dump_done' at #A-1-2 after
>>     calling dev_coredumpm().
>>     When thread B calling devcd->free() at #B-2-1, it wakes up
>>     thread A from point #A-1-2, which will call device_del() to
>>     delete the device.
>>     If #B-2-2 comes before #A-3-1, but #B-4 comes after #A-4, it
>>     will hit use-after-free issue when trying to access
>>     'devcd->failing_dev->kobj.sd'.
>>
>>     #A-1-1: dev_coredumpm()
>>       #A-1-2: wait_for_completion(&dump_state.dump_done)
>>       #A-1-3: device_del()
>>         #A-2: kobject_del()
>>           #A-3-1: sysfs_remove_dir() --> set kobj->sd=NULL
>>           #A-3-2: kernfs_put()
>>             #A-4: kmem_cache_free() --> free kobj->sd
>>
>>     #B-1: devcd_dev_release()
>>       #B-2-1: devcd->free(devcd->data)
>>       #B-2-2: check devcd->failing_dev->kobj.sd
>>       #B-2-3: sysfs_delete_link()
>>         #B-3: kernfs_remove_by_name_ns()
>>           #B-4: kernfs_find_ns() --> access devcd->failing_dev->kobj.sd
>>
>> To fix this issue, put operations on devcd->failing_dev before
>> calling the free() callback in devcd_dev_release().
>>
>> Signed-off-by: Yu Wang <quic_yyuwang@xxxxxxxxxxx>
>> ---
>>  drivers/base/devcoredump.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Also, what commit id does this fix?

Thanks for your comment :)
Do you mean the commit which introduced this issue? It's from initial version of devcoredump.c.

> 
> thanks,
> 
> greg k-h