Re: [PATCH v2 3/5] ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Improper handling of the REQUESTED SERVICE ACTION field for reporting option 3 in REPORT SUPPORTED OPERATION CODES.
--

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

ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

This commit updates ata_scsi_report_supported_opcodes() to consider the
service action specified in the SCSI command CDB. It adds support for failing
reporting options 1 if the opcode has service actions, and matches opcodes
with their service actions for reporting option 3.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4f0ae44f59dd2..86ac41a7d81ab 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3642,32 +3642,46 @@ static const struct ata_scsi_cmd ata_supported_cmds[] = {
>  	{ SECURITY_PROTOCOL_OUT, 12,	false,	0 },
>  };
>  
> -static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op)
> +static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op, u16 sa)
>  {
>  	const struct ata_scsi_cmd *cmd;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
>  		cmd = &ata_supported_cmds[i];
> -		if (cmd->op == op)
> +		if (cmd->op == op && cmd->sa == sa)

[Severity: Medium]
If the queried command does not have a service action, its cmd->sa will be 0.
If a non-zero service action was passed in through sa, this strict comparison
will fail.

Since SPC-5 states that the REQUESTED SERVICE ACTION field "shall be ignored"
when the operation code does not have service actions, could this result in
valid commands being incorrectly reported as unsupported?

>  			return cmd;
>  	}
>  
>  	return NULL;
>  }

[ ... ]

> @@ -3713,15 +3727,28 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
>  {
>  	struct ata_scsi_cmd_support sup;
>  	u8 *cdb = cmd->cmnd;
> -
> -	if (cdb[2] != 1 && cdb[2] != 3) {
> +	u16 sa = 0;
> +
> +	switch (cdb[2]) {
> +	case 1:
> +		/* One command format with command support data, ignore sa. */
> +		if (ata_scsi_supported_cmd_use_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}
> +		break;
> +	case 3:
> +		/* One command format */
> +		sa = get_unaligned_be16(&cdb[4]);
> +		break;

[Severity: Medium]
If the queried command (e.g., READ_10) doesn't use service actions, does
extracting the service action unconditionally here pass a non-zero value to
ata_scsi_get_supported_cmd() if the initiator provided one?

Should sa remain 0 if ata_scsi_supported_cmd_use_sa(cdb[3]) is false?

> +	default:
>  		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
>  		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
>  		return 0;
>  	}

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