Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export

From: "Christian König" <christian.koenig@amd.com>
To: cuigaosheng <cuigaosheng1@huawei.com>,
	Charan Teja Kalla <quic_charante@quicinc.com>,
	"T.J. Mercier" <tjmercier@google.com>
Cc: sumit.semwal@linaro.org, Dan Carpenter <dan.carpenter@oracle.com>,
	Pavan Kondeti <quic_pkondeti@quicinc.com>,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org
Subject: Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export
Date: Thu, 24 Nov 2022 13:37:41 +0100	[thread overview]
Message-ID: <689ad89b-0e34-3743-24c9-3f92856720f8@amd.com> (raw)
In-Reply-To: <b2d5d904-99f2-7974-a22d-63a6cf864973@huawei.com>

Am 24.11.22 um 13:05 schrieb cuigaosheng:
> Some tips:
>     Before we call the dma_buf_stats_setup(), we have to finish 
> creating the file,
> otherwise dma_buf_stats_setup() will return -EINVAL, maybe we need to 
> think about
> this when making a new patch.

I was already wondering why the order is this way.

Why is dma_buf_stats_setup() needing the file in the first place?

Thanks,
Christian.

>
> Hope these tips are useful, thanks!
>
> On 2022/11/24 13:56, Charan Teja Kalla wrote:
>> Thanks T.J and Christian for the inputs.
>>
>> On 11/19/2022 7:00 PM, Christian König wrote:
>>>>      Yes, exactly that's the idea.
>>>>
>>>>      The only alternatives I can see would be to either move 
>>>> allocating
>>>>      the
>>>>      file and so completing the dma_buf initialization last again 
>>>> or just
>>>>      ignore errors from sysfs.
>>>>
>>>>      > If we still want to avoid calling 
>>>> dmabuf->ops->release(dmabuf) in
>>>>      > dma_buf_release like the comment says I guess we could use
>>>>      sysfs_entry
>>>>      > and ERR_PTR to flag that, otherwise it looks like we'd need 
>>>> a bit
>>>>      > somewhere.
>>>>
>>>>      No, this should be dropped as far as I can see. The sysfs cleanup
>>>>      code
>>>>      looks like it can handle not initialized kobj pointers.
>>>>
>>>>
>>>> Yeah there is also the null check in dma_buf_stats_teardown() that
>>>> would prevent it from running, but I understood the comment to be
>>>> referring to the release() dma_buf_ops call into the exporter which
>>>> comes right after the teardown call. That looks like it's preventing
>>>> the fput task work calling back into the exporter after the exporter
>>>> already got an error from dma_buf_export(). Otherwise the exporter
>>>> sees a release() for a buffer that it doesn't know about / thinks
>>>> shouldn't exist. So I could imagine an exporter trying to double free:
>>>> once for the failed dma_buf_export() call, and again when the
>>>> release() op is called later.
>>>
>>> Oh, very good point as well. Yeah, then creating the file should
>>> probably come last.
>>>
>> @Gaosheng: Could you please make these changes or you let me to do?
>>
>>> Regards,
>>> Christian.
>> .