Re: [PATCH 1/1] nvme-tcp: check for invalid request

[Sagi Grimberg <sagi@grimberg.me>]

On 08/04/2024 7:53, soni.ankit@samsung.com wrote:
> From: Ankit Soni <soni.ankit@samsung.com>
>
> The blk_mq_tag_to_rq() returns NULL for invalid tag.
> Added check condition to prevent NULL Pointer derefernece.

Does this fix an actual bug? Or a theoretic one?

>
> Signed-off-by: Ankit Soni <soni.ankit@samsung.com>
> ---
>   drivers/nvme/host/tcp.c | 34 ++++++++++++++++++++++------------
>   1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index fdbcdcedcee9..01a90bbed70b 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -801,9 +801,17 @@ static int nvme_tcp_recv_data(struct nvme_tcp_queue *queue, struct sk_buff *skb,
>   			      unsigned int *offset, size_t *len)
>   {
>   	struct nvme_tcp_data_pdu *pdu = (void *)queue->pdu;
> -	struct request *rq =
> -		nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
> -	struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> +	struct nvme_tcp_request *req;
> +	struct request *rq;
> +
> +	rq = nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
> +	if (unlikely(!rq)) {
> +		pr_err("could not locate request for tag %#x\n",
> +			pdu->command_id);
> +		return -EFAULT;
> +	}
> +
> +	req = blk_mq_rq_to_pdu(rq);

We already checked the validity when processing the c2hpdu, there is no 
need to do
that for all the data payload chunks as well as it does not change.

>   
>   	while (true) {
>   		int recv_len, ret;
> @@ -875,6 +883,8 @@ static int nvme_tcp_recv_ddgst(struct nvme_tcp_queue *queue,
>   	char *ddgst = (char *)&queue->recv_ddgst;
>   	size_t recv_len = min_t(size_t, *len, queue->ddgst_remaining);
>   	off_t off = NVME_TCP_DIGEST_LENGTH - queue->ddgst_remaining;
> +	struct nvme_tcp_request *req;
> +	struct request *rq;
>   	int ret;
>   
>   	ret = skb_copy_bits(skb, *offset, &ddgst[off], recv_len);
> @@ -887,13 +897,17 @@ static int nvme_tcp_recv_ddgst(struct nvme_tcp_queue *queue,
>   	if (queue->ddgst_remaining)
>   		return 0;
>   
> -	if (queue->recv_ddgst != queue->exp_ddgst) {
> -		struct request *rq = nvme_cid_to_rq(nvme_tcp_tagset(queue),
> -					pdu->command_id);
> -		struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> +	rq = nvme_cid_to_rq(nvme_tcp_tagset(queue), pdu->command_id);
> +	if (unlikely(!rq)) {
> +		pr_err("could not locate request for tag %#x\n",
> +			pdu->command_id);
> +		return -EFAULT;
> +	}

Same here, the ddgst comes as a trailer to a c2hdata pdu which we already
checked the validity, you are simply taking an already validated pdu and 
using
it. It is redundant.

>   
> -		req->status = cpu_to_le16(NVME_SC_DATA_XFER_ERROR);
> +	req = blk_mq_rq_to_pdu(rq);
>   
> +	if (queue->recv_ddgst != queue->exp_ddgst) {
> +		req->status = cpu_to_le16(NVME_SC_DATA_XFER_ERROR);
>   		dev_err(queue->ctrl->ctrl.device,
>   			"data digest error: recv %#x expected %#x\n",
>   			le32_to_cpu(queue->recv_ddgst),
> @@ -901,10 +915,6 @@ static int nvme_tcp_recv_ddgst(struct nvme_tcp_queue *queue,
>   	}
>   
>   	if (pdu->hdr.flags & NVME_TCP_F_DATA_SUCCESS) {
> -		struct request *rq = nvme_cid_to_rq(nvme_tcp_tagset(queue),
> -					pdu->command_id);
> -		struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> -
>   		nvme_tcp_end_request(rq, le16_to_cpu(req->status));
>   		queue->nr_cqe++;
>   	}