On Mon, Feb 04 2008 at 22:05 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Sun, 3 Feb 2008, Matthew Dharm wrote: > >> But, the modifications to usb_stor_access_xfer_buf() look good -- no >> request from a sub-driver should be allowed to scribble into memory. The >> current code does make the implicit assumption that there is enough >> storage, and will walk right off the end of the sg list if there isn't. >> >> I'm not sure I like the mods to usb_stor_set_xfer_buf(). Any place we set >> a status that we know is going to be thrown away is an invitation for a >> problem later if someone changes the code to preserve that status. It's a >> jack-in-the-box, waiting to spring open in our face later. The limit check >> (which mirrors the usb_stor_access_xfer_buf modification) and WARN_ON() are >> probably good. >> >> In a strictly technical sense, the change to protocol.c are sufficient. >> That is, they will prevent a serious error. There is a justification tho >> to fix all of the users of usb_stor_access_buf() to not attempt to use more >> SCSI buffer than exists. >> >> My opinion is this: Let's make the protocol.c mods (modulo my comments >> about setting useless status bits) now. Then, let's decide if we're going >> to patch all the other users of the usb_stor_*_xfer_buf() functions as a >> separate discussion. > > I think the correct approach is to modify those routines so that they > will never overrun the s-g buffer (like Boaz has done), and _document_ > this behavior. Then the callers can feel free to try and transfer as > much as they want, knowing that an overrun can't occur. There won't > be any need for a WARN_ON or anything else. > > However the interface to usb_stor_access_xfer_buf() will have to change > slightly. Right now if it sees that *sgptr is NULL, it assumes this > means it should start at the beginning of the s-g buffer. But with > Boaz's change, *sgptr == NULL means the transfer has reached the end of > the buffer. So I'll have to go through and audit all the callers. > > Alan Stern > > - No it does not, this as not changed. Please look again. Note that this patch was tested and working. It is a bug in v2.2.24 and it should be accepted already. One way or the other. Callers of usb_stor_access_xfer_buf() need not change. Matthew Dharm should decide if he wants the WARN_ON in usb_stor_set_xfer_buf() or not and be done with it. I have found and fixed the bug, but it is not a SCSI related bug, and it is not do to any scsi changes. It is a bug from the SG changes of early 2.6.24. Please take it through the USB tree. Feel free to change it the way you like it, and submit it. 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
