| Message ID | 20240515084601.3240503-5-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
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>
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!
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 ") \