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

[Yang, Ziye]

Hi Sagi,

Got it. 




Best Regards
Ziye Yang 

-----Original Message-----
From: Sagi Grimberg <sagi at grimberg.me> 
Sent: Friday, August 21, 2020 11:56 PM
To: Yang, Ziye <ziye.yang at intel.com>; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] 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 | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c 
> index 9eda91162fe4..8e0d766d2722 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -160,6 +160,11 @@ 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(!queue->nr_cmds)) {
> +		/* We didn't allocate cmds yet, send 0xffff */
> +		return USHRT_MAX;
> +	}
> +
>   	return cmd - queue->cmds;
>   }
>   
> @@ -866,7 +871,10 @@ 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;
>   
>   	if (le32_to_cpu(data->data_offset) != cmd->rbytes_done) {
>   		pr_err("ttag %u unexpected data offset %u (expected %u)\n",
> 

Ziye, usually we send versioning of the patches with prefix PATCH v1/2/3,

and also add what was changed since prior versions.

This patch looks good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>