From: Baokun Li <[email protected]>
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 <[email protected]>
Signed-off-by: Baokun Li <[email protected]>
Acked-by: Jeff Layton <[email protected]>
Reviewed-by: Jia Zhu <[email protected]>
---
fs/cachefiles/internal.h | 1 +
fs/cachefiles/ondemand.c | 23 +++++++++++++++++++----
2 files changed, 20 insertions(+), 4 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 4ba42f1fa3b4..c011fb24d238 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)
{
@@ -330,6 +336,7 @@ 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;
@@ -356,15 +363,22 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
complete(&req->done);
}
+ cachefiles_req_put(req);
return n;
err_put_fd:
if (msg->opcode == CACHEFILES_OP_OPEN)
close_fd(((struct cachefiles_open *)msg->data)->fd);
error:
- xa_erase(&cache->reqs, id);
- req->error = ret;
- complete(&req->done);
+ xas_reset(&xas);
+ xas_lock(&xas);
+ if (xas_load(&xas) == req) {
+ req->error = ret;
+ complete(&req->done);
+ xas_store(&xas, NULL);
+ }
+ xas_unlock(&xas);
+ cachefiles_req_put(req);
return ret;
}
@@ -395,6 +409,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;
@@ -456,7 +471,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.
--
2.39.2
On 5/22/24 7:42 PM, [email protected] wrote:
> From: Baokun Li <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Baokun Li <[email protected]>
> Acked-by: Jeff Layton <[email protected]>
> Reviewed-by: Jia Zhu <[email protected]>
LGTM.
Reviewed-by: Jingbo Xu <[email protected]>
> ---
> fs/cachefiles/internal.h | 1 +
> fs/cachefiles/ondemand.c | 23 +++++++++++++++++++----
> 2 files changed, 20 insertions(+), 4 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 4ba42f1fa3b4..c011fb24d238 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)
> {
> @@ -330,6 +336,7 @@ 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;
> @@ -356,15 +363,22 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
> complete(&req->done);
> }
>
> + cachefiles_req_put(req);
> return n;
>
> err_put_fd:
> if (msg->opcode == CACHEFILES_OP_OPEN)
> close_fd(((struct cachefiles_open *)msg->data)->fd);
> error:
> - xa_erase(&cache->reqs, id);
> - req->error = ret;
> - complete(&req->done);
> + xas_reset(&xas);
> + xas_lock(&xas);
> + if (xas_load(&xas) == req) {
> + req->error = ret;
> + complete(&req->done);
> + xas_store(&xas, NULL);
> + }
> + xas_unlock(&xas);
> + cachefiles_req_put(req);
> return ret;
> }
>
> @@ -395,6 +409,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;
> @@ -456,7 +471,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.
--
Thanks,
Jingbo