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]

Message-ID: <0fbf1756-5b97-44fc-9802-d481190d2bd8@suse.de>
Date: Fri, 28 Jun 2024 08:47:03 +0200
From: Hannes Reinecke <hare@...e.de>
To: Niklas Cassel <cassel@...nel.org>, Igor Pylypiv <ipylypiv@...gle.com>
Cc: 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 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.

The sentence:

"Descriptor format sense data shall include the ATA Status Return
  Descriptor"

should be interpreted as:

  _If_ the sense code is formatted in descriptor format _then_ the
  ATA Status Return Descriptor should be included.

IE if the sense code is not in descriptor format then the ATA Status
Return Descriptor shouldn't be included (kinda obvious, really).
But of course the D_SENSE bit should be honoured.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@...e.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich