Re: [PATCH v1 5/6] 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:
- [High] The helper `ata_scsi_supported_cmd_has_sa()` incorrectly assumes that a command with `sa == 0` does not take service actions, causing Reporting Option 2 to reject valid queries for commands like `ZBC_IN`.
- [High] Reporting Option 3 incorrectly fails to ignore the REQUESTED SERVICE ACTION field for commands without service actions.
--

commit ae74931c866547ba834a0558a06a2d04d82842f2
Author: Damien Le Moal <[email protected]>
ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

This commit adds support for the reporting options value 2 of the REPORT
SUPPORTED OPERATION CODES command in ata_scsi_report_supported_opcodes().
It verifies that the opcode to check has a service action using the new
ata_scsi_supported_cmd_has_sa() helper. If supported, it replies 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 d54ec1631e9a6..3f313050c63f3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> +static bool ata_scsi_supported_cmd_has_sa(u8 op)
> +{
> +	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)
> +			return cmd->sa != 0;

[Severity: High]
Does this incorrectly assume that commands with a service action of 0 don't
have a service action at all? 

For example, ZBC_IN has op == 0x53 and its service action ZI_REPORT_ZONES
is 0x00. This helper will return false for ZBC_IN.

> +	}
> +
> +	return false;
> +}

[ ... ]

> +static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
> +						      struct scsi_cmnd *cmd,
> +						      u8 *rbuf)
> +{
> +	struct ata_scsi_cmd_support sup;
> +	u8 *cdb = cmd->cmnd;
> +	u16 sa = 0;
> +
> +	switch (cdb[2]) {
> +	case 1:
> +		/* One command format with command support data, ignore sa. */
> +		if (ata_scsi_supported_cmd_has_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}
> +		break;
> +	case 2:
> +		/* One command format, must have sa. */
> +		if (!ata_scsi_supported_cmd_has_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}

[Severity: High]
Because of the logic in ata_scsi_supported_cmd_has_sa(), if a host issues a
REPORT SUPPORTED OPERATION CODES command with Option 2 and CDB[3] = ZBC_IN,
won't this actively reject a valid zone capabilities query with an Invalid
Field in CDB error?

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

[Severity: High]
When processing Reporting Option 3, is it safe to unconditionally extract the
requested service action without verifying if the command actually takes
service actions?

The SPC specification requires ignoring this field for commands that do not
take service actions. If a host sends an Option 3 query for a command like
READ_16 and leaves uninitialized non-zero data in the ignored service action
field, the exact match lookup in ata_scsi_get_supported_cmd() will fail and
incorrectly report the command as unsupported.

> +		break;
> +	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/[email protected]?part=5