Re: [PATCH v2 4/5] ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Reporting options 1 and 2 incorrectly terminate with CHECK CONDITION (INVALID FIELD IN CDB) for valid queries, violating SCSI protocol.
- [Medium] Reporting option 3 incorrectly evaluates the REQUESTED SERVICE ACTION field for commands that do not use service actions.
--

commit 8b0fcbdec2fbb5690930a99f6f8a7faf9e8bd511
Author: Damien Le Moal <[email protected]>

ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES
    
Add support for the reporting options value 2 of the in REPORT SUPPORTED
OPERATION CODES command in ata_scsi_report_supported_opcodes(). With this
reporting option, the specified opcode to check must have a service
action. Use ata_scsi_supported_cmd_has_sa() to check for this and fill the
command reply buffer using the one command format based on the command
support.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 86ac41a7d81ab..e75ed573e234b 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3737,6 +3737,13 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
>  			return 0;
>  		}
>  		break;
> +	case 2:
> +		/* One command format, must have sa. */
> +		if (!ata_scsi_supported_cmd_use_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}

[Severity: Medium]
Does this validation check incorrectly abort execution with a SCSI error for
option 2 commands that are unknown?

According to SPC-5, option 2 queries for unsupported commands (and similarly,
option 1 queries for commands with service actions) must return SUPPORT=001b.
Instead, this implementation erroneously aborts the command by calling
ata_scsi_set_invalid_field(), preventing the code from falling through and
correctly populating the response buffer.

> +		fallthrough;
>  	case 3:
>  		/* One command format */
>  		sa = get_unaligned_be16(&cdb[4]);

[Severity: Medium]
If an initiator issues REPORT SUPPORTED OPERATION CODES with option 3 for a
standard command that lacks service actions, while providing non-zero data in
the requested service action field, wouldn't this unconditional extraction of
sa cause a regression?

SPC-5 dictates that if the requested opcode does not have service actions, the
requested service action field must be ignored. By unconditionally extracting
sa here, it will fail to match in ata_scsi_get_supported_cmd() since
non-service-action commands expect sa == 0, improperly yielding SUPPORT=001b
for a valid, supported command.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4