Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <ab509035-cf3c-4902-87a8-cbba03ac948c@quicinc.com>
Date:   Tue, 31 Oct 2023 16:29:43 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Devcoredump: fix use-after-free issue when releasing
 devcd device
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        Greg KH <gregkh@linuxfoundation.org>
CC:     <rafael@kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel@quicinc.com>
References: <20231027055521.2679-1-quic_yyuwang@quicinc.com>
 <2023102730-twins-thieving-d04e@gregkh>
 <22ab53d1ae36d4925732e6e1dc989dc75af126da.camel@sipsolutions.net>
From:   Yu Wang <quic_yyuwang@quicinc.com>
In-Reply-To: <22ab53d1ae36d4925732e6e1dc989dc75af126da.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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