Skip to content

bdev/nvme: prevent UAF by unregistering connect poller and null-guarding qpair #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
derekbit merged 1 commit into from
Dec 1, 2025
Merged

bdev/nvme: prevent UAF by unregistering connect poller and null-guarding qpair #68

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions module/bdev/nvme/bdev_nvme.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,17 @@ bdev_nvme_reset_check_qpair_connected(void *ctx)
struct nvme_qpair *nvme_qpair = ctrlr_ch->qpair;
struct spdk_nvme_qpair *qpair;

/* Check if nvme_qpair or its controller was freed due to disconnect callback.
* This can happen when ctrlr_ch was set to NULL and nvme_qpair was freed
* in bdev_nvme_disconnected_qpair_cb() while this poller was still scheduled.
* Just return and let the poller be cleaned up through normal channel destruction.
*/
if (nvme_qpair == NULL || nvme_qpair->ctrlr == NULL) {
SPDK_DEBUGLOG(bdev_nvme, "connect_poller: nvme_qpair=%p or ctrlr freed, "
"returning (UAF avoided).\n", nvme_qpair);
return SPDK_POLLER_BUSY;
}

if (ctrlr_ch->reset_iter == NULL) {
/* qpair was already failed to connect and the reset sequence is being aborted. */
assert(ctrlr_ch->connect_poller == NULL);
Expand Down Expand Up @@ -3719,7 +3730,22 @@ bdev_nvme_destroy_ctrlr_channel_cb(void *io_device, void *ctx_buf)
* We need to poll it until it is actually disconnected.
* Just detach the qpair from the deleting ctrlr_channel.
*/
if (ctrlr_ch->connect_poller) {
/* Unregister the connect poller to avoid it firing after this
* ctrlr_channel context has been freed. The poller may still
* be running if a connect/disconnect/reset sequence was in flight.
*/
NVME_CTRLR_INFOLOG(nvme_qpair->ctrlr,
"Unregistering connect_poller %p during channel destroy "
"(ctrlr_ch=%p, nvme_qpair=%p).\n",
ctrlr_ch->connect_poller, ctrlr_ch, nvme_qpair);
spdk_poller_unregister(&ctrlr_ch->connect_poller);

@derekbit derekbit Dec 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked the codes for registering and unregistering connect poller. Some questions

  • When will the connect register be registered?
  • It seems the UAF doesn't always happen. Do you know why?

@c3y1huang c3y1huang Dec 1, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will the connect register be registered?

It's registered during reset/failover: https://github.com/longhorn/spdk/blob/longhorn-v25.05/module/bdev/nvme/bdev_nvme.c#L2460. A detach during this window can trigger a race.

It seems the UAF doesn't always happen. Do you know why?

It’s a race between three async pieces that can arrive in different orders:

  1. Channel destroy callback (bdev_nvme_destroy_ctrlr_channel_cb)
  2. Disconnect callback (bdev_nvme_disconnected_qpair_cb)
  3. The connect poller firing (bdev_nvme_reset_check_qpair_connected)

Ref: longhorn/longhorn#11698 (comment)

}
NVME_CTRLR_DEBUGLOG(nvme_qpair->ctrlr,
"Clearing ctrlr_ch->qpair pointer (ctrlr_ch=%p, nvme_qpair=%p).\n",
ctrlr_ch, nvme_qpair);
nvme_qpair->ctrlr_ch = NULL;
ctrlr_ch->qpair = NULL;
} else {
assert(ctrlr_ch->reset_iter == NULL);

Expand Down