Re: [PATCH 3/8] ata: libata-scsi: reject MODE SELECT mode pages with a wrong SPF bit

[Damien Le Moal <dlemoal@kernel.org>]

On 11/4/26 05:54, Niklas Cassel wrote:
> ata_scsi_mode_select_xlat() accepts the caching and control mode pages
> both in page_0 format and in sub_page format with SUBPAGE CODE 00h.
> 
> However, SAT-2 and later versions of SAT define the SPF bit of the
> Caching mode page and of the Control mode page as not changeable and
> set to zero (see SAT-6, 10.4.9 and 10.4.6), and SPC-7 (6.12, MODE
> SELECT(10) command) requires the device server to terminate a MODE
> SELECT command with INVALID FIELD IN PARAMETER LIST if the application
> client sets any field that is reported as not changeable to a value
> other than its current value.
> 
> Reject a mode page with the SPF bit set to one and SUBPAGE CODE 00h,
> with the FIELD POINTER set to the byte containing the SPF bit. The only
> mode page with the SPF bit set to one that remains accepted is the ATA
> Feature Control mode page (control mode page, sub-page f2h), for which
> SAT-6 (12.3.4) requires the SPF bit to be set to one. The opposite case
> does not need a check: a mode page in page_0 format always has subpage
> code 00h, so it can never be the ATA Feature Control mode page.
> 
> This also allows the mode page handlers to rely on the format of their
> mode page, which a later patch will make use of.
> 
> Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d7923bc664d6..7b0a02903fda 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4629,6 +4629,16 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>  			goto invalid_param_len;
>  
>  		spg = p[1];
> +		/*
> +		 * Mode pages with subpage code 00h use the page_0 format: SAT
> +		 * defines the SPF bit of the Caching and Control mode pages,
> +		 * for subpage code 00h, as not changeable and set to zero.
> +		 */
> +		if (!spg) {
> +			fp = hdr_len + bd_len;
> +			goto invalid_param;
> +		}

The comment block in-between the assignment and test is odd. Can you move the
assignment right before the if, after the comment block? That makes the code a
lot prettier :)

> +
>  		pg_len = get_unaligned_be16(&p[2]);
>  		p += 4;
>  		len -= 4;


-- 
Damien Le Moal
Western Digital Research