[PATCH v2] scsi: ufs: Change the error check sequence for auto hibern8

[PATCH v2] scsi: ufs: Change the error check sequence for auto hibern8

[HOYOUNG SEO]

In the case of auto hibern8, hibern8 entering or exiting occurs
without a cmd, so it is not correct to check
for the presence of a cmd before checking for auto hibern8 errors.
Thus, the order of the auto hibern8 error check changed.

Change in v2:
 - changed to check UFSHCD_UIC_HIBERN8_MASK for auto hibern
 only when cmd is null.
Separated from the case of cmd, there will be no problem
in the normal processing situation of cmd.
In the case of retval, when UFSHCD_UIC_HIBERN8_MASK is set,
the ufshcd_check_errors() function is performed
by hba->error and IRQ_HANDLED is set in the retval.

Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bcc2512bf34b..4f4380c93ce7 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;
+		}
 	}
 
-	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
-		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
-
 	if (intr_status & UIC_COMMAND_COMPL) {
 		/*
 		 * Store the UIC command result in the lowest byte of
-- 
2.34.1

Re: [PATCH v2] scsi: ufs: Change the error check sequence for auto hibern8

[sashiko-bot]

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