Re: [PATCH 2/5] scsi: ufs: Fix NULL dereferences after tag lookup

[Bart Van Assche <bvanassche@acm.org>]

On 7/15/26 11:19 PM, liqiang wrote:
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 13b60a2d06db..15850b0658f9 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -555,8 +555,8 @@ static int ufshcd_mcq_sq_start(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
>   int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>   {
>   	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
> -	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> -	struct request *rq = scsi_cmd_to_rq(cmd);
> +	struct ufshcd_lrb *lrbp;
> +	struct request *rq;
>   	struct ufs_hw_queue *hwq;
>   	void __iomem *reg, *opr_sqd_base;
>   	u32 nexus, id, val;
> @@ -568,6 +568,8 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>   	if (!cmd)
>   		return -EINVAL;
>   
> +	lrbp = scsi_cmd_priv(cmd);
> +	rq = scsi_cmd_to_rq(cmd);
>   	hwq = ufshcd_mcq_req_to_hwq(hba, rq);
>   	if (!hwq)
>   		return 0;

Please drop this change. Calling scsi_cmd_priv(NULL) or
scsi_cmd_to_rq(NULL) is safe if the pointers returned by these functions
are not used.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3541da5b6f4d..f2a5fa624e74 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5857,13 +5857,14 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   			  struct cq_entry *cqe)
>   {
>   	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
> -	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> +	struct ufshcd_lrb *lrbp;
>   	enum utp_ocs ocs;

Same comment here: calling scsi_cmd_priv(NULL) is safe if the returned
pointer is not used as is the case here.

>   
> -	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
> -		      le64_to_cpu(cqe->command_desc_base_addr)))
> +	if (WARN_ONCE(!cmd, "invalid completion tag %d, cqe->command_desc_base_addr = %#llx\n",
> +		      task_tag, cqe ? le64_to_cpu(cqe->command_desc_base_addr) : 0ULL))
>   		return;

The above change looks good to me.

Thanks,

Bart.