[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <867877cc-ea3d-31e9-97ba-0764e1f05b9e@quicinc.com>
Date: Sat, 28 Oct 2023 14:50:44 +0530
From: Mukesh Ojha <quic_mojha@...cinc.com>
To: Yu Wang <quic_yyuwang@...cinc.com>, <johannes@...solutions.net>,
<gregkh@...uxfoundation.org>, <rafael@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <kernel@...cinc.com>
Subject: Re: [PATCH] Devcoredump: fix use-after-free issue when releasing
devcd device
On 10/27/2023 11:52 AM, Mukesh Ojha wrote:
>
>
> On 10/27/2023 11:25 AM, 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@...cinc.com>
>
> Awesome explanation.
But it is still not solving the original race like device can go
anytime., what if caller of dev_coredumpm() is not waiting and
called device_del() right during this devcd_dev_release() where,
this race can still happen.
Although, dev_coredumpm() does keep reference to the device
devcd->failing_dev = get_device(dev);
Does it also need to keep reference to
kernfs_get(devcd->failing_dev->kobj.sd) inside dev_coredumpm() itself
and release reference kernfs_put(devcd->failing_dev->kobj.sd) after
sysfs_delete_link() to avoid this issue completely.
thoughts ?
>
> Reviewed-by: Mukesh Ojha <quic_mojha@...cinc.com>
Ignore this for now
-Mukesh
>
> -Mukesh
>
>> ---
>> drivers/base/devcoredump.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
>> index 91536ee05f14..35c704ddfeae 100644
>> --- a/drivers/base/devcoredump.c
>> +++ b/drivers/base/devcoredump.c
>> @@ -83,9 +83,6 @@ static void devcd_dev_release(struct device *dev)
>> {
>> struct devcd_entry *devcd = dev_to_devcd(dev);
>> - devcd->free(devcd->data);
>> - module_put(devcd->owner);
>> -
>> /*
>> * this seems racy, but I don't see a notifier or such on
>> * a struct device to know when it goes away?
>> @@ -95,6 +92,8 @@ static void devcd_dev_release(struct device *dev)
>> "devcoredump");
>> put_device(devcd->failing_dev);
>> + devcd->free(devcd->data);
>> + module_put(devcd->owner);
>> kfree(devcd);
>> }
Powered by blists - more mailing lists