linux-kernel - Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 17:49:22 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Hannes Reinecke <hare@...e.de>
Cc: Igor Pylypiv <ipylypiv@...gle.com>, Damien Le Moal <dlemoal@...nel.org>,
	Tejun Heo <tj@...nel.org>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, Akshat Jain <akshatzen@...gle.com>,
	stable@...r.kernel.org
Subject: Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed
 format sense data

On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote:
> On 6/27/24 14:08, Niklas Cassel wrote:
> > Hello Igor, Hannes,
> > 
> > The changes in this patch looks good, however, there is still one thing that
> > bothers me:
> > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
> > 
> > Specifically the code in the else statement below:
> > 
> > 	if (qc->err_mask ||
> > 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > 				   &sense_key, &asc, &ascq);
> > 		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> > 	} else {
> > 		/*
> > 		 * ATA PASS-THROUGH INFORMATION AVAILABLE
> > 		 * Always in descriptor format sense.
> > 		 */
> > 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> > 	}
> > 
> > Looking at sat6r01, I see that this is table:
> > Table 217 — ATA command results
> > 
> > And this text:
> > No error, successful completion or command in progress. The SATL
> > shall terminate the command with CHECK CONDITION status with
> > the sense key set to RECOVERED ERROR with the additional
> > sense code set to ATA PASS-THROUGH INFORMATION
> > AVAILABLE (see SPC-5). Descriptor format sense data shall include
> > the ATA Status Return sense data descriptor (see 12.2.2.7).
> > 
> > However, I don't see anything in this text that says that the
> > sense key should always be in descriptor format sense.
> > 
> > In fact, what will happen if the user has not set the D_SENSE bit
> > (libata will default not set it), is that:
> > 
> > The else statement above will be executed, filling in sense key in
> > descriptor format, after this if/else, we will continue checking
> > if the sense buffer is in descriptor format, or fixed format.
> > 
> > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> > is called with (..., 1, ..., ..., ...) it will always generate
> > the sense data in descriptor format, regardless of
> > dev->flags ATA_DFLAG_D_SENSE being set or not.
> > 
> > Should perhaps the code in the else statement be:
> > 
> > } else {
> > 	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> > }
> > 
> > So that we actually respect the D_SENSE bit?
> > 
> > (We currently respect if when filling the sense data buffer with
> > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
> > respect it for successful ATA PASS-THROUGH commands.)
> > 
> I guess that we've misread the spec.

I think I might have an idea where you got this from:

In sat5r06.pdf
"""
12.2.2.8 Fixed format sense data

Table 212 shows the fields returned in the fixed format sense data (see SPC-5) for ATA PASS-THROUGH
commands. SATLs compliant with ANSI INCITS 431-2007, SCSI/ATA Translation (SAT) return descriptor
format sense data for the ATA PASS-THROUGH commands regardless of the setting of the D_SENSE bit.
"""

In sat6r01.pdf:
"""
12.2.2.8 Fixed format sense data

Table 219 shows the fields returned in the fixed format sense data (see SPC-5)
for ATA PASS-THROUGH
commands.
"""

In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should
ignore D_SENSE bit and unconditionally return sense data in descriptor format.

Anyway, considering that:
1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS
   431-2007.
2) This text has been removed from SAT-6.
3) We currently honour the D_SENSE bit when creating the sense buffer with the
   SK/ASC/ASCQ that we get from the device.

I think that it makes sense to honour the D_SENSE bit also when generating
sense data for successful ATA PASS-THROUGH commands (from ATA registers).


Kind regards,
Niklas