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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024/5/20 15:36, Jingbo Xu wrote:

On 5/15/24 4:45 PM, libaokun@xxxxxxxxxxxxxxx wrote:
From: Baokun Li <libaokun1@xxxxxxxxxx>

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@xxxxxxxxxx>
Reviewed-by: Jia Zhu <zhujia.zj@xxxxxxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxx>

Thank you very much for your review!

--
With Best Regards,
Baokun Li





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux