[PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

The kernel log indicates a crash in ufshcd_add_command_trace,
due to a NULL pointer dereference when accessing hwq->id.
This can happen if ufshcd_mcq_req_to_hwq() returns NULL.

This patch adds a NULL check for hwq before accessing its
id field to prevent a kernel crash.

Kernel log excerpt:
[<ffffffd5d192dc4c>] notify_die+0x4c/0x8c
[<ffffffd5d1814e58>] __die+0x60/0xb0
[<ffffffd5d1814d64>] die+0x4c/0xe0
[<ffffffd5d181575c>] die_kernel_fault+0x74/0x88
[<ffffffd5d1864db4>] __do_kernel_fault+0x314/0x318
[<ffffffd5d2a3cdf8>] do_page_fault+0xa4/0x5f8
[<ffffffd5d2a3cd34>] do_translation_fault+0x34/0x54
[<ffffffd5d1864524>] do_mem_abort+0x50/0xa8
[<ffffffd5d2a297dc>] el1_abort+0x3c/0x64
[<ffffffd5d2a29718>] el1h_64_sync_handler+0x44/0xcc
[<ffffffd5d181133c>] el1h_64_sync+0x80/0x88
[<ffffffd5d255c1dc>] ufshcd_add_command_trace+0x23c/0x320
[<ffffffd5d255bad8>] ufshcd_compl_one_cqe+0xa4/0x404
[<ffffffd5d2572968>] ufshcd_mcq_poll_cqe_lock+0xac/0x104
[<ffffffd5d11c7460>] ufs_mtk_mcq_intr+0x54/0x74 [ufs_mediatek_mod]
[<ffffffd5d19ab92c>] __handle_irq_event_percpu+0xc8/0x348
[<ffffffd5d19abca8>] handle_irq_event+0x3c/0xa8
[<ffffffd5d19b1f0c>] handle_fasteoi_irq+0xf8/0x294
[<ffffffd5d19aa778>] generic_handle_domain_irq+0x54/0x80
[<ffffffd5d18102bc>] gic_handle_irq+0x1d4/0x330
[<ffffffd5d1838210>] call_on_irq_stack+0x44/0x68
[<ffffffd5d183af30>] do_interrupt_handler+0x78/0xd8
[<ffffffd5d2a29c00>] el1_interrupt+0x48/0xa8
[<ffffffd5d2a29ba8>] el1h_64_irq_handler+0x14/0x24
[<ffffffd5d18113c4>] el1h_64_irq+0x80/0x88
[<ffffffd5d2527fb4>] arch_local_irq_enable+0x4/0x1c
[<ffffffd5d25282e4>] cpuidle_enter+0x34/0x54
[<ffffffd5d195a678>] do_idle+0x1dc/0x2f8
[<ffffffd5d195a7c4>] cpu_startup_entry+0x30/0x3c
[<ffffffd5d18155c4>] secondary_start_kernel+0x134/0x1ac
[<ffffffd5d18640bc>] __secondary_switched+0xc4/0xcc

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ec175a099459..44efb03765b9 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -515,8 +515,8 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, struct scsi_cmnd *cmd,
 
 	if (hba->mcq_enabled) {
 		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
-
-		hwq_id = hwq->id;
+		if (hwq)
+			hwq_id = hwq->id;
 	} else {
 		doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	}
-- 
2.45.2

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Bart Van Assche]

On 2/22/26 10:56 PM, peter.wang@mediatek.com wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ec175a099459..44efb03765b9 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -515,8 +515,8 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, struct scsi_cmnd *cmd,
>   
>   	if (hba->mcq_enabled) {
>   		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
> -
> -		hwq_id = hwq->id;
> +		if (hwq)
> +			hwq_id = hwq->id;
>   	} else {
>   		doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>   	}

This will cause "-1" to be assigned to hwq_id instead of a queue number
if a request has already been completed. Wouldn't it be better to
introduce a new helper function that returns READ_ONCE(req->mq_hctx)
->queue_num instead of making the above change?

Thanks,

Bart.

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Peter Wang (王信友)]

On Mon, 2026-02-23 at 09:13 -0800, Bart Van Assche wrote:
> This will cause "-1" to be assigned to hwq_id instead of a queue
> number
> if a request has already been completed. Wouldn't it be better to
> introduce a new helper function that returns READ_ONCE(req->mq_hctx)
> ->queue_num instead of making the above change?
> 
> Thanks,
> 
> Bart.

Hi Bart,

The default value of hwq_id is 0:
u32 hwq_id = 0;

Additionally, since READ_ONCE(req->mq_hctx) could be NULL,
ufshcd_mcq_req_to_hwq already takes this into account, as shown below:

struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
                                           struct request *req)
{
    struct blk_mq_hw_ctx *hctx = READ_ONCE(req->mq_hctx);

    return hctx ? &hba->uhq[hctx->queue_num] : NULL;
}

Therefore, there is no need to assign hwq_id separately.

Thanks
Peter

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Bart Van Assche]

On 2/23/26 9:29 PM, Peter Wang (王信友) wrote:
> Therefore, there is no need to assign hwq_id separately.

Hi Peter,

That's not what I proposed. When ignoring the NULL test, this is what
the current ufshcd_add_command_trace() implementation does:

	hwq_id = hba->uhq[READ_ONCE(req->mq_hctx)->queue_num].id;

That's more complicated than necessary. This should be sufficient
(again ignoring the NULL test):

	hwq_id = READ_ONCE(req->mq_hctx)->queue_num;

Anyway, since the proposed change probably only results in a small
performance improvement, let's proceed with the current patch.

Bart.

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Bart Van Assche]

On 2/22/26 10:56 PM, peter.wang@mediatek.com wrote:
> Kernel log excerpt:
> [ ... ]
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>

Bug fixes should have a Fixes: tag. Once that tag has been added, feel
free to add:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Martin K. Petersen]

On Mon, 23 Feb 2026 14:56:09 +0800, peter.wang@mediatek.com wrote:

> The kernel log indicates a crash in ufshcd_add_command_trace,
> due to a NULL pointer dereference when accessing hwq->id.
> This can happen if ufshcd_mcq_req_to_hwq() returns NULL.
> 
> This patch adds a NULL check for hwq before accessing its
> id field to prevent a kernel crash.
> 
> [...]

Applied to 7.0/scsi-fixes, thanks!

[1/1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()
      https://git.kernel.org/mkp/scsi/c/30df81f2228d

-- 
Martin K. Petersen

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Peter Wang (王信友)]

On Tue, 2026-02-24 at 08:48 -0800, Bart Van Assche wrote:
> Hi Peter,
> 
> That's not what I proposed. When ignoring the NULL test, this is what
> the current ufshcd_add_command_trace() implementation does:
> 
>         hwq_id = hba->uhq[READ_ONCE(req->mq_hctx)->queue_num].id;
> 
> That's more complicated than necessary. This should be sufficient
> (again ignoring the NULL test):
> 
>         hwq_id = READ_ONCE(req->mq_hctx)->queue_num;
> 
> Anyway, since the proposed change probably only results in a small
> performance improvement, let's proceed with the current patch.
> 
> Bart.

Hi Bart,

OK, I understand your point. However, there really shouldn’t
be a significant performance difference.
As for the patch with the fixes tag, I don’t think it’s 
necessarily an bug in that patch. It seems more like a corner
case that only occurs after errors happen consecutively.
Also, since Martin has already applied it, I won’t add the fixes
tag for now.

Thanks for the review.
Peter

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Bart Van Assche]

On 2/24/26 6:08 PM, Martin K. Petersen wrote:
> Applied to 7.0/scsi-fixes, thanks!
> 
> [1/1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()
>        https://git.kernel.org/mkp/scsi/c/30df81f2228d
Hi Martin,

Do you agree that the following should be added to this patch?

Fixes: 4a52338bf288 ("scsi: ufs: core: Add trace event for MCQ")

 From commit 4a52338bf288:

@@ -456,9 +458,16 @@ static void ufshcd_add_command_trace(struct ufs_hba 
*hba, unsigned int tag,
         }

         intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-       doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+
+       if (is_mcq_enabled(hba)) {
+               struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
+
+               hwq_id = hwq->id;
+       } else {
+               doorbell = ufshcd_readl(hba, 
REG_UTP_TRANSFER_REQ_DOOR_BELL);
+       }
         trace_ufshcd_command(dev_name(hba->dev), str_t, tag,
-                       doorbell, transfer_len, intr, lba, opcode, 
group_id);
+                       doorbell, hwq_id, transfer_len, intr, lba, 
opcode, group_id);
  }

Thanks,

Bart.

Re: [PATCH v1] ufs: core: Fix possible NULL pointer dereference in ufshcd_add_command_trace()

[Martin K. Petersen]

Hi Bart!

> Do you agree that the following should be added to this patch?
>
> Fixes: 4a52338bf288 ("scsi: ufs: core: Add trace event for MCQ")

If I hadn't already applied the patch I would add the tag. However, with
"Fix" and "NULL pointer dereference" in the title, hopefully some AI
will pick it up.

Worst case we can Cc: stable once Linus has pulled it...

-- 
Martin K. Petersen