Elias Oltmanns wrote: > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: >> This patch (as1108) fixes a problem that can occur with certain USB >> mass-storage devices: They return invalid data together with a residue >> indicating that the data should be ignored. Rather than leave the >> invalid data in a transfer buffer, where it can get misinterpreted, >> the patch clears the invalid portion of the buffer. > > I've only just stumbled upon this patch and I don't quite understand how > it is supposed to work. > >> This solves a problem (wrong write-protect setting detected) reported >> by Maciej Rutecki and Peter Teoh. >> >> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> >> Tested-by: Peter Teoh <htmldeveloper@xxxxxxxxx> >> >> --- >> >> Index: usb-2.6/drivers/scsi/scsi_lib.c >> =================================================================== >> --- usb-2.6.orig/drivers/scsi/scsi_lib.c >> +++ usb-2.6/drivers/scsi/scsi_lib.c >> @@ -207,6 +207,15 @@ int scsi_execute(struct scsi_device *sde >> */ >> blk_execute_rq(req->q, NULL, req, 1); >> >> + /* >> + * Some devices (USB mass-storage in particular) may transfer >> + * garbage data together with a residue indicating that the data >> + * is invalid. Prevent the garbage from being misinterpreted >> + * and prevent security leaks by zeroing out the excess data. >> + */ >> + if (unlikely(req->data_len > 0 && req->data_len <= bufflen)) >> + memset(buffer + (bufflen - req->data_len), 0, req->data_len); > > Sorry, I don't understand that line at all. Surely, we want to zero out > either the excess data, i.e. buffer -> buffer + req->data_len, or the > residue, i.e. buffer + req->data_len -> buffer + bufflen. Your patch > implies that there are bufflen - req->data_len bytes of valid data at > the beginning of buffer. If this is intentional, please bear with me and > explain. Otherwise, what about the following patch to 2.6.26? On the > other hand, the same could probably be achieved by setting req->data_len > to 0. Oh dear, it would appear that I'm completely lost here. > > Elias > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index cbf55d5..977f22b 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -214,7 +214,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > * and prevent security leaks by zeroing out the excess data. > */ > if (unlikely(req->data_len > 0 && req->data_len <= bufflen)) > - memset(buffer + (bufflen - req->data_len), 0, req->data_len); > + memset(buffer + req->data_len, 0, bufflen - req->data_len); > > ret = req->errors; > out: This is not your fault it is built confusing. The req->data_len is used in to ways. At first it is used as bufflen, as input to blk_execute but at the very last stage of execution it is set to be the residual of the transfer, from the scsi_cmnd->resid member. So at this stage you see above, req->data_len is whats left of @bufflen that was not written/read by blk_execute. Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
