diff mbox series

[v2,2/5] ata: libata-scsi: Fail MODE SELECT for unsupported mode pages

Message ID 20250418075517.369098-3-dlemoal@kernel.org
State New
Headers show
Series CDL Feature control improvements | expand

Commit Message

Damien Le Moal April 18, 2025, 7:55 a.m. UTC
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(+)

Comments

Niklas Cassel April 18, 2025, 8:40 a.m. UTC | #1
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
Damien Le Moal April 18, 2025, 9:30 a.m. UTC | #2
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
Niklas Cassel April 18, 2025, 11:45 a.m. UTC | #3
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
Damien Le Moal April 18, 2025, 11:02 p.m. UTC | #4
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 mbox series

Patch

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;
 		}