linux-kernel - Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue

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]

Date: Sun, 5 May 2024 21:50:15 +0530
From: Charan Teja Kalla <quic_charante@...cinc.com>
To: "T.J. Mercier" <tjmercier@...gle.com>
CC: Christian König <christian.koenig@....com>,
        zhiguojiang
	<justinjiang@...o.com>,
        Sumit Semwal <sumit.semwal@...aro.org>, <linux-media@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <linaro-mm-sig@...ts.linaro.org>,
        <linux-kernel@...r.kernel.org>, <opensource.kernel@...o.com>
Subject: Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue

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