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

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

Posted by Chanwoo Lee 1 week, 5 days ago

ufshcd_tag_to_cmd() may return NULL if no command is associated with
the given tag. However, several callers dereference the returned cmd
pointer via scsi_cmd_priv() without checking for NULL first, leading
to a potential NULL pointer dereference.

Fix this by adding NULL checks for cmd before calling scsi_cmd_priv()
and moving the lrbp initialization after the NULL check

Signed-off-by: Chanwoo Lee <cw9316.lee@samsung.com>
---
 drivers/ufs/core/ufs-mcq.c | 14 +++++++++++---
 drivers/ufs/core/ufshcd.c  | 17 ++++++++++++++---
 2 files changed, 25 insertions(+), 6 deletions(-)

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);
+
 	hwq = ufshcd_mcq_req_to_hwq(hba, rq);
 	if (!hwq)
 		return 0;
@@ -637,7 +640,7 @@ static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba,
 				  struct ufs_hw_queue *hwq, int task_tag)
 {
 	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
-	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+	struct ufshcd_lrb *lrbp;
 	struct utp_transfer_req_desc *utrd;
 	__le64  cmd_desc_base_addr;
 	bool ret = false;
@@ -647,6 +650,11 @@ static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba,
 	if (hba->quirks & UFSHCD_QUIRK_MCQ_BROKEN_RTC)
 		return true;
 
+	if (!cmd)
+		return false;
+
+	lrbp = scsi_cmd_priv(cmd);
+
 	mutex_lock(&hwq->sq_mutex);
 
 	ufshcd_mcq_sq_stop(hba, hwq);
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();
@@ -7893,8 +7895,12 @@ static void ufshcd_set_req_abort_skip(struct ufs_hba *hba, unsigned long bitmap)
 
 	for_each_set_bit(tag, &bitmap, hba->nutrs) {
 		struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, tag);
-		struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+		struct ufshcd_lrb *lrbp;
+
+		if (!cmd)
+			continue;
 
+		lrbp = scsi_cmd_priv(cmd);
 		lrbp->req_abort_skip = true;
 	}
 }
@@ -7915,11 +7921,16 @@ static void ufshcd_set_req_abort_skip(struct ufs_hba *hba, unsigned long bitmap)
 int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
 {
 	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, tag);
-	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+	struct ufshcd_lrb *lrbp;
 	int err;
 	int poll_cnt;
 	u8 resp = 0xF;
 
+	if (!cmd)
+		return -EINVAL;
+
+	lrbp = scsi_cmd_priv(cmd);
+
 	for (poll_cnt = 100; poll_cnt; poll_cnt--) {
 		err = ufshcd_issue_tm_cmd(hba, lrbp->lun, tag, UFS_QUERY_TASK,
 					  &resp);
-- 
2.43.0

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

Posted by Chanwoo Lee 1 week, 4 days ago

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.

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

Posted by Bart Van Assche 1 week, 4 days ago

On 5/27/26 5:43 PM, Chanwoo Lee wrote:
> 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.

The remaining changes look good to me.

Thanks,

Bart.

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

Posted by Bart Van Assche 1 week, 5 days ago

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.