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

On 7/2/26 15:52, [email protected] wrote:
> 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.

Sashiko, please reread the SPC specifications. It says:

If the REQUESTED OPERATION CODE field specifies an operation code for
which the device server does not implement service actions a, then the
device server shall terminate the command with CHECK CONDITION
status, with the sense key set to ILLEGAL REQUEST, and the additional
sense code set to INVALID FIELD IN CDB.

Which is exactly what we are testing here.

> 
>> +		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?

No. Service action is not ignored with reporting options 3.

> 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.

No. SPC says:

a) the operation code specified by the REQUESTED OPERATION CODE
field specifies an operation code for which the device server does
not implement service actions, the REQUESTED SERVICE ACTION field
is set to 00h, and the command is supported; or
...
then the command support data shall indicate that the command is
supported (i.e., the SUPPORT field (see table 262) is set to 011b or 101b).
Otherwise, the command support data shall indicate that the command is
not supported (i.e., the SUPPORT field is set to 001b).

Set for commands that do not support service actions, the command must have
service action set to 0.


-- 
Damien Le Moal
Western Digital Research