From: "T.J. Mercier" <tjmercier@google.com>
To: Charan Teja Kalla <quic_charante@quicinc.com>
Cc: "Christian König" <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
Subject: Re: [PATCH] dmabuf: fix dmabuf file poll uaf issue
Date: Fri, 3 May 2024 16:13:44 -0700 [thread overview]
Message-ID: <CABdmKX3Zu8LihAFjMuUHx4xzZoqgmY7OKdyVz-D26gM-LECn6A@mail.gmail.com> (raw)
In-Reply-To: <289b9ad6-58a3-aa39-48ae-a244fe108354@quicinc.com>
On Fri, May 3, 2024 at 6:40 AM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> 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
Hi Charan,
It looks like a similar conclusion about epoll was reached at:
https://lore.kernel.org/all/a87d7ef8-2c59-4dc5-ba0a-b821d1effc72@amd.com/
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.
Christian's patch there makes me wonder about what the epoll man page
says about closing files.
"Will closing a file descriptor cause it to be removed from all epoll
interest lists?" Answer: Yes
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man/man7/epoll.7#n442
It looks like eventpoll_release_file is responsible for that, but if
the epitem is changed to hold a reference then that can't be true
anymore because __fput will never call eventpoll_release_file (until
EPOLL_CTL_DEL). But how do you call EPOLL_CTL_DEL if you've closed all
your references to the file in userspace? So I think epoll needs a
slightly more complicated fix.
next prev parent reply other threads:[~2024-05-03 23:13 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-27 2:29 [PATCH] dmabuf: fix dmabuf file poll uaf issue Zhiguo Jiang
2024-03-29 23:36 ` T.J. Mercier
2024-04-01 6:52 ` zhiguojiang
2024-04-01 12:22 ` Christian König
2024-04-02 6:49 ` zhiguojiang
2024-04-02 8:07 ` Christian König
2024-04-02 18:22 ` T.J. Mercier
2024-04-12 6:19 ` zhiguojiang
2024-04-12 6:39 ` Christian König
2024-04-15 10:35 ` zhiguojiang
2024-04-15 11:57 ` Christian König
2024-04-18 1:33 ` zhiguojiang
2024-04-18 6:46 ` Christian König
2024-05-03 13:40 ` Charan Teja Kalla
2024-05-03 23:13 ` T.J. Mercier [this message]
2024-05-05 16:20 ` Charan Teja Kalla
2024-05-06 9:30 ` Charan Teja Kalla
2024-05-06 19:04 ` T.J. Mercier
2024-05-07 10:10 ` Christian König
2024-05-07 13:39 ` Daniel Vetter
2024-05-07 14:04 ` Christian König
2024-05-07 18:00 ` T.J. Mercier
2024-05-07 20:19 ` Rob Clark
2024-05-08 11:51 ` David Laight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABdmKX3Zu8LihAFjMuUHx4xzZoqgmY7OKdyVz-D26gM-LECn6A@mail.gmail.com \
--to=tjmercier@google.com \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=justinjiang@vivo.com \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=opensource.kernel@vivo.com \
--cc=quic_charante@quicinc.com \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).