From 6f0ff37bbdd42747a90eecd7086beda55cf87b5c Mon Sep 17 00:00:00 2001 From: Changpeng Liu Date: Mon, 7 Feb 2022 21:48:08 +0800 Subject: [PATCH] nvmf/vfio-user: fix an heap-use-after-free issue The controller data structure may be freed before subsystem resume done callback, we can take endpoint as the input parameter to avoid this issue. AddressSanitizer: heap-use-after-free on address 0x625000046100 at pc 0x00000082818f bp 0x7fff7b09bd10 sp 0x7fff7b09bd00 READ of size 8 at 0x625000046100 thread T0 (reactor_0) #0 0x82818e in vfio_user_dev_quiesce_resume_done /spdk/lib/nvmf/vfio_user.c:2147 #1 0x782cc0 in subsystem_state_change_done /spdk/lib/nvmf/subsystem.c:634 #2 0xad047b in _call_completion /spdk/lib/thread/thread.c:2344 #3 0xabc48d in msg_queue_run_batch /spdk/lib/thread/thread.c:710 #4 0xac0670 in thread_poll /spdk/lib/thread/thread.c:926 #5 0xac0ead in spdk_thread_poll /spdk/lib/thread/thread.c:986 #6 0x9a5b4f in _reactor_run /spdk/lib/event/reactor.c:920 #7 0x9a6442 in reactor_run /spdk/lib/event/reactor.c:958 #8 0x9a717c in spdk_reactors_start /spdk/lib/event/reactor.c:1060 #9 0x99884a in spdk_app_start /spdk/lib/event/app.c:643 #10 0x407e82 in main /spdk/app/nvmf_tgt/nvmf_main.c:75 #11 0x7f822095ff42 in __libc_start_main (/lib64/libc.so.6+0x23f42) #12 0x407abd in _start (/spdk/build/bin/nvmf_tgt+0x407abd) 0x625000046100 is located 0 bytes inside of 8320-byte region [0x625000046100,0x625000048180) freed by thread T0 (reactor_0) here: #0 0x7f82219ff91f in __interceptor_free (/lib64/libasan.so.5+0x10d91f) #1 0x837059 in _free_ctrlr /spdk/lib/nvmf/vfio_user.c:2976 #2 0x837327 in free_ctrlr /spdk/lib/nvmf/vfio_user.c:2996 #3 0x843541 in nvmf_vfio_user_close_qpair /spdk/lib/nvmf/vfio_user.c:3742 #4 0x7d1d91 in nvmf_transport_qpair_fini /spdk/lib/nvmf/transport.c:604 #5 0x7ad922 in _nvmf_qpair_destroy /spdk/lib/nvmf/nvmf.c:1055 #6 0x761362 in nvmf_qpair_request_cleanup /spdk/lib/nvmf/ctrlr.c:4026 #7 0x761906 in spdk_nvmf_request_free /spdk/lib/nvmf/ctrlr.c:4041 #8 0x75a931 in nvmf_qpair_free_aer /spdk/lib/nvmf/ctrlr.c:3576 #9 0x7ae626 in spdk_nvmf_qpair_disconnect /spdk/lib/nvmf/nvmf.c:1127 #10 0x83db36 in _vfio_user_qpair_disconnect /spdk/lib/nvmf/vfio_user.c:3433 #11 0xabc48d in msg_queue_run_batch /spdk/lib/thread/thread.c:710 #12 0xac0670 in thread_poll /spdk/lib/thread/thread.c:926 #13 0xac0ead in spdk_thread_poll /spdk/lib/thread/thread.c:986 #14 0x9a5b4f in _reactor_run /spdk/lib/event/reactor.c:920 #15 0x9a6442 in reactor_run /spdk/lib/event/reactor.c:958 #16 0x9a717c in spdk_reactors_start /spdk/lib/event/reactor.c:1060 #17 0x99884a in spdk_app_start /spdk/lib/event/app.c:643 #18 0x407e82 in main /spdk/app/nvmf_tgt/nvmf_main.c:75 #19 0x7f822095ff42 in __libc_start_main (/lib64/libc.so.6+0x23f42) previously allocated by thread T0 (reactor_0) here: #0 0x7f82219fff16 in __interceptor_calloc (/lib64/libasan.so.5+0x10df16) #1 0x837413 in nvmf_vfio_user_create_ctrlr /spdk/lib/nvmf/vfio_user.c:3010 #2 0x83bc68 in nvmf_vfio_user_accept /spdk/lib/nvmf/vfio_user.c:3313 #3 0xabfbd8 in thread_execute_timed_poller /spdk/lib/thread/thread.c:872 #4 0xac0c75 in thread_poll /spdk/lib/thread/thread.c:960 #5 0xac0ead in spdk_thread_poll /spdk/lib/thread/thread.c:986 #6 0x9a5b4f in _reactor_run /spdk/lib/event/reactor.c:920 #7 0x9a6442 in reactor_run /spdk/lib/event/reactor.c:958 #8 0x9a717c in spdk_reactors_start /spdk/lib/event/reactor.c:1060 #9 0x99884a in spdk_app_start /spdk/lib/event/app.c:643 #10 0x407e82 in main /spdk/app/nvmf_tgt/nvmf_main.c:75 #11 0x7f822095ff42 in __libc_start_main (/lib64/libc.so.6+0x23f42) SUMMARY: AddressSanitizer: heap-use-after-free /spdk/lib/nvmf/vfio_user.c:2147 in vfio_user_dev_quiesce_resume_done Change-Id: Icf5e5b360b9107a3c5eb960ae59b7fe10ace1c66 Signed-off-by: Changpeng Liu Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11420 Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Dong Yi Reviewed-by: John Levon Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/nvmf/vfio_user.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/nvmf/vfio_user.c b/lib/nvmf/vfio_user.c index 78fa69f80b7..7f51fa38ca3 100644 --- a/lib/nvmf/vfio_user.c +++ b/lib/nvmf/vfio_user.c @@ -2205,15 +2205,18 @@ vfio_user_dev_quiesce_done(struct spdk_nvmf_subsystem *subsystem, void *cb_arg, int status); static void -vfio_user_dev_quiesce_resume_done(struct spdk_nvmf_subsystem *subsystem, - void *cb_arg, int status) +vfio_user_endpoint_resume_done(struct spdk_nvmf_subsystem *subsystem, + void *cb_arg, int status) { - struct nvmf_vfio_user_ctrlr *vu_ctrlr = cb_arg; - struct nvmf_vfio_user_endpoint *endpoint = vu_ctrlr->endpoint; + struct nvmf_vfio_user_endpoint *endpoint = cb_arg; + struct nvmf_vfio_user_ctrlr *vu_ctrlr = endpoint->ctrlr; int ret; - SPDK_DEBUGLOG(nvmf_vfio, "%s resumed done with status %d\n", ctrlr_id(vu_ctrlr), status); + SPDK_DEBUGLOG(nvmf_vfio, "%s resumed done with status %d\n", endpoint_id(endpoint), status); + if (!vu_ctrlr) { + return; + } vu_ctrlr->state = VFIO_USER_CTRLR_RUNNING; /* Basically, once we call `vfu_device_quiesced` the device is unquiesced from @@ -2261,7 +2264,7 @@ vfio_user_dev_quiesce_done(struct spdk_nvmf_subsystem *subsystem, SPDK_DEBUGLOG(nvmf_vfio, "%s start to resume\n", ctrlr_id(vu_ctrlr)); vu_ctrlr->state = VFIO_USER_CTRLR_RESUMING; ret = spdk_nvmf_subsystem_resume((struct spdk_nvmf_subsystem *)endpoint->subsystem, - vfio_user_dev_quiesce_resume_done, vu_ctrlr); + vfio_user_endpoint_resume_done, endpoint); if (ret < 0) { vu_ctrlr->state = VFIO_USER_CTRLR_PAUSED; SPDK_ERRLOG("%s: failed to resume, ret=%d\n", endpoint_id(endpoint), ret);