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

[sashiko-bot@kernel.org]

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