lkml.org 
[lkml]   [2024]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] dmabuf: fix dmabuf file poll uaf issue
From
Am 27.03.24 um 03:29 schrieb Zhiguo Jiang:
> The issue is a UAF issue of dmabuf file fd. Throght debugging, we found
> that the dmabuf file fd is added to the epoll event listener list, and
> when it is released, it is not removed from the epoll list, which leads
> to the UAF(Use-After-Free) issue.

As far as I can see that's not because of the DMA-buf code, but because
you are somehow using this interface incorrectly.

When dma_buf_poll() is called it is mandatory for the caller to hold a
reference to the file descriptor on which the poll operation is executed.

So adding code like "if (!file_count(file))" in the beginning of
dma_buf_poll() is certainly broken.

My best guess is that you have some unbalanced
dma_buf_get()/dma_buf_put() somewhere instead.

Regards,
Christian.

>
> The UAF issue can be solved by checking dmabuf file->f_count value and
> skipping the poll operation for the closed dmabuf file in the
> dma_buf_poll(). We have tested this solved patch multiple times and
> have not reproduced the uaf issue.
>
> crash dump:
> list_del corruption, ffffff8a6f143a90->next is LIST_POISON1
> (dead000000000100)
> ------------[ cut here ]------------
> kernel BUG at lib/list_debug.c:55!
> Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> pc : __list_del_entry_valid+0x98/0xd4
> lr : __list_del_entry_valid+0x98/0xd4
> sp : ffffffc01d413d00
> x29: ffffffc01d413d00 x28: 00000000000000c0 x27: 0000000000000020
> x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000080007
> x23: ffffff8b22e5dcc0 x22: ffffff88a6be12d0 x21: ffffff8b22e572b0
> x20: ffffff80254ed0a0 x19: ffffff8a6f143a00 x18: ffffffda5efed3c0
> x17: 6165642820314e4f x16: 53494f505f545349 x15: 4c20736920747865
> x14: 6e3e2d3039613334 x13: 2930303130303030 x12: 0000000000000018
> x11: ffffff8b6c188000 x10: 00000000ffffffff x9 : 6c8413a194897b00
> x8 : 6c8413a194897b00 x7 : 74707572726f6320 x6 : 6c65645f7473696c
> x5 : ffffff8b6c3b2a3e x4 : ffffff8b6c3b2a40 x3 : ffff103000001005
> x2 : 0000000000000001 x1 : 00000000000000c0 x0 : 000000000000004e
> Call trace:
> __list_del_entry_valid+0x98/0xd4
> dma_buf_file_release+0x48/0x90
> __fput+0xf4/0x280
> ____fput+0x10/0x20
> task_work_run+0xcc/0xf4
> do_notify_resume+0x2a0/0x33c
> el0_svc+0x5c/0xa4
> el0t_64_sync_handler+0x68/0xb4
> el0t_64_sync+0x1a0/0x1a4
> Code: d0006fe0 912c5000 f2fbd5a2 94231a01 (d4210000)
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Oops - BUG: Fatal exception
> SMP: stopping secondary CPUs
>
> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com>
> ---
> drivers/dma-buf/dma-buf.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
> mode change 100644 => 100755 drivers/dma-buf/dma-buf.c
>
> 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 if the file exists */
> + if (!file_count(file))
> + return EPOLLERR;
> +
> dmabuf = file->private_data;
> if (!dmabuf || !dmabuf->resv)
> return EPOLLERR;
> @@ -266,8 +270,15 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> spin_unlock_irq(&dmabuf->poll.lock);
>
> if (events & EPOLLOUT) {
> - /* Paired with fput in dma_buf_poll_cb */
> - get_file(dmabuf->file);
> + /*
> + * Paired with fput in dma_buf_poll_cb,
> + * Skip poll for the closed file.
> + */
> + if (!get_file_rcu(&dmabuf->file)) {
> + events &= ~EPOLLOUT;
> + dcb->active = 0;
> + goto clear_out_event;
> + }
>
> if (!dma_buf_poll_add_cb(resv, true, dcb))
> /* No callback queued, wake up any other waiters */
> @@ -277,6 +288,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> }
> }
>
> +clear_out_event:
> if (events & EPOLLIN) {
> struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in;
>
> @@ -289,8 +301,15 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> spin_unlock_irq(&dmabuf->poll.lock);
>
> if (events & EPOLLIN) {
> - /* Paired with fput in dma_buf_poll_cb */
> - get_file(dmabuf->file);
> + /*
> + * Paired with fput in dma_buf_poll_cb,
> + * Skip poll for the closed file.
> + */
> + if (!get_file_rcu(&dmabuf->file)) {
> + events &= ~EPOLLIN;
> + dcb->active = 0;
> + goto clear_in_event;
> + }
>
> if (!dma_buf_poll_add_cb(resv, false, dcb))
> /* No callback queued, wake up any other waiters */
> @@ -300,6 +319,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> }
> }
>
> +clear_in_event:
> dma_resv_unlock(resv);
> return events;
> }


\
 
 \ /
  Last update: 2024-05-27 16:18    [W:3.055 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site