[PATCH v2 4/4] nvme: code command_id with a genctr for use-after-free validation

[Hannes Reinecke]

On 5/19/21 7:43 PM, Sagi Grimberg wrote:
> We cannot detect a (perhaps buggy) controller that is sending us
> a completion for a request that was already completed (for example
> sending a completion twice), this phenomenon was seen in the wild
> a few times.
> 
> So to protect against this, we use the upper 4 msbits of the nvme sqe
> command_id to use as a 4-bit generation counter and verify it matches
> the existing request generation that is incrementing on every execution.
> 
> The 16-bit command_id structure now is constructed by:
> | xxxx | xxxxxxxxxxxx |
>   gen    request tag
> 
> This means that we are giving up some possible queue depth as 12 bits
> allow for a maximum queue depth of 4095 instead of 65536, however we
> never create such long queues anyways so no real harm done.
> 
> Suggested-by: Keith Busch <kbusch at kernel.org>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/host/core.c   |  3 ++-
>  drivers/nvme/host/nvme.h   | 47 +++++++++++++++++++++++++++++++++++++-
>  drivers/nvme/host/pci.c    |  2 +-
>  drivers/nvme/host/rdma.c   |  4 ++--
>  drivers/nvme/host/tcp.c    | 26 ++++++++++-----------
>  drivers/nvme/target/loop.c |  4 ++--
>  6 files changed, 66 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e6612971f4eb..8308dd1e9e3f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1006,7 +1006,8 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req)
>  		return BLK_STS_IOERR;
>  	}
>  
> -	cmd->common.command_id = req->tag;
> +	nvme_req(req)->genctr++;
> +	cmd->common.command_id = nvme_cid(req);
>  	trace_nvme_setup_cmd(req, cmd);
>  	return ret;
>  }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 05f31a2c64bb..e6f72c82582d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -158,6 +158,7 @@ enum nvme_quirks {
>  struct nvme_request {
>  	struct nvme_command	*cmd;
>  	union nvme_result	result;
> +	u8			genctr;
>  	u8			retries;
>  	u8			flags;
>  	u16			status;
> @@ -497,6 +498,49 @@ struct nvme_ctrl_ops {
>  	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
>  };
>  
> +/*
> + * nvme command_id is constructed as such:
> + * | xxxx | xxxxxxxxxxxx |
> + *   gen    request tag
> + */
> +#define nvme_genctr_mask(gen)			(gen & 0xf)
> +#define nvme_cid_install_genctr(gen)		(nvme_genctr_mask(gen) << 12)
> +#define nvme_genctr_from_cid(cid)		((cid & 0xf000) >> 12)
> +#define nvme_tag_from_cid(cid)			(cid & 0xfff)
> +
> +static inline u16 nvme_cid(struct request *rq)
> +{
> +	return nvme_cid_install_genctr(nvme_req(rq)->genctr) | rq->tag;
> +}
> +
> +static inline struct request *nvme_find_rq(struct blk_mq_tags *tags,
> +		u16 command_id)
> +{
> +	u8 genctr = nvme_genctr_from_cid(command_id);
> +	u16 tag = nvme_tag_from_cid(command_id);
> +	struct request *rq;
> +
> +	rq = blk_mq_tag_to_rq(tags, tag);
> +	if (unlikely(!rq)) {
> +		pr_err("could not locate request for tag %#x\n",
> +			tag);
> +		return NULL;
> +	}
> +	if (unlikely(nvme_genctr_mask(nvme_req(rq)->genctr) != genctr)) {
> +		dev_err(nvme_req(rq)->ctrl->device,
> +			"request %#x genctr mismatch (got %#x expected %#x)\n",
> +			tag, genctr, nvme_req(rq)->genctr);
> +		return NULL;
> +	}
> +	return rq;
> +}
> +
> +static inline struct request *nvme_cid_to_rq(struct blk_mq_tags *tags,
> +                u16 command_id)
> +{
> +	return blk_mq_tag_to_rq(tags, nvme_tag_from_cid(command_id));
> +}
> +
>  #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
>  void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
>  			    const char *dev_name);

What about raising NVMF_MAX_QUEUE_SIZE to 4096?

Other than that:

Reviewed-by: Hannes Reinecke <hare at suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		        Kernel Storage Architect
hare at suse.de			               +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)