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

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

[soni.ankit]

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.

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);
 
 	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;
+	}
 
-		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++;
 	}
-- 
2.43.0

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

[Sagi Grimberg]

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++;
>   	}

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

[Chaitanya Kulkarni]

On 4/8/24 04:08, Sagi Grimberg wrote:
>
>
> 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?

if it does fix actual bug then why not add a blktest as I don'
think we have this condition covered because we now triggering
error path, unless I'm missing something ...

-ck