Re: [PATCH v2 34/40] target: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized stack variable read in debug log due to concurrent list modification

Pre-existing issues:
- [High] Logic error causes Unit Attention Interlock to fail and incorrectly return SAM_STAT_BUSY
--

commit 95a802c45c6965b17dd9cccbc6b6421dcf571862
Author: Damien Le Moal <dlemoal@kernel.org>

target: use 16-bits defined sense codes

Refactor the target core code to use the 16-bits sense codes for building
sense and replace all hard-coded additional sense codes and additional
sense code qualifiers with the enum values defined in
include/scsi/scsi_sense.h. This helps with code clarity as the sense codes
being processed are easier to test and self-documented.

No functional change intended.

> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index f964a531e053a..f11766dfb5428 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -1226,9 +1226,9 @@ static sense_reason_t spc_emulate_request_sense(struct se_cmd *cmd)
>  {
>  	unsigned char *cdb = cmd->t_task_cdb;
>  	unsigned char *rbuf;
> -	u8 ua_asc = 0, ua_ascq = 0;
>  	unsigned char buf[SE_SENSE_BUF];
>  	bool desc_format = target_sense_desc_format(cmd->se_dev);
> +	u16 sense_code;

[Severity: Medium]
Could this uninitialized variable cause an issue?

The previous code initialized ua_asc and ua_ascq to 0, but sense_code is
left uninitialized here.

[ ... ]

> diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
> index 4276690fb6cbd..ea97b05d47ab7 100644
> --- a/drivers/target/target_core_ua.c
> +++ b/drivers/target/target_core_ua.c
[ ... ]
> @@ -232,8 +230,7 @@ bool core_scsi3_ua_for_check_condition(struct se_cmd *cmd, u8 *key, u8 *asc,
>  		 * clearing it.
>  		 */
>  		if (!dev_ua_intlck_clear) {
> -			*asc = ua->ua_asc;
> -			*ascq = ua->ua_ascq;
> +			*code = scsi_sense_code(ua->ua_asc, ua->ua_ascq);
>  			break;
>  		}

[Severity: High]
This is a pre-existing issue, but if dev_ua_intlck_clear is false, the loop
breaks early without updating the head variable to 0.

Since head remains 1, core_scsi3_ua_for_check_condition() later returns
head == 0, which evaluates to false.

Does this incorrect return value mislead the target core into aborting the
sense processing in translate_sense_reason() and returning a SAM_STAT_BUSY
status instead of a CHECK CONDITION?

[ ... ]

> @@ -317,7 +310,8 @@ int core_scsi3_ua_clear_for_request_sense(
>  	pr_debug("[%s]: Released UNIT ATTENTION condition, mapped"
>  		" LUN: %llu, got REQUEST_SENSE reported ASC: 0x%02x,"
>  		" ASCQ: 0x%02x\n", nacl->se_tpg->se_tpg_tfo->fabric_name,
> -		cmd->orig_fe_lun, *asc, *ascq);
> +		cmd->orig_fe_lun, scsi_sense_code_asc(*code),
> +		scsi_sense_code_ascq(*code));
>  
>  	return (head) ? -EPERM : 0;
>  }

[Severity: Medium]
Is it possible for this debug log to read the uninitialized stack variable
from spc_emulate_request_sense()?

If the list is cleared concurrently between the lockless list_empty_careful()
check and acquiring the ua_lock, the loop is skipped and *code is never
assigned.

Will this unconditionally pass uninitialized memory to pr_debug() and
potentially trigger a KMSAN warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903034201.112211-1-dlemoal@kernel.org?part=34