linux-kernel - Re: [PATCH v2] ata: libata-scsi: Set INFORMATION sense data field consistently

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: <Z_QXAA5Mq1kFP4Ao@google.com>
Date: Mon, 7 Apr 2025 11:18:40 -0700
From: Igor Pylypiv <ipylypiv@...gle.com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Damien Le Moal <dlemoal@...nel.org>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ata: libata-scsi: Set INFORMATION sense data field
 consistently

On Mon, Apr 07, 2025 at 10:52:19AM +0200, Niklas Cassel wrote:
> On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote:
> > 
> > Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands
> > already contains the ATA LBA in bytes [6..11] so it seems redundant to
> > also include the same in the Information sense data descriptor.   
> 
> Damien and I talked about this.
> 
> Since this patch only affects non-PT commands, what it this patch actually
> solving?

For ATA PASS-THROUGH + fixed format sense data + NCQ autosense, this patch
eliminates reduntant writes to set the INFORMATION field and the VALID bit.

First write: scsi_set_sense_information() sets the INFORMATION field
to ATA LBA (which is incorrect for ATA PASS-THROUGH).

Second write: ata_scsi_set_passthru_sense_fields() sets the INFORMATION
field to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.

User space should not see an issue because second write overwrites
the incorrect data from the first write. I think we should still fix
this in case someone updates the code to remove the second write in
the future.

How I found this issue?

In commit 38dab832c3f4 (ata: libata-scsi: Fix offsets for the fixed format
sense data) the offsets of fixed format sense data were updated to match 
the SAT spec. To simplify the migration from old incorrect offsets to
the new correct offsets I was considering using the VALID bit to determine
what offsets to use in user space during the migration. The problem is that
for ATA PASS-THROUGH + fixed format sense data + NCQ autosense the VALID bit
is also being set by the scsi_set_sense_information() so we cannot use
the VALID bit to reliably determine if kernel uses the correct fixed sense
data offsets or not.

> 
> Since for non-PT commands, sense data is not returned to the user.
> 
> So unless SCSI core does some specific handling based on the INFORMATION
> field (and AFAICT, SCSI core only looks at SK/ASC/ASCQ), I can't see how
> this patch can actually solve anything.

+1 the patch does not seem to solve any issues for non-PT commands besides
a spec compliance which is not visible to a user.

Deleting ata_scsi_set_sense_information() should work as well. If SCSI core
does not use the INFORMATION field perhaps there is no need to set it at all?
This will eliminate the reduntant writes for ATA PASS-THROUGH as well.

Thanks,
Igor

> 
> 
> Kind regards,
> Niklas