On Mon, Jul 01, 2013 at 07:12:07PM +0300, Felipe Balbi wrote: > On Mon, Jul 01, 2013 at 08:31:38AM -0700, Greg KH wrote: > > On Mon, Jul 01, 2013 at 05:24:18PM +0300, Felipe Balbi wrote: > > > Hi, > > > > > > On Mon, Jul 01, 2013 at 10:19:28AM -0400, Alan Stern wrote: > > > > > On Mon, Jul 01, 2013 at 09:56:37AM -0400, Alan Stern wrote: > > > > > > On Mon, 1 Jul 2013, Felipe Balbi wrote: > > > > > > > > > > > > > bInterval must be within the range 1 - 16, > > > > > > > > > > > > That's true only for high speed and SuperSpeed. For low speed and full > > > > > > speed, bInterval is allowed to range from 1 to 255. See p. 271 in the > > > > > > USB-2 spec. > > > > > > > > > > > > > in order to catch drivers passing a too > > > > > > > large bInterval (thus zeroing urb->interval), > > > > > > > let's clamp() the argument to the allowed > > > > > > > range. > > > > > > > > > > > > > > Signed-off-by: Felipe Balbi <balbi@xxxxxx> > > > > > > > --- > > > > > > > include/linux/usb.h | 4 ++++ > > > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > > > diff --git a/include/linux/usb.h b/include/linux/usb.h > > > > > > > index a232b7e..0883e3a 100644 > > > > > > > --- a/include/linux/usb.h > > > > > > > +++ b/include/linux/usb.h > > > > > > > @@ -1545,6 +1545,10 @@ static inline void usb_fill_int_urb(struct urb *urb, > > > > > > > urb->transfer_buffer_length = buffer_length; > > > > > > > urb->complete = complete_fn; > > > > > > > urb->context = context; > > > > > > > + > > > > > > > + /* make sure interval is within allowed range */ > > > > > > > + interval = clamp(interval, 1, 16); > > > > > > > > > > > > These lines should go within the first branch of the "if" statement > > > > > > below. > > > > > > > > > > could you look at the newer version I sent as a reply to this and tell > > > > > me what you think of it ? > > > > > > > > I shouldn't have replied so quickly to the original post -- it was a > > > > violation of my normal policy to read through all the accumulated inbox > > > > messages before replying to any of them. :-( > > > > > > > > Yeah, the updated version is better. I don't think we need to test the > > > > full/low-speed case, though. There already are checks in > > > > usb_submit_urb() to handle unreasonable values of urb->interval. The > > > > only reason for checking in the high/SuperSpeed case is that the stored > > > > value is computed from the input value, and you don't want the > > > > computation to overflow. > > > > > > right, which is what happens on two drivers already (see patches 1 and > > > 2). > > > > Based on the age of those drivers, I would think that those drivers are > > correct, right? Those were probably only tested on full speed devices > > (like the ATM one.) > > sparse still complains, should we then patch usb_fill_int_urb() for > highspeed and leave those drivers alone ? Nah, the drivers are probably incorrect, it's just that no one has noticed all of these years (I doubt many people use the ATM driver, and the cdc-acm patch was just for broken devices.) So fixing the api up would be good. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
