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

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 <[email protected]> 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