Re: [PATCH v2 03/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_get_fd()

Jingbo Xu
2024-05-20T08:06:59Z

On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> We got the following issue in a fuzz test of randomly issuing the restore
> command:
>
> ==================================================================
> BUG: KASAN: slab-use-after-free in cachefiles_ondemand_daemon_read+0x609/0xab0
> Write of size 4 at addr ffff888109164a80 by task ondemand-04-dae/4962
>
> CPU: 11 PID: 4962 Comm: ondemand-04-dae Not tainted 6.8.0-rc7-dirty #542
> Call Trace:
> kasan_report+0x94/0xc0
> cachefiles_ondemand_daemon_read+0x609/0xab0
> vfs_read+0x169/0xb50
> ksys_read+0xf5/0x1e0
>
> Allocated by task 626:
> __kmalloc+0x1df/0x4b0
> cachefiles_ondemand_send_req+0x24d/0x690
> cachefiles_create_tmpfile+0x249/0xb30
> cachefiles_create_file+0x6f/0x140
> cachefiles_look_up_object+0x29c/0xa60
> cachefiles_lookup_cookie+0x37d/0xca0
> fscache_cookie_state_machine+0x43c/0x1230
> [...]
>
> Freed by task 626:
> kfree+0xf1/0x2c0
> cachefiles_ondemand_send_req+0x568/0x690
> cachefiles_create_tmpfile+0x249/0xb30
> cachefiles_create_file+0x6f/0x140
> cachefiles_look_up_object+0x29c/0xa60
> cachefiles_lookup_cookie+0x37d/0xca0
> fscache_cookie_state_machine+0x43c/0x1230
> [...]
> ==================================================================
>
> Following is the process that triggers the issue:
>
> mount | daemon_thread1 | daemon_thread2
> ------------------------------------------------------------
> cachefiles_ondemand_init_object
> cachefiles_ondemand_send_req
> REQ_A = kzalloc(sizeof(*req) + data_len)
> wait_for_completion(&REQ_A->done)
>
> cachefiles_daemon_read
> cachefiles_ondemand_daemon_read
> REQ_A = cachefiles_ondemand_select_req
> cachefiles_ondemand_get_fd
> copy_to_user(_buffer, msg, n)
> process_open_req(REQ_A)
> ------ restore ------
> cachefiles_ondemand_restore
> xas_for_each(&xas, req, ULONG_MAX)
> xas_set_mark(&xas, CACHEFILES_REQ_NEW);
>
> cachefiles_daemon_read
> cachefiles_ondemand_daemon_read
> REQ_A = cachefiles_ondemand_select_req
>
> write(devfd, ("copen %u,%llu", msg->msg_id, size));
> cachefiles_ondemand_copen
> xa_erase(&cache->reqs, id)
> complete(&REQ_A->done)
> kfree(REQ_A)
> cachefiles_ondemand_get_fd(REQ_A)
> fd = get_unused_fd_flags
> file = anon_inode_getfile
> fd_install(fd, file)
> load = (void *)REQ_A->msg.data;
> load->fd = fd;
> // load UAF !!!
>
> This issue is caused by issuing a restore command when the daemon is still
> alive, which results in a request being processed multiple times thus
> triggering a UAF. So to avoid this problem, add an additional reference
> count to cachefiles_req, which is held while waiting and reading, and then
> released when the waiting and reading is over.
>
> Note that since there is only one reference count for waiting, we need to
> avoid the same request being completed multiple times, so we can only
> complete the request if it is successfully removed from the xarray.
>
> Fixes: e73fa11a356c ("cachefiles: add restore command to recover inflight ondemand read requests")
> Suggested-by: Hou Tao <houtao1@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>

How could we protect it from being erased from the xarray with the same
message id in this case?

--
Thanks,
Jingbo