Received-SPF: pass (google.com: domain of linux-kernel+bounces-167715-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <289b9ad6-58a3-aa39-48ae-a244fe108354@quicinc.com>
Date: Fri, 3 May 2024 19:10:18 +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
To: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        zhiguojiang
	<justinjiang@vivo.com>,
        "T.J. Mercier" <tjmercier@google.com>
CC: 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>
Content-Language: en-US
From: Charan Teja Kalla <quic_charante@quicinc.com>
In-Reply-To: <d4209754-5f26-422d-aca0-45cccbc44ad0@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

Thanks Christian/TJ for the inputs!!

On 4/18/2024 12:16 PM, Christian König wrote:
> As far as I can see the EPOLL holds a reference to the files it
> contains. So it is perfectly valid to add the file descriptor to EPOLL
> and then close it.
>
> In this case the file won't be closed, but be kept alive by it's
> reference in the EPOLL file descriptor.

I am not seeing that adding a 'fd' into the epoll monitoring list will
increase its refcount.  Confirmed by writing a testcase that just do
open + EPOLL_CTL_ADD and see the /proc/../fdinfo of epoll fd(Added
file_count() info to the output)
# cat /proc/136/fdinfo/3
pos:    0
flags:  02
mnt_id: 14
ino:    1041
tfd:        4 events:       19 data:                4  pos:0 ino:81 sdev:5
refcount: 1<-- The fd added to the epoll monitoring is still 1(same as
open file refcount)

From the code too, I don't see a file added in the epoll monitoring list
will have an extra refcount but momentarily (where it increases the
refcount of target file, add it to the rbtree of the epoll context and
then decreasing the file refcount):
do_epoll_ctl():
	f = fdget(epfd);
	tf = fdget(fd);
	epoll_mutex_lock(&ep->mtx)
	EPOLL_CTL_ADD:
		ep_insert(ep, epds, tf.file, fd, full_check); // Added to the epoll
monitoring rb tree list as epitem.
	mutex_unlock(&ep->mtx);
	fdput(tf);
	fdput(f);


Not sure If i am completely mistaken what you're saying here.

> The fs layer which calls dma_buf_poll() should make sure that the file
> can't go away until the function returns.
> 
> Then inside dma_buf_poll() we add another reference to the file while
> installing the callback:
> 
>                         /* Paired with fput in dma_buf_poll_cb */
>                         get_file(dmabuf->file); No, exactly that can't
> happen either.
> 

I am not quite comfortable with epoll internals but I think below race
is possible where "The 'file' passed to dma_buf_poll() is proper but
->f_count == 0, which is indicating that a parallel freeing is
happening". So, we should check the file->f_count value before taking
the refcount.

(A 'fd' registered for the epoll monitoring list is maintained as
'epitem(epi)' in the rbtree of 'eventpoll(ep, called as epoll context).

epoll_wait()				    __fput()(as file->f_count=0)
------------------------------------------------------------------------
a) ep_poll_callback():
     Is the waitqueue function
   called when signaled on the
   wait_queue_head_t of the registered
   poll() function.

   1) It links the 'epi' to the ready list
      of 'ep':
       if (!ep_is_linked(epi))
	 list_add_tail_lockless(&epi->rdllink,
		&ep->rdllist)

   2) wake_up(&ep->wq);
	Wake up the process waiting
	on epoll_wait() that endup
	in ep_poll.


b) ep_poll():
    1) while (1) {
	eavail = ep_events_available(ep);
	(checks ep->rdlist)
	ep_send_events(ep);
	(notify the events to user)
    }
    (epoll_wait() calling process gets
     woken up from a.2 and process the
     events raised added to rdlist in a.1)

   2) ep_send_events():
	mutex_lock(&ep->mtx);
	(** The sync lock is taken **)
	list_for_each_entry_safe(epi, tmp,
			&txlist, rdllink) {
	    list_del_init(&epi->rdllink);
	    revents = ep_item_poll(epi, &pt, 1)
	    (vfs_poll()-->...f_op->poll(=dma_buf_poll)
	}
	mutex_unlock(&ep->mtx)
	(**release the lock.**)

	(As part of procession of events,
	 each epitem is removed from rdlist
         and call the f_op->poll() of a file
	 which will endup in dma_buf_poll())

   3) dma_buf_poll():
 	get_file(dmabuf->file);
	(** f_count changed from 0->1 **)
	dma_buf_poll_add_cb(resv, true, dcb):
	(registers dma_buf_poll_cb() against fence)


				c) eventpoll_release_file():
				   mutex_lock(&ep->mtx);
				   __ep_remove(ep, epi, true):
				   mutex_unlock(&ep->mtx);
				  (__ep_remove() will remove the
				   'epi' from rbtree and if present
				   from rdlist as well)

				d) file_free(file), free the 'file'.

e) dma_buf_poll_cb:
 /* Paired with get_file in dma_buf_poll */
 fput(dmabuf->file);
 (f_count changed from 1->0, where
  we try to free the 'file' again
  which is UAF/double free).


In the above race, If c) gets called first, then the 'epi' is removed
from both rbtree and 'rdlink' under ep->mtx lock thus b.2 don't end up
in calling the ->poll() as it don't see this event in the rdlist.

Race only exist If b.2 executes first, where it will call dma_buf_poll
with __valid 'struct file' under ep->mtx but its refcount is already
could have been zero__. Later When e) is executed, it turns into double
free of the 'file' structure.

If you're convinced with the above race, should the fix here will be
this simple check:
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 8fe5aa67b167..e469dd9288cc
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -240,6 +240,10 @@ static __poll_t dma_buf_poll(struct file *file,
poll_table *poll)
 	struct dma_resv *resv;
 	__poll_t events;

+	/* Check parallel freeing of file */
+	if (!file_count(file))
+		return 0;
+

Thanks,
Charan