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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab509035-cf3c-4902-87a8-cbba03ac948c@quicinc.com>
Date:   Tue, 31 Oct 2023 16:29:43 +0800
From:   Yu Wang <quic_yyuwang@...cinc.com>
To:     Johannes Berg <johannes@...solutions.net>,
        Greg KH <gregkh@...uxfoundation.org>
CC:     <rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...cinc.com>
Subject: Re: [PATCH] Devcoredump: fix use-after-free issue when releasing
 devcd device

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