linux-kernel - Re: [PATCH] mass storage : emulation of sat scsi_pass

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: <20080310012014.GG2820@one-eyed-alien.net>
Date:	Sun, 9 Mar 2008 18:20:14 -0700
From:	Matthew Dharm <mdharm-kernel@...-eyed-alien.net>
To:	matthieu castet <castet.matthieu@...e.fr>
Cc:	linux-usb@...r.kernel.org,
	Linux Kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH]  mass storage : emulation of sat scsi_pass_thru with ATACB

On Sun, Mar 09, 2008 at 10:42:50PM +0100, matthieu castet wrote:
> Hi
> 
> here an update version of the patch

We're getting very close here.

> + 	/* this value can change, but most(all ?) manufacturers keep the 
> cypress
> +	 * default : 0x24
> +	 */

Line wrap problem.  This happens a few times in your patch, but this was
the first one I noticed.

> +	srb->cmnd[0] = 0x24;
> +	srb->cmnd[1] = 0x24;
> +	srb->cmnd[2] = 0;
> +
> +	srb->cmnd[3] = 0xff - 1;
> +	srb->cmnd[4] = 1;

What are these magic constants?  Symbolic names or comments would be
apropriate here.

> +	if (srb->cmnd[12] == ATA_CMD_ID_ATA || srb->cmnd[12] == 
> ATA_CMD_ID_ATAPI)
> +		srb->cmnd[2] |= (1<<7);

What does this block do?  In general, you could use some more comments
about the code flow...

> +	/* if the device doesn't support ATACB
> +	 * abort and register usb_stor_transparent_scsi_command
> +	 * callback
> +	 */
> +	if (srb->result == SAM_STAT_CHECK_CONDITION &&
> +			memcmp(srb->sense_buffer, usb_stor_sense_invalidCDB,
> +				sizeof(usb_stor_sense_invalidCDB)) == 0) {
> +		us->subclass = US_SC_SCSI;
> +		us->proto_handler = usb_stor_transparent_scsi_command;
> +		goto end;
> +	}

This is an interesting section of code.  Do you have reason to believe that
some of the units you've claimed via unusual_devs.h do not support this
protocol, or are you just being forward-looking?

Either way, some debug printout would be good here...

> --- linux-2.6.24.2.orig/include/linux/usb_usual.h	2008-03-09 
> 21:15:01.000000000 +0100
> +++ linux-2.6.24.2/include/linux/usb_usual.h	2008-03-09 
> 21:24:12.000000000 +0100
> @@ -81,8 +81,9 @@
>  #define US_SC_8070	0x05		/* Removable media */
>  #define US_SC_SCSI	0x06		/* Transparent */
>  #define US_SC_ISD200    0x07		/* ISD200 ATA */
> +#define US_SC_CYP_ATACB 0x08		/* Cypress ATACB */
>  #define US_SC_MIN	US_SC_RBC
> -#define US_SC_MAX	US_SC_ISD200
> +#define US_SC_MAX	US_SC_CYP_ATACB
> 
>  #define US_SC_DEVICE	0xff		/* Use device's value */

I thought we changed this so that the non-spec protocols started from 0xfe
and worked their way down?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@...-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997

Content of type "application/pgp-signature" skipped