* Re: [PATCH v2] scsi: ufs: Change the error check sequence for auto hibern8
2026-10-22 7:42 ` [PATCH v2] scsi: ufs: Change the error check sequence for auto hibern8 HOYOUNG SEO
@ 2026-10-22 7:57 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-22 7:57 UTC (permalink / raw)
To: HOYOUNG SEO, linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ufs: NULL pointer dereference in ufshcd_uic_cmd_compl() on auto-hibern8 error
- [High] scsi: ufs: missed auto-hibern8 error detection in ufshcd_uic_cmd_compl() when cmd is active
--
commit b23209e884c306055d4141ae75fa4129efd7affa
Author: HOYOUNG SEO <hy50.seo@samsung.com>
scsi: ufs: Change the error check sequence for auto hibern8
This commit moves the ufshcd_is_auto_hibern8_error() check in
ufshcd_uic_cmd_compl() into the !cmd branch so auto-hibern8 errors can be
recorded when no UIC command is active.
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c65021..4d44dbcda6423 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5930,14 +5930,15 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> guard(spinlock_irqsave)(&hba->host->host_lock);
> cmd = hba->active_uic_cmd;
> if (!cmd) {
> - dev_err(hba->dev,
> - "No active UIC command. Maybe a timeout occurred?\n");
> - return retval;
> + if (ufshcd_is_auto_hibern8_error(hba, intr_status)) {
> + hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> + } else {
> + dev_err(hba->dev,
> + "No active UIC command. Maybe a timeout occurred?\n");
> + return retval;
> + }
> }
[Severity: High]
When cmd is NULL and ufshcd_is_auto_hibern8_error() returns true, execution
falls through to the remainder of ufshcd_uic_cmd_compl() with cmd still NULL:
drivers/ufs/core/ufshcd.c:ufshcd_uic_cmd_compl() {
...
if (intr_status & UIC_COMMAND_COMPL) {
cmd->argument2 |= ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2) &
MASK_UIC_COMMAND_RESULT;
cmd->argument3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
if (!hba->uic_async_done)
cmd->cmd_active = false;
complete(&cmd->done);
retval = IRQ_HANDLED;
}
if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
cmd->cmd_active = false;
complete(hba->uic_async_done);
retval = IRQ_HANDLED;
}
}
If UIC_COMMAND_COMPL is also set in intr_status (for example, if a prior UIC
command timed out in ufshcd_wait_for_uic_cmd() and cleared
hba->active_uic_cmd to NULL before a late completion interrupt arrived
alongside an auto-hibern8 error), won't dereferencing cmd->argument2 cause a
NULL pointer dereference?
Similarly, UFSHCD_UIC_PWR_MASK includes UFSHCD_UIC_HIBERN8_MASK, so
intr_status & UFSHCD_UIC_PWR_MASK is always true on this path. In
ufshcd_uic_pwr_ctrl(), hba->uic_async_done is assigned and host_lock is
released before __ufshcd_send_uic_cmd() sets hba->active_uic_cmd:
drivers/ufs/core/ufshcd.c:ufshcd_uic_pwr_ctrl() {
...
hba->uic_async_done = &uic_async_done;
ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
spin_unlock_irqrestore(&hba->host->host_lock, flags);
ret = __ufshcd_send_uic_cmd(hba, cmd);
...
}
If an auto-hibern8 error interrupt is serviced in that window while cmd is
NULL, won't cmd->cmd_active = false also dereference a NULL cmd pointer?
Should the auto-hibern8 error branch return retval after setting hba->errors?
>
> - if (ufshcd_is_auto_hibern8_error(hba, intr_status))
> - hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> -
[Severity: High]
ufshcd_is_auto_hibern8_error() is only called from ufshcd_uic_cmd_compl(),
and it explicitly checks whether hba->active_uic_cmd is a manual hibernate
command:
drivers/ufs/core/ufshcd.c:ufshcd_is_auto_hibern8_error() {
...
if (hba->active_uic_cmd &&
(hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_ENTER ||
hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_EXIT))
return false;
return true;
}
By moving the ufshcd_is_auto_hibern8_error() check inside if (!cmd), that
hba->active_uic_cmd check in ufshcd_is_auto_hibern8_error() becomes
unreachable because hba->active_uic_cmd is always NULL at the call site.
If an auto-hibern8 enter or exit error occurs while a non-hibernate UIC
command (such as UIC_CMD_DME_GET, UIC_CMD_DME_PEER_GET, or UIC_CMD_DME_SET)
is active in hba->active_uic_cmd, hba->errors will no longer be updated with
UFSHCD_UIC_HIBERN8_MASK & intr_status. Because UFSHCD_UIC_HIBERN8_MASK is not
part of UFSHCD_ERROR_MASK, ufshcd_sl_intr() relies on hba->errors being set
to invoke ufshcd_check_errors():
drivers/ufs/core/ufshcd.c:ufshcd_sl_intr() {
if (intr_status & UFSHCD_UIC_MASK)
retval |= ufshcd_uic_cmd_compl(hba, intr_status);
if (intr_status & UFSHCD_ERROR_MASK || hba->errors)
retval |= ufshcd_check_errors(hba, intr_status);
...
}
In addition, if a power mode command is active in ufshcd_uic_pwr_ctrl() with
hba->uic_async_done set, an auto-hibern8 error interrupt will match
intr_status & UFSHCD_UIC_PWR_MASK and complete hba->uic_async_done without
recording the error in hba->errors.
Could ufshcd_is_auto_hibern8_error() be checked before the if (!cmd) check so
that auto-hibern8 errors are caught both when cmd is NULL and when a
non-hibernate UIC command is active?
> if (intr_status & UIC_COMMAND_COMPL) {
> /*
> * Store the UIC command result in the lowest byte of
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022074249.3098100-1-hy50.seo@samsung.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread