[PATCH] nvme: reject completions for requests that are not in flight

[Chao S]

Hi Keith, Christoph, Jens,

Apologies for the slow reply. I've had several patches and crash analyses
from the same fuzzing effort in flight at once, along with writing the
work up against a paper deadline, and this thread slipped. That's on me;
I'll be prompt on follow-ups from here.

On the framing -- you're right, and I'll drop the "malicious device"
wording. e7006de6c238 was written for a malfunctioning controller; I've
reworded it to match, with no threat-model claim.

On whether this belongs in blk-mq rather than nvme -- I've kept the check
in nvme_find_rq() for now, since that is where a device-supplied command
id becomes a request. It isn't unique to nvme, though; if you'd rather the
check live in the blk-mq completion path, I'm happy to write it there.

Sending v2 with the reworded message shortly.

Thanks for the reviews,
Chao

On Wed, May 27, 2026 at 11:02 AM Jens Axboe <axboe at kernel.dk> wrote:
>
> On 5/27/26 8:19 AM, Christoph Hellwig wrote:
> > On Fri, May 22, 2026 at 11:30:34AM -0400, Chao Shi wrote:
> >> nvme_find_rq() resolves a device-supplied command id to a request with
> >> blk_mq_tag_to_rq(), which returns whatever request last used that tag -
> >> possibly one that is no longer in flight (freed, or never dispatched and
> >> thus with a NULL rq->mq_hctx).  Commit e7006de6c238 ("nvme: code
> >> command_id with a genctr for use-after-free validation") guards against
> >> this, but its generation counter is only 4 bits wide and can be matched
> >> by a malfunctioning or malicious device replaying command ids.  The
> >> driver then completes a request that is not outstanding, dereferencing a
> >> NULL rq->mq_hctx or double-completing a command:
> >
> > I don't think an intentionally malicious device is part of the threat
> > model here.  This was added to protect against buggy devices.
>
> Malicious devices are explicitly NOT part of the linux threat model. If
> this is a real device, I'd say go talk to whomever made it and get the
> firmware fixed. If this is a "hardening" effort to protect against the
> threat of malicious devices, then I don't think we should bother.
>
> >> +     * blk_mq_tag_to_rq() returns whatever request last used this tag, which
> >> +     * may no longer be in flight if the device reports a bogus command id.
> >> +     * Completing it would deref a NULL rq->mq_hctx or double-complete a
> >> +     * command; the 4-bit genctr below only narrows the window.
> >> +     */
> >> +    if (unlikely(blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT)) {
> >> +            dev_err(nvme_req(rq)->ctrl->device,
> >> +                    "completion for request %#x not in flight\n", tag);
> >> +            return NULL;
> >> +    }
> >
> > Although this check looks cheap enough that it should not hurt to add
> > it.  So I think this should be ok, but maybe respin with your planned
> > commit message update.
>
> Only for the right reasons, imho.
>
> --
> Jens Axboe