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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Reporting option 3 fails to ignore the REQUESTED SERVICE ACTION field for commands that do not have service actions.
- [Medium] Validation for reporting options 1 and 2 incorrectly handles unsupported commands by conflating 'unsupported' with 'no service action'.
--

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

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 dca46e7779b64..157a4b6979a89 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3774,6 +3774,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])) {

[Severity: Medium]
Does ata_scsi_supported_cmd_use_sa() accurately distinguish between an
unsupported command and a command that lacks a service action?

If a host queries an unsupported command that architecturally does have
a service action (such as opcode 0x5e), it appears the helper would return
false because the command is simply not in the supported list.

For option 2, this causes the command to incorrectly fail with a check
condition (invalid field in cdb) instead of gracefully returning 'Not
supported' in the data phase.

> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}
> +		fallthrough;
>  	case 3:
>  		/* One command format */
>  		sa = get_unaligned_be16(&cdb[4]);

[Severity: Medium]
Will this unconditionally read and use the service action field for
reporting option 3 even when the command doesn't have a service action?

According to the specification, the requested service action field should
be ignored for commands without service actions. If a host provides a
non-zero garbage value in this field for a command without a service action,
passing this value into ata_scsi_cmd_is_supported() might cause the lookup
to fail and incorrectly report the command as unsupported.

>  		break;

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