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