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+0xb41/0xb60
Read of size 8 at addr ffff888122e84088 by task ondemand-04-dae/963
CPU: 13 PID: 963 Comm: ondemand-04-dae Not tainted 6.8.0-dirty #564
Call Trace:
kasan_report+0x93/0xc0
cachefiles_ondemand_daemon_read+0xb41/0xb60
vfs_read+0x169/0xb50
ksys_read+0xf5/0x1e0
Allocated by task 116:
kmem_cache_alloc+0x140/0x3a0
cachefiles_lookup_cookie+0x140/0xcd0
fscache_cookie_state_machine+0x43c/0x1230
[...]
Freed by task 792:
kmem_cache_free+0xfe/0x390
cachefiles_put_object+0x241/0x480
fscache_cookie_state_machine+0x5c8/0x1230
[...]
==================================================================
Following is the process that triggers the issue:
mount | daemon_thread1 | daemon_thread2
------------------------------------------------------------
cachefiles_withdraw_cookie
cachefiles_ondemand_clean_object(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
msg->object_id = req->object->ondemand->ondemand_id
------ 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
copy_to_user(_buffer, msg, n)
xa_erase(&cache->reqs, id)
complete(&REQ_A->done)
------ close(fd) ------
cachefiles_ondemand_fd_release
cachefiles_put_object
cachefiles_put_object
kmem_cache_free(cachefiles_object_jar, object)
REQ_A->object->ondemand->ondemand_id
// object UAF !!!
When we see the request within xa_lock, req->object must not have been
freed yet, so grab the reference count of object before xa_unlock to
avoid the above issue.
Fixes: 0a7e54c1959c ("cachefiles: resend an open request if the read request's object is closed")
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
---
fs/cachefiles/ondemand.c | 2 ++
include/trace/events/cachefiles.h | 6 +++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 56d12fe4bf73..bb94ef6a6f61 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -336,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);
+ cachefiles_grab_object(req->object, cachefiles_obj_get_read_req);
xa_unlock(&cache->reqs);
if (msg->opcode == CACHEFILES_OP_OPEN) {
@@ -355,6 +356,7 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
close_fd(((struct cachefiles_open *)msg->data)->fd);
}
out:
+ cachefiles_put_object(req->object, cachefiles_obj_put_read_req);
/* Remove error request and CLOSE request has no reply */
if (ret || msg->opcode == CACHEFILES_OP_CLOSE) {
xas_reset(&xas);
diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
index cf4b98b9a9ed..119a823fb5a0 100644
--- a/include/trace/events/cachefiles.h
+++ b/include/trace/events/cachefiles.h
@@ -33,6 +33,8 @@ enum cachefiles_obj_ref_trace {
cachefiles_obj_see_withdrawal,
cachefiles_obj_get_ondemand_fd,
cachefiles_obj_put_ondemand_fd,
+ cachefiles_obj_get_read_req,
+ cachefiles_obj_put_read_req,
};
enum fscache_why_object_killed {
@@ -127,7 +129,9 @@ enum cachefiles_error_trace {
EM(cachefiles_obj_see_lookup_cookie, "SEE lookup_cookie") \
EM(cachefiles_obj_see_lookup_failed, "SEE lookup_failed") \
EM(cachefiles_obj_see_withdraw_cookie, "SEE withdraw_cookie") \
- E_(cachefiles_obj_see_withdrawal, "SEE withdrawal")
+ EM(cachefiles_obj_see_withdrawal, "SEE withdrawal") \
+ EM(cachefiles_obj_get_read_req, "GET read_req") \
+ E_(cachefiles_obj_put_read_req, "PUT read_req")
#define cachefiles_coherency_traces \
EM(cachefiles_coherency_check_aux, "BAD aux ") \
--
2.39.2
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+0xb41/0xb60
> Read of size 8 at addr ffff888122e84088 by task ondemand-04-dae/963
>
> CPU: 13 PID: 963 Comm: ondemand-04-dae Not tainted 6.8.0-dirty #564
> Call Trace:
> kasan_report+0x93/0xc0
> cachefiles_ondemand_daemon_read+0xb41/0xb60
> vfs_read+0x169/0xb50
> ksys_read+0xf5/0x1e0
>
> Allocated by task 116:
> kmem_cache_alloc+0x140/0x3a0
> cachefiles_lookup_cookie+0x140/0xcd0
> fscache_cookie_state_machine+0x43c/0x1230
> [...]
>
> Freed by task 792:
> kmem_cache_free+0xfe/0x390
> cachefiles_put_object+0x241/0x480
> fscache_cookie_state_machine+0x5c8/0x1230
> [...]
> ==================================================================
>
> Following is the process that triggers the issue:
>
> mount | daemon_thread1 | daemon_thread2
> ------------------------------------------------------------
> cachefiles_withdraw_cookie
> cachefiles_ondemand_clean_object(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
> msg->object_id = req->object->ondemand->ondemand_id
> ------ 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
> copy_to_user(_buffer, msg, n)
> xa_erase(&cache->reqs, id)
> complete(&REQ_A->done)
> ------ close(fd) ------
> cachefiles_ondemand_fd_release
> cachefiles_put_object
> cachefiles_put_object
> kmem_cache_free(cachefiles_object_jar, object)
> REQ_A->object->ondemand->ondemand_id
> // object UAF !!!
>
> When we see the request within xa_lock, req->object must not have been
> freed yet, so grab the reference count of object before xa_unlock to
> avoid the above issue.
>
> Fixes: 0a7e54c1959c ("cachefiles: resend an open request if the read request's object is closed")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
> ---
> fs/cachefiles/ondemand.c | 2 ++
> include/trace/events/cachefiles.h | 6 +++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 56d12fe4bf73..bb94ef6a6f61 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -336,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);
> + cachefiles_grab_object(req->object, cachefiles_obj_get_read_req);
> xa_unlock(&cache->reqs);
>
> if (msg->opcode == CACHEFILES_OP_OPEN) {
> @@ -355,6 +356,7 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
> close_fd(((struct cachefiles_open *)msg->data)->fd);
> }
> out:
> + cachefiles_put_object(req->object, cachefiles_obj_put_read_req);
> /* Remove error request and CLOSE request has no reply */
> if (ret || msg->opcode == CACHEFILES_OP_CLOSE) {
> xas_reset(&xas);
> diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
> index cf4b98b9a9ed..119a823fb5a0 100644
> --- a/include/trace/events/cachefiles.h
> +++ b/include/trace/events/cachefiles.h
> @@ -33,6 +33,8 @@ enum cachefiles_obj_ref_trace {
> cachefiles_obj_see_withdrawal,
> cachefiles_obj_get_ondemand_fd,
> cachefiles_obj_put_ondemand_fd,
> + cachefiles_obj_get_read_req,
> + cachefiles_obj_put_read_req,
How about cachefiles_obj_[get|put]_ondemand_read, so that it could be
easily identified as ondemand mode at the first glance?
> };
>
> enum fscache_why_object_killed {
> @@ -127,7 +129,9 @@ enum cachefiles_error_trace {
> EM(cachefiles_obj_see_lookup_cookie, "SEE lookup_cookie") \
> EM(cachefiles_obj_see_lookup_failed, "SEE lookup_failed") \
> EM(cachefiles_obj_see_withdraw_cookie, "SEE withdraw_cookie") \
> - E_(cachefiles_obj_see_withdrawal, "SEE withdrawal")
> + EM(cachefiles_obj_see_withdrawal, "SEE withdrawal") \
> + EM(cachefiles_obj_get_read_req, "GET read_req") \
> + E_(cachefiles_obj_put_read_req, "PUT read_req")
Ditto.
Otherwise, LGTM.
Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>
--
Thanks,
Jingbo
On 2024/5/20 15:36, Jingbo Xu wrote:
>
> 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+0xb41/0xb60
>> Read of size 8 at addr ffff888122e84088 by task ondemand-04-dae/963
>>
>> CPU: 13 PID: 963 Comm: ondemand-04-dae Not tainted 6.8.0-dirty #564
>> Call Trace:
>> kasan_report+0x93/0xc0
>> cachefiles_ondemand_daemon_read+0xb41/0xb60
>> vfs_read+0x169/0xb50
>> ksys_read+0xf5/0x1e0
>>
>> Allocated by task 116:
>> kmem_cache_alloc+0x140/0x3a0
>> cachefiles_lookup_cookie+0x140/0xcd0
>> fscache_cookie_state_machine+0x43c/0x1230
>> [...]
>>
>> Freed by task 792:
>> kmem_cache_free+0xfe/0x390
>> cachefiles_put_object+0x241/0x480
>> fscache_cookie_state_machine+0x5c8/0x1230
>> [...]
>> ==================================================================
>>
>> Following is the process that triggers the issue:
>>
>> mount | daemon_thread1 | daemon_thread2
>> ------------------------------------------------------------
>> cachefiles_withdraw_cookie
>> cachefiles_ondemand_clean_object(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
>> msg->object_id = req->object->ondemand->ondemand_id
>> ------ 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
>> copy_to_user(_buffer, msg, n)
>> xa_erase(&cache->reqs, id)
>> complete(&REQ_A->done)
>> ------ close(fd) ------
>> cachefiles_ondemand_fd_release
>> cachefiles_put_object
>> cachefiles_put_object
>> kmem_cache_free(cachefiles_object_jar, object)
>> REQ_A->object->ondemand->ondemand_id
>> // object UAF !!!
>>
>> When we see the request within xa_lock, req->object must not have been
>> freed yet, so grab the reference count of object before xa_unlock to
>> avoid the above issue.
>>
>> Fixes: 0a7e54c1959c ("cachefiles: resend an open request if the read request's object is closed")
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com>
>> ---
>> fs/cachefiles/ondemand.c | 2 ++
>> include/trace/events/cachefiles.h | 6 +++++-
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index 56d12fe4bf73..bb94ef6a6f61 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -336,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);
>> + cachefiles_grab_object(req->object, cachefiles_obj_get_read_req);
>> xa_unlock(&cache->reqs);
>>
>> if (msg->opcode == CACHEFILES_OP_OPEN) {
>> @@ -355,6 +356,7 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>> close_fd(((struct cachefiles_open *)msg->data)->fd);
>> }
>> out:
>> + cachefiles_put_object(req->object, cachefiles_obj_put_read_req);
>> /* Remove error request and CLOSE request has no reply */
>> if (ret || msg->opcode == CACHEFILES_OP_CLOSE) {
>> xas_reset(&xas);
>> diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
>> index cf4b98b9a9ed..119a823fb5a0 100644
>> --- a/include/trace/events/cachefiles.h
>> +++ b/include/trace/events/cachefiles.h
>> @@ -33,6 +33,8 @@ enum cachefiles_obj_ref_trace {
>> cachefiles_obj_see_withdrawal,
>> cachefiles_obj_get_ondemand_fd,
>> cachefiles_obj_put_ondemand_fd,
>> + cachefiles_obj_get_read_req,
>> + cachefiles_obj_put_read_req,
> How about cachefiles_obj_[get|put]_ondemand_read, so that it could be
> easily identified as ondemand mode at the first glance?
The ondemand_read tends to confuse whether it's
ondemand_daemon_read or ondemand_data_read. I think it's better
to emphasise the read request, and currently only the ondemand
mode has a cachefiles req.
>> };
>>
>> enum fscache_why_object_killed {
>> @@ -127,7 +129,9 @@ enum cachefiles_error_trace {
>> EM(cachefiles_obj_see_lookup_cookie, "SEE lookup_cookie") \
>> EM(cachefiles_obj_see_lookup_failed, "SEE lookup_failed") \
>> EM(cachefiles_obj_see_withdraw_cookie, "SEE withdraw_cookie") \
>> - E_(cachefiles_obj_see_withdrawal, "SEE withdrawal")
>> + EM(cachefiles_obj_see_withdrawal, "SEE withdrawal") \
>> + EM(cachefiles_obj_get_read_req, "GET read_req") \
>> + E_(cachefiles_obj_put_read_req, "PUT read_req")
> Ditto.
>
>
> Otherwise, LGTM.
>
> Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>
Thank you very much for your review!
--
With Best Regards,
Baokun Li
© 2016 - 2026 Red Hat, Inc.