| Message ID | 20250418075517.369098-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | CDL Feature control improvements | expand |
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 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
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
On 18 April 2025 11:30:35 CEST, Damien Le Moal <dlemoal@kernel.org> wrote: >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. > I linked to the wrong line. https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/ata/libata-scsi.c#L4081 The rest of the comment is still valid. This case that this patch tries to fix can already not happen. Kind regards, Niklas >> >> 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 > > >-- >Damien Le Moal >Western Digital Research
On 4/18/25 20:45, Niklas Cassel wrote: > > > On 18 April 2025 11:30:35 CEST, Damien Le Moal <dlemoal@kernel.org> wrote: >> 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. >> > > I linked to the wrong line. > > https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/ata/libata-scsi.c#L4081 > > The rest of the comment is still valid. > > This case that this patch tries to fix can already not happen. You are absolutely correct ! How did I miss that :) Sending V3 with this patch dropped.
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(+)