Re: [PATCH] scsi: ufs: core: Fix NULL pointer dereference in scsi_cmd_priv() calls

[Chanwoo Lee]

On 5/27/26 16:14, Bart Van Assche wrote:
>On 5/27/26 12:22 AM, Chanwoo Lee wrote:
>> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
>> index c1b1d67a1ddc..798b2a910128 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,9 @@ 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);
>> +
>
>These changes are not necessary. Although scsi_cmd_priv() and
>scsi_cmd_to_rq() both return an invalid pointer if their argument is
>NULL, these pointers are not dereferenced before the cmd != NULL check.
>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 9e0336098e26..0371dea44887 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -5833,13 +5833,15 @@ 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;
>>   
>>   	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
>>   		      le64_to_cpu(cqe->command_desc_base_addr)))
>>   		return;
>>   
>> +	lrbp = scsi_cmd_priv(cmd);
>> +
>>   	if (hba->monitor.enabled) {
>>   		lrbp->compl_time_stamp = ktime_get();
>>   		lrbp->compl_time_stamp_local_clock = local_clock();
>
>These changes are not necessary either because lrbp is not dereferenced
>before the cmd != NULL check.
>
>Thanks,
>
>Bart.

Thank you for the review. You're right that there is no runtime
crash since scsi_cmd_priv() and scsi_cmd_to_rq() only perform
pointer arithmetic without dereferencing, and the derived pointers
are not used before the NULL check.

I made these changes because it felt logically awkward to derive
values from a potentially NULL pointer, even if they aren't
dereferenced before the check. But I understand your point and
will drop these two changes.

Could you let me know if you think the remaining changes are
acceptable, or if you consider the entire patch unnecessary?
I'd like to clarify this before sending v2.

Thanks,
Chanwoo Lee.