| 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
| ||
|
Message-ID: <34ba3b5c-638c-4622-8bcb-a2ef74b22f69@bytedance.com>
Date: Wed, 24 Apr 2024 22:55:57 +0800
From: Jia Zhu <zhujia.zj@...edance.com>
To: libaokun@...weicloud.com, netfs@...ts.linux.dev
Cc: dhowells@...hat.com, jlayton@...nel.org, jefflexu@...ux.alibaba.com,
linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, Baokun Li <libaokun1@...wei.com>,
Hou Tao <houtao1@...wei.com>, zhujia.zj@...edance.com
Subject: Re: [PATCH 03/12] cachefiles: fix slab-use-after-free in
cachefiles_ondemand_get_fd()
在 2024/4/24 11:39, libaokun@...weicloud.com 写道:
> From: Baokun Li <libaokun1@...wei.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.
Hi Baokun,
Thank you for catching this issue. Shall we fix this by following steps:
cachefiles_ondemand_fd_release()
xas_for_each(req)
if req is not CACHEFILES_OP_READ
flush
cachefiles_ondemand_restore()
xas_for_each(req)
if req is not CACHEFILES_REQ_NEW && op is (OPEN or CLOSE)
reset req to CACHEFILES_REQ_NEW
By implementing these steps:
1. In real daemon failover case： only pending read reqs will be
reserved. cachefiles_ondemand_select_req will reopen the object by
processing READ req.
2. In daemon alive case： Only read reqs will be reset to
CACHEFILES_REQ_NEW.
>
> 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@...wei.com>
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> ---
> fs/cachefiles/internal.h | 1 +
> fs/cachefiles/ondemand.c | 44 ++++++++++++++++++++++------------------
> 2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index d33169f0018b..7745b8abc3aa 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -138,6 +138,7 @@ static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
> struct cachefiles_req {
> struct cachefiles_object *object;
> struct completion done;
> + refcount_t ref;
> int error;
> struct cachefiles_msg msg;
> };
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index fd49728d8bae..56d12fe4bf73 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -4,6 +4,12 @@
> #include <linux/uio.h>
> #include "internal.h"
>
> +static inline void cachefiles_req_put(struct cachefiles_req *req)
> +{
> + if (refcount_dec_and_test(&req->ref))
> + kfree(req);
> +}
> +
> static int cachefiles_ondemand_fd_release(struct inode *inode,
> struct file *file)
> {
> @@ -299,7 +305,6 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
> {
> struct cachefiles_req *req;
> struct cachefiles_msg *msg;
> - unsigned long id = 0;
> size_t n;
> int ret = 0;
> XA_STATE(xas, &cache->reqs, cache->req_id_next);
> @@ -330,41 +335,39 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>
> xas_clear_mark(&xas, CACHEFILES_REQ_NEW);
> cache->req_id_next = xas.xa_index + 1;
> + refcount_inc(&req->ref);
> xa_unlock(&cache->reqs);
>
> - id = xas.xa_index;
> -
> if (msg->opcode == CACHEFILES_OP_OPEN) {
> ret = cachefiles_ondemand_get_fd(req);
> if (ret) {
> cachefiles_ondemand_set_object_close(req->object);
> - goto error;
> + goto out;
> }
> }
>
> - msg->msg_id = id;
> + msg->msg_id = xas.xa_index;
> msg->object_id = req->object->ondemand->ondemand_id;
>
> if (copy_to_user(_buffer, msg, n) != 0) {
> ret = -EFAULT;
> if (msg->opcode == CACHEFILES_OP_OPEN)
> close_fd(((struct cachefiles_open *)msg->data)->fd);
> - goto error;
> }
> -
> - /* CLOSE request has no reply */
> - if (msg->opcode == CACHEFILES_OP_CLOSE) {
> - xa_erase(&cache->reqs, id);
> - complete(&req->done);
> +out:
> + /* Remove error request and CLOSE request has no reply */
> + if (ret || msg->opcode == CACHEFILES_OP_CLOSE) {
> + xas_reset(&xas);
> + xas_lock(&xas);
> + if (xas_load(&xas) == req) {
> + req->error = ret;
> + complete(&req->done);
> + xas_store(&xas, NULL);
> + }
> + xas_unlock(&xas);
> }
> -
> - return n;
> -
> -error:
> - xa_erase(&cache->reqs, id);
> - req->error = ret;
> - complete(&req->done);
> - return ret;
> + cachefiles_req_put(req);
> + return ret ? ret : n;
> }
>
> typedef int (*init_req_fn)(struct cachefiles_req *req, void *private);
> @@ -394,6 +397,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
> goto out;
> }
>
> + refcount_set(&req->ref, 1);
> req->object = object;
> init_completion(&req->done);
> req->msg.opcode = opcode;
> @@ -455,7 +459,7 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
> wake_up_all(&cache->daemon_pollwq);
> wait_for_completion(&req->done);
> ret = req->error;
> - kfree(req);
> + cachefiles_req_put(req);
> return ret;
> out:
> /* Reset the object to close state in error handling path.
Powered by blists - more mailing lists