Received-SPF: pass (google.com: domain of linux-kernel+bounces-169571-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <83605228-92ee-b666-d894-1c145af2e5ab@quicinc.com>
Date: Mon, 6 May 2024 15:00:34 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue
Content-Language: en-US
From: Charan Teja Kalla <quic_charante@quicinc.com>
To: "T.J. Mercier" <tjmercier@google.com>
CC: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        zhiguojiang
	<justinjiang@vivo.com>,
        Sumit Semwal <sumit.semwal@linaro.org>, <linux-media@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <linaro-mm-sig@lists.linaro.org>,
        <linux-kernel@vger.kernel.org>, <opensource.kernel@vivo.com>
References: <20240327022903.776-1-justinjiang@vivo.com>
 <5cf29162-a29d-4af7-b68e-aac5c862d20e@amd.com>
 <cc7defae-60c1-4cc8-aee5-475d4460e574@vivo.com>
 <23375ba8-9558-4886-9c65-af9fe8e8e8b6@amd.com>
 <CABdmKX2Kf4ZmVzv3LGTz2GyP-9+rAtFY9hSAxdkrwK8mG0gDvQ@mail.gmail.com>
 <e55cad9b-a361-4d27-a351-f6a4f5b8b734@vivo.com>
 <40ac02bb-efe2-4f52-a4f2-7b56d9b93d2c@amd.com>
 <4fedd80c-d5b6-4478-bfd3-02d1ee1a26e5@vivo.com>
 <aab5ec51-fcff-44f2-a4f5-2979bd776a03@amd.com>
 <2ebca2fd-9465-4e64-b3cc-ffb88ef87800@vivo.com>
 <d4209754-5f26-422d-aca0-45cccbc44ad0@amd.com>
 <289b9ad6-58a3-aa39-48ae-a244fe108354@quicinc.com>
 <CABdmKX3Zu8LihAFjMuUHx4xzZoqgmY7OKdyVz-D26gM-LECn6A@mail.gmail.com>
 <8ca45837-cbed-28da-4a6f-0dcec8294f51@quicinc.com>
In-Reply-To: <8ca45837-cbed-28da-4a6f-0dcec8294f51@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

Hi TJ,

Seems I have got answers from [1], where it is agreed upon epoll() is
the source of issue.

Thanks a lot for the discussion.

[1] https://lore.kernel.org/lkml/0000000000002d631f0615918f1e@google.com/

Thanks
Charan

On 5/5/2024 9:50 PM, Charan Teja Kalla wrote:
> Thanks T.J for the reply!!
> 
> On 5/4/2024 4:43 AM, T.J. Mercier wrote:
>> It looks like a similar conclusion about epoll was reached at:
>> https://lore.kernel.org/all/a87d7ef8-2c59-4dc5-ba0a-b821d1effc72@amd.com/
>>
> I am unaware of this discussion. Thanks...
> 
>> I agree with Christian that it should not be possible for the file to
>> be freed while inside dma_buf_poll. Aside from causing problems in
>> dma_buf_poll, ep_item_poll itself would have issues dereferencing the
>> freed file pointer.
>>
> Not sure about my understanding: ep_item_poll() always call the ->poll()
> interface with a stable 'struct file' because of ep->mtx. This lock
> ensures that:
>    a) If eventpoll_release_file() get the ep->mtx first, ->poll()
> corresponds to the epitem(target file) will never be called, because it
> is removed from the rdlist.
> 
>    b) If ep_send_events() get the ep->mtx() first, ->poll() will get
> called with a stable 'struct file', __but the refcount(->f_count) of a
> file can be zero__. I am saying that this is stable because the 'struct
> file' contents are still valid till we are in ->poll().
> 
> Can you/Christian help me with what I am missing here to say that
> ->poll() is receiving stale 'struct file*', please?
> 
> And, If you are convinced with above, I think, It should have been the
> responsibility of ->poll() implementation to have taken refcount on a
> file that is going to be still valid even after ->poll() exits. Incase
> of dma_buf_poll() implementation, it took the refcount on a file that is
> not going to be valid once the dma_buf_poll() exits(because of mentioned
> race with the freeing of the 'struct file*').
> 
> So, in dma_buf_poll(), Should we be using atomic_long_inc_not_zero()
> based implementation to take the refcount on a file?
> 
> Thanks,
> Charan