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

[Niklas Cassel <cassel@kernel.org>]

On Wed, Nov 04, 2026 at 06:22:08AM +0900, Damien Le Moal wrote:
> 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 :)

Fixed up while applying.


Kind regards,
Niklas