linux-kernel - Re: [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field twice for ATA PT

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] [day] [month] [year] [list]

Message-ID: <aAogiHxc-mdORpe8@ryzen>
Date: Thu, 24 Apr 2025 13:29:12 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>
Cc: Damien Le Moal <dlemoal@...nel.org>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ata: libata-scsi: Do not set the INFORMATION field
 twice for ATA PT

On Tue, Apr 22, 2025 at 10:21:23AM -0700, Igor Pylypiv wrote:
> For ATA PASS-THROUGH + fixed format sense data + NCQ autosense
> the INFORMATION sense data field is being written twice:
> 
> - 1st write: (redundant)
> scsi_set_sense_information() sets the INFORMATION field to ATA LBA.
> This is incorrect for ATA PASS-THROUGH.
> 
> - 2nd write: (correct)
> ata_scsi_set_passthru_sense_fields() sets the INFORMATION field
> to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec.
> 
> There is no user-visible issue because second write overwrites
> the incorrect data from the first write.
> 
> This patch eliminates the reduntant write by moving the INFORMATION
> sense data field population logic to ata_scsi_qc_complete().
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@...gle.com>
> ---
> 
> Changes in v3:
> - Rewrote the commit message to clearly point that this patch removes
>   a redundant write to the INFORMATION sense data field.
> 
> Changes in v2:
> - Rephrased commit message to make it clearer.
> - Dropped kernel-doc comment for the ata_scsi_set_sense_information().
> 
> 
>  drivers/ata/libata-sata.c |  2 --
>  drivers/ata/libata-scsi.c | 31 ++++++++++++++-----------------
>  drivers/ata/libata.h      |  3 ---
>  3 files changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 2e4463d3a356..74a0af2888fa 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1659,8 +1659,6 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
>  		if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) {
>  			ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc,
>  					   ascq);
> -			ata_scsi_set_sense_information(dev, qc->scsicmd,
> -						       &qc->result_tf);
>  			qc->flags |= ATA_QCFLAG_SENSE_VALID;
>  		}
>  	}
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 2796c0da8257..ef117a0bc248 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -216,17 +216,21 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>  }
>  
> -void ata_scsi_set_sense_information(struct ata_device *dev,
> -				    struct scsi_cmnd *cmd,
> -				    const struct ata_taskfile *tf)
> +static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)

Nice refactoring of ata_scsi_set_sense_information()!

However, since it is a separate logical change, I would have prefered for it
to be in a separate patch, but I don't think you need to respin just for this.


Reviewed-by: Niklas Cassel <cassel@...nel.org>