Re:Re: Re: [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()

[kensanya]

At 2026-07-26 15:55:11, "Leon Romanovsky" <leon@xxxxxxxxxx> wrote:
>On Fri, Jul 24, 2026 at 10:42:53AM +0800, kensanya wrote:
>> At 2026-07-23 17:51:20, "Leon Romanovsky" <leon@xxxxxxxxxx> wrote:
>> >On Thu, Jul 23, 2026 at 10:44:56AM +0800, kensanya@xxxxxxx wrote:
>> >> From: TanZheng <tanzheng@xxxxxxxxxx>
>> >> 
>> >> If srpt_get_desc_tbl() fails, srpt_handle_cmd() jumps to
>> >> target_send_busy() before target_init_cmd() has set cmd->se_tfo.
>> >> target_send_busy() then dereferences a NULL se_tfo.
>> >> 
>> >> Call target_init_cmd() first with placeholder data_length and
>> >> data_direction, parse the descriptor table next, then fill in the
>> >> real data_length. That way target_send_busy() is safe if parsing
>> >> fails.
>> >> 
>> >> target_send_busy() is only valid after a successful target_init_cmd().
>> >> If target_init_cmd() itself fails, free the send ioctx tag locally
>> >> with target_free_tag() instead of calling target_send_busy().
>> >> 
>> >> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF submission failure")
>> >> Link: https://lore.kernel.org/all/20260717025402.64054-1-kensanya@xxxxxxx/
>> >> Suggested-by: Bart Van Assche <bvanassche@xxxxxxx>
>> >> Signed-off-by: TanZheng <tanzheng@xxxxxxxxxx>
>> >> ---
>> >> v2:
>> >> - On target_init_cmd() failure, use target_free_tag() instead of
>> >>   target_send_busy()
>> >> 
>> >>  drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
>> >>  1 file changed, 26 insertions(+), 10 deletions(-)
>> >> 
>> >> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> >> index f66cfd70c263..71af1fb0d380 100644
>> >> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> >> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> >> @@ -1558,8 +1558,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>> >>  	struct srp_cmd *srp_cmd;
>> >>  	struct scatterlist *sg = NULL;
>> >>  	unsigned sg_cnt = 0;
>> >> -	u64 data_len;
>> >> -	enum dma_data_direction dir;
>> >> +	u64 data_len = 0;
>> >> +	enum dma_data_direction dir = DMA_NONE;
>> >>  	int rc;
>> >>  
>> >>  	BUG_ON(!send_ioctx);
>> >> @@ -1584,6 +1584,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>> >>  		break;
>> >>  	}
>> >>  
>> >> +	/*
>> >> +	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
>> >> +	 * cmd->se_tfo is set if descriptor parsing fails and
>> >> +	 * target_send_busy() is used. data_length is filled in after a
>> >> +	 * successful srpt_get_desc_tbl().
>> >> +	 *
>> >> +	 * target_send_busy() is only valid after a successful
>> >> +	 * target_init_cmd(). On init failure free the tag locally.
>> >> +	 */
>> >> +	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
>> >> +			     scsilun_to_int(&srp_cmd->lun), 0 /* data_len */,
>> >> +			     TCM_SIMPLE_TAG, DMA_NONE, TARGET_SCF_ACK_KREF);
>> >
>> >1. TCM_SIMPLE_TAG -> you need to rebase to latest rdma-next.
>> 
>>       I will modify and resend the patch.
>> 
>> >2. You are hardcoding dir, while it can be overwritten in srpt_get_desc_tbl().
>> 
>>       In the srpt_handle_cmd() function:
>>       &send_ioctx->cmd;
>>       srpt_get_desc_tbl(..., send_ioctx, ...) 
>>            ioctx->cmd.data_direction = *dir;
>> 
>>      Therefore, after successfully calling srpt_get_desc_tbl(), even if target_init_cmd() is
>>      passed DMA_NONE, cmd->data_direction should already have stored the actual direction.
>> 
>>      Do you mean  that after calling srpt_get_desc_tbl(), displaying the overwritten content
>>      would be better？I want to make sure I address the intent of your comment correctly.
>
>You need to do this correctly: target_init_cmd() must receive the
>correct directory, as calculated by srpt_get_desc_tbl().
>
>Thanks
>
>> >
>> >Thanks
>> 
>> Thanks,
>> TanZheng

There is a contradiction between the two suggestions:
- Bart suggests calling target_init_cmd() before calling
   srpt_get_desc_tbl() (using a placeholder) to ensure that
   target_send_busy() can be safely executed if the
   descriptor parsing fails.
- Leon points out that target_init_cmd() must receive the
  actual data direction calculated by srpt_get_desc_tbl(),
  rather than a placeholder like DMA_NONE that will
  be overwritten later.

If we continue to modify the current patch, the only way I
can think of is to extract the direction parsing logic from
srpt_get_desc_tbl and place it before target_init_cmd, so
that target_init_cmd can receive the correct direction.
How about this?

Thanks,
TanZheng