linux-kernel - Re: [BUG] 2.6.24-git usb reset problems

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Jan 2008 11:34:15 -0500
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Boaz Harrosh <bharrosh@...asas.com>, Greg KH <greg@...ah.com>,
	Jens Axboe <jens.axboe@...cle.com>,
	Matthew Dharm <mdharm-usb@...-eyed-alien.net>,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-scsi@...r.kernel.org
Subject: Re: [BUG] 2.6.24-git usb reset problems

On Tue, 2008-01-29 at 10:36 -0500, Alan Stern wrote:
> On Tue, 29 Jan 2008, Boaz Harrosh wrote:
> 
> > --- a/drivers/usb/storage/transport.c
> > +++ b/drivers/usb/storage/transport.c
> > @@ -462,18 +462,24 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
> >   * Common used function. Transfer a complete command
> >   * via usb_stor_bulk_transfer_sglist() above. Set cmnd resid
> >   */
> > -int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
> > -		      struct scsi_cmnd* srb)
> > +int usb_stor_bulk_srb_length(struct us_data* us, unsigned int pipe,
> > +		      struct scsi_cmnd* srb, unsigned length)
> >  {
> >  	unsigned int partial;
> >  	int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
> > -				      scsi_sg_count(srb), scsi_bufflen(srb),
> > +				      scsi_sg_count(srb), length,
> >  				      &partial);
> >  
> >  	scsi_set_resid(srb, scsi_bufflen(srb) - partial);
> >  	return result;
> >  }
> >  
> > +int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
> > +		struct scsi_cmnd* srb)
> > +{
> > +	return usb_stor_bulk_srb_length(us, pipe, srb, scsi_bufflen(srb));
> > +}
> > +
> 
> I don't like this patch very much.  Why add another layer of 
> indirection when the two subroutines do hardly any work?  Leave 
> usb_stor_bulk_srb() the way it was, and add usb_stor_bulk_srb_length() 
> as a separate routine that simply calls usb_stor_bulk_transfer_sglist() 
> and scsi_set_resid().
> 
> BTW, the standard coding style calls for a blank line after the list of 
> local variables at the start of a function or block.

There's another bug in the transport.c conversion in that the residuals
are updated with bogus data in several error cases, since
usb_stor_bulk_transfer_sglist() only sets the actual length if the urb
is actually sent.

I'm not sure if this is is the solution to the problem at hand, but it
definitely fixes another bug in the code.

James

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index d9f4912..bab0858 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -465,7 +465,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
 int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
 		      struct scsi_cmnd* srb)
 {
-	unsigned int partial;
+	unsigned int partial = scsi_get_resid(srb);
 	int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
 				      scsi_sg_count(srb), scsi_bufflen(srb),
 				      &partial);


--
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/