linux-kernel - Re: [PATCH] ata: ata_id_is

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: <20081019180545.GT19428@kernel.dk>
Date:	Sun, 19 Oct 2008 20:05:46 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org,
	Jeff Garzik <jgarzik@...ox.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCH] ata: ata_id_is_ssd() bugfix

On Sat, Oct 18 2008, Bartlomiej Zolnierkiewicz wrote:
> We need to explicitly check for major and minor version
> of supported ATA spec as earlier revisions used word 217
> for different purposes.

What did they use to put in that word? Just curious if it does any harm,
because as it stands, this patch will prevent ANY ssd from being
correctly flagged as such. So I'm inclined to file this as too much spec
fiddling, it'll do more harm than good.

> 
> [ The issue was originally spotted by Alan Cox. ]
> 
> This patch fixes regression introduced by:
> commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
> ("libata: set queue SSD flag for SSD devices").
> 
> Cc: Jeff Garzik <jgarzik@...ox.com>
> Cc: Alan Cox <alan@...rguk.ukuu.org.uk>
> Cc: Jens Axboe <jens.axboe@...cle.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
> ---
> somebody owe me one for going through all these spec drafts... ;)
> 
>  include/linux/ata.h |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Index: b/include/linux/ata.h
> ===================================================================
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -75,6 +75,7 @@ enum {
>  	ATA_ID_EIDE_PIO_IORDY	= 68,
>  	ATA_ID_QUEUE_DEPTH	= 75,
>  	ATA_ID_MAJOR_VER	= 80,
> +	ATA_ID_MINOR_VER	= 81,
>  	ATA_ID_COMMAND_SET_1	= 82,
>  	ATA_ID_COMMAND_SET_2	= 83,
>  	ATA_ID_CFSSE		= 84,
> @@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
>  
>  static inline int ata_id_is_ssd(const u16 *id)
>  {
> -	return id[ATA_ID_ROT_SPEED] == 0x01;
> +	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
> +	if (ata_id_major_version(id) >= 8 &&
> +	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
> +	    id[ATA_ID_ROT_SPEED] == 0x01)
> +		return 1;
> +	return 0;

Is the check even correct? It'll match version 8 AND the currently
listed minor version, not newer.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/