Re: [PATCH 1/1] nvme: fix use-after-free of admin queue via stale pointer

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 10/31/25 12:19, Casey Chen wrote:
> On Thu, Oct 30, 2025 at 1:12 AM Chaitanya Kulkarni
> <chaitanyak@nvidia.com> wrote:
>> On 10/29/25 17:39, Keith Busch wrote:
>>> On Wed, Oct 29, 2025 at 03:08:53PM -0600, Casey Chen wrote:
>>>> Fix this by taking an additional reference on the admin queue during
>>>> namespace allocation and releasing it during namespace cleanup.
>>> Since the namespaces already hold references on the controller, would it
>>> be simpler to move the controller's final blk_put_queue to the final
>>> ctrl free? This should have the same lifetime as your patch, but with
>>> simpler ref counting:
>>>
>>> ---
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index fa4181d7de736..0b83d82f67e75 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -4901,7 +4901,6 @@ void nvme_remove_admin_tag_set(struct nvme_ctrl *ctrl)
>>>            */
>>>           nvme_stop_keep_alive(ctrl);
>>>           blk_mq_destroy_queue(ctrl->admin_q);
>>> -       blk_put_queue(ctrl->admin_q);
>>>           if (ctrl->ops->flags & NVME_F_FABRICS) {
>>>                   blk_mq_destroy_queue(ctrl->fabrics_q);
>>>                   blk_put_queue(ctrl->fabrics_q);
>>> @@ -5045,6 +5044,7 @@ static void nvme_free_ctrl(struct device *dev)
>>>                   container_of(dev, struct nvme_ctrl, ctrl_device);
>>>           struct nvme_subsystem *subsys = ctrl->subsys;
>>>
>>> +       blk_put_queue(ctrl->admin_q);
>>>           if (!subsys || ctrl->instance != subsys->instance)
>>>                   ida_free(&nvme_instance_ida, ctrl->instance);
>>>           nvme_free_cels(ctrl);
>>> --
>>>
>> above is much better approach that doesn't rely on taking extra
>> ref count but using existing count to protect the UAF.
>> I've added required comments that are very much needed here,
>> totally untested :-
>>
>> nvme: fix UAF when accessing admin queue after removal
>>
>> Fix a use-after-free where userspace IOCTLs can access ctrl->admin_q
>> after it has been freed during controller removal.
>>
>> The Race Condition:
>>
>>     Thread 1 (userspace IOCTL)          Thread 2 (sysfs remove)
>>     --------------------------          -------------------
>>     open(/dev/nvme0n1) -> fd=3
>>     ioctl(3, NVME_IOCTL_ADMIN_CMD)
>>       nvme_ioctl()
>>       nvme_user_cmd()
>>                                          echo 1 > .../remove
>>                                          pci_device_remove()
>>                                          nvme_remove()
>>    nvme_remove_admin_tag_set()
>>                                            blk_put_queue(admin_q)
>>                                            [RCU grace period]
>>                                            blk_free_queue(admin_q)
>>                                              kmem_cache_free() <- FREED
>>       nvme_submit_user_cmd(ns->ctrl->admin_q) <- STALE POINTER
>>         blk_mq_alloc_request(admin_q)
>>           blk_queue_enter(admin_q)
>>             *** USE-AFTER-FREE ***
>>
>>
>> The admin queue is freed in nvme_remove_admin_tag_set() while userspace
>> may still hold open file descriptors to namespace devices. These open
>> file descriptors can issue IOCTLs that dereference ctrl->admin_q after
>> it has been freed.
>>
>> Defer blk_put_queue(ctrl->admin_q) from nvme_remove_admin_tag_set() to
>> nvme_free_ctrl(). Since each namespace holds a controller reference via
>> nvme_get_ctrl()/nvme_put_ctrl(), the controller will only be freed after
>> all namespaces (and their open file descriptors) are released. This
>> guarantees admin_q remains allocated while it may still be accessed.
>>
>> After blk_mq_destroy_queue() in nvme_remove_admin_tag_set(), the queue
>> is marked dying (QUEUE_FLAG_DYING), so new IOCTL attempts fail safely
>> at blk_queue_enter() with -ENODEV. The queue structure remains valid for
>> pointer dereference until nvme_free_ctrl() is called.
>>
>> ---
>>    drivers/nvme/host/core.c | 22 +++++++++++++++++++++-
>>    1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 734ad725e6f4..dbbcf99dbef8 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -4897,7 +4897,19 @@ void nvme_remove_admin_tag_set(struct nvme_ctrl
>> *ctrl)
>>         */
>>        nvme_stop_keep_alive(ctrl);
>>        blk_mq_destroy_queue(ctrl->admin_q);
>> -    blk_put_queue(ctrl->admin_q);
>> +    /**
>> +     * Defer blk_put_queue() to nvme_free_ctrl() to prevent use-after-free.
>> +     *
>> +     * Userspace may hold open file descriptors to namespace devices and
>> +     * issue IOCTLs that dereference ctrl->admin_q after controller removal
>> +     * starts. Since each namespace holds a controller reference, deferring
>> +     * the final queue release ensures admin_q remains allocated until all
>> +     * namespace references are released.
>> +     *
>> +     * blk_mq_destroy_queue() above marks the queue dying
>> (QUEUE_FLAG_DYING),
>> +     * causing new requests to fail at blk_queue_enter() with -ENODEV while
>> +     * keeping the structure valid for pointer access.
>> +     */
>>        if (ctrl->ops->flags & NVME_F_FABRICS) {
>>            blk_mq_destroy_queue(ctrl->fabrics_q);
>>            blk_put_queue(ctrl->fabrics_q);
>> @@ -5041,6 +5053,14 @@ static void nvme_free_ctrl(struct device *dev)
>>            container_of(dev, struct nvme_ctrl, ctrl_device);
>>        struct nvme_subsystem *subsys = ctrl->subsys;
>>
>> +    /**
>> +     * Release admin_q's final reference. All namespace references have
>> +     * been released at this point. NULL check is needed for to handle
>> +     * allocation failure in nvme_alloc_admin_tag_set().
>> +     */
>> +    if (ctrl->admin_q)
>> +        blk_put_queue(ctrl->admin_q);
>> +
>>        if (!subsys || ctrl->instance != subsys->instance)
>>            ida_free(&nvme_instance_ida, ctrl->instance);
>>        nvme_free_cels(ctrl);
>>
>> -ck
>>
>>
>>
> Thanks Chaitanya. I tested your fix and all tests look good. We're
> looking forward to your final version.


This is Keith's patch I just tested his patch and added commit log-comments.


-ck