| Message ID | 20250418075517.369098-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | CDL Feature control improvements | expand |
On 4/18/25 17:40, Niklas Cassel wrote: > On Fri, Apr 18, 2025 at 04:55:14PM +0900, Damien Le Moal wrote: >> For devices that do not support CDL, the subpage F2h of the control mode >> page 0Ah should not be supported. However, the function >> ata_mselect_control_ata_feature() does not fail for a device that does >> not have the ATA_DFLAG_CDL device flag set, which can lead to an invalid >> SET FEATURES command (which will be failed by the device) to be issued. >> >> Modify ata_mselect_control_ata_feature() to return -EOPNOTSUPP if it is >> executed for a device without CDL support. This error code is checked by >> ata_scsi_mode_select_xlat() (through ata_mselect_control()) to fail the >> MODE SELECT command immediately with an ILLEGAL REQUEST / INVALID FIELD >> IN CDB asc/ascq as mandated by the SPC specifications for unsupported >> mode pages. >> >> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/ata/libata-scsi.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index 24e662c837e3..15661b05cb48 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -3896,6 +3896,15 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc, >> struct ata_taskfile *tf = &qc->tf; >> u8 cdl_action; >> >> + /* >> + * The sub-page f2h should only be supported for devices that support >> + * the T2A and T2B command duration limits mode pages (note here the >> + * "should" which is what SAT-6 defines). So fail this command if the >> + * device does not support CDL. >> + */ >> + if (!(dev->flags & ATA_DFLAG_CDL)) >> + return -EOPNOTSUPP; >> + >> /* >> * The first four bytes of ATA Feature Control mode page are a header, >> * so offsets in mpage are off by 4 compared to buf. Same for len. >> @@ -4101,6 +4110,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc) >> case CONTROL_MPAGE: >> ret = ata_mselect_control(qc, spg, p, pg_len, &fp); >> if (ret < 0) { >> + if (ret == -EOPNOTSUPP) >> + goto invalid_fld; >> fp += hdr_len + bd_len; >> goto invalid_param; >> } >> -- > > I would prefer if we did not merge this patch, as it is already handled in > higher up in the (only) calling function: > https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/ata/libata-scsi.c#L2582-L2589 This code you point to is for mode sense. This patch deals with mode select, where we are not checking for the subpage support, which is wrong. > > We only break if "dev->flags & ATA_DFLAG_CDL && pg == CONTROL_MPAGE" > > if this expression is false, we do a fallthrough, > which means fp = 3; goto invalid_fld; > > > Kind regards, > Niklas
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 24e662c837e3..15661b05cb48 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3896,6 +3896,15 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc, struct ata_taskfile *tf = &qc->tf; u8 cdl_action; + /* + * The sub-page f2h should only be supported for devices that support + * the T2A and T2B command duration limits mode pages (note here the + * "should" which is what SAT-6 defines). So fail this command if the + * device does not support CDL. + */ + if (!(dev->flags & ATA_DFLAG_CDL)) + return -EOPNOTSUPP; + /* * The first four bytes of ATA Feature Control mode page are a header, * so offsets in mpage are off by 4 compared to buf. Same for len. @@ -4101,6 +4110,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc) case CONTROL_MPAGE: ret = ata_mselect_control(qc, spg, p, pg_len, &fp); if (ret < 0) { + if (ret == -EOPNOTSUPP) + goto invalid_fld; fp += hdr_len + bd_len; goto invalid_param; }
For devices that do not support CDL, the subpage F2h of the control mode page 0Ah should not be supported. However, the function ata_mselect_control_ata_feature() does not fail for a device that does not have the ATA_DFLAG_CDL device flag set, which can lead to an invalid SET FEATURES command (which will be failed by the device) to be issued. Modify ata_mselect_control_ata_feature() to return -EOPNOTSUPP if it is executed for a device without CDL support. This error code is checked by ata_scsi_mode_select_xlat() (through ata_mselect_control()) to fail the MODE SELECT command immediately with an ILLEGAL REQUEST / INVALID FIELD IN CDB asc/ascq as mandated by the SPC specifications for unsupported mode pages. Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)