[PATCH] nvmet/tcp: Solve coredump issue while initiator sends write CapsuleCmd

[Yang, Ziye]

Hi Sagi,

No problem. Let me revise the comments and use your commit message  instead. 




Best Regards
Ziye Yang 

-----Original Message-----
From: Sagi Grimberg <sagi at grimberg.me> 
Sent: Friday, August 21, 2020 1:58 PM
To: Yang, Ziye <ziye.yang at intel.com>; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] nvmet/tcp: Solve coredump issue while initiator sends write CapsuleCmd

Hey Ziye,

> When the initiator sneds write CapsuleCmd without incapsuledata, we
		     sends                          in-capsule data

> can use the queue->connect command to handle. If we do not do that, we 
> will have a coredump because queue->cmds are not initialized.

This issue is specific to the I/O queue connect where the payload comes solicited via r2t.

I suggest to document the change log to the following:

nvmet-tcp: Fix NULL dereference when a connect data comes in h2cdata pdu

When handling commands without in-capsule data, we assign the ttag assuming we already have the queue commands array allocated (based on the queue size information in the connect data payload).

However if the connect itself did not send the connect data in-capsule we have yet to allocate the queue commands, and we will assign a bogus ttag and suffer a NULL dereference when we receive the corresponding h2cdata pdu.

Fix this by checking if we already allocated commands before dereferencing it when handling h2cdata, if we didn't, its for sure a connect and we should use the preallocated connect command.

> Signed-off-by: Ziye Yang <ziye.yang at intel.com>
> ---
>   drivers/nvme/target/tcp.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c 
> index 9eda91162fe4..9d3eeb9338cc 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -866,7 +866,11 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue)
>   	struct nvme_tcp_data_pdu *data = &queue->pdu.data;
>   	struct nvmet_tcp_cmd *cmd;
>   
> -	cmd = &queue->cmds[data->ttag];
> +	if (likely(queue->nr_cmds)) {
> +		cmd = &queue->cmds[data->ttag];
> +	} else {
> +		cmd = &queue->connect;
> +	}

No need for brackets on one-liner conditionals.

Also, I think it'd make sense to set the ttag to 0xffff if we hit this condition, otherwise we are sending a bogus ttag.

Something like:
--
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c index dfcbc6522aba..3fec5119ec90 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -159,6 +159,10 @@ static void nvmet_tcp_finish_cmd(struct nvmet_tcp_cmd *cmd);
  static inline u16 nvmet_tcp_cmd_tag(struct nvmet_tcp_queue *queue,
                 struct nvmet_tcp_cmd *cmd)
  {
+       if (unlikely(cmd == queue->connect)) {
+               /* We didn't allocate cmds yet, send 0xffff */
+               return USHRT_MAX;
+       }
         return cmd - queue->cmds;
  }
--