| Message ID | 20200907145108.3766613-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: new USB control message helper functions | expand |
On Mon, Sep 07, 2020 at 04:51:00PM +0200, Greg Kroah-Hartman wrote: > There are a few calls to usb_control_msg() that can be converted to use > usb_control_msg_send() instead, so do that in order to make the error > checking a bit simpler. > > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-usb@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Oops, Andy, sorry, you gave me a Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> on the previous version of this, I'll add it next round, or when it's queued up. thanks, greg k-h
On Mon, Sep 07, 2020 at 04:51:01PM +0200, Greg Kroah-Hartman wrote: > There are a few calls to usb_control_msg() that can be converted to use > usb_control_msg_send() instead, so do that in order to make the error > checking a bit simpler and the code smaller. > > Cc: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > v2: > - dropped changes to usb_req_set_sel() thanks to review from Alan > @@ -4056,7 +4035,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, > * associated with the link state we're about to enable. > */ > ret = usb_req_set_sel(udev, state); > - if (ret < 0) { > + if (ret) { > dev_warn(&udev->dev, "Set SEL for device-initiated %s failed.\n", > usb3_lpm_names[state]); > return; Did this change survive by mistake? Actually, it looks like usb_req_set_sel needs to check the value returned by usb_control_msg -- a perfect example of the sort of thing you were trying to fix in the first place! Alan Stern
On Mon, Sep 07, 2020 at 04:56:44PM +0200, Greg Kroah-Hartman wrote: > On Mon, Sep 07, 2020 at 04:51:00PM +0200, Greg Kroah-Hartman wrote: > > There are a few calls to usb_control_msg() that can be converted to use > > usb_control_msg_send() instead, so do that in order to make the error > > checking a bit simpler. > > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: linux-usb@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Oops, Andy, sorry, you gave me a Reviewed-by: Andy Shevchenko > <andriy.shevchenko@linux.intel.com> on the previous version of this, > I'll add it next round, or when it's queued up. NP! Whatever works better for you.
On Mon, Sep 07, 2020 at 11:08:58AM -0400, Alan Stern wrote: > On Mon, Sep 07, 2020 at 04:51:01PM +0200, Greg Kroah-Hartman wrote: > > There are a few calls to usb_control_msg() that can be converted to use > > usb_control_msg_send() instead, so do that in order to make the error > > checking a bit simpler and the code smaller. > > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > v2: > > - dropped changes to usb_req_set_sel() thanks to review from Alan > > > @@ -4056,7 +4035,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, > > * associated with the link state we're about to enable. > > */ > > ret = usb_req_set_sel(udev, state); > > - if (ret < 0) { > > + if (ret) { > > dev_warn(&udev->dev, "Set SEL for device-initiated %s failed.\n", > > usb3_lpm_names[state]); > > return; > > Did this change survive by mistake? > > Actually, it looks like usb_req_set_sel needs to check the value > returned by usb_control_msg -- a perfect example of the sort of thing > you were trying to fix in the first place! Ugh, good catch, and yes, the original code is buggy :) thanks, greg k-h
In a recent discussion about a USB networking bug found by syzbot, and fixed by Himadri Pandya, the design of the existing usb_control_msg() call was brought up as not being the "nicest" thing to use by Dmitry Vyukov: https://lore.kernel.org/r/CACT4Y+YbDODLRFn8M5QcY4CazhpeCaunJnP_udXtAs0rYoASSg@mail.gmail.com The function makes it hard to get right, in that it will return the number of bytes sent/received, but almost no one checks to see if a short read/write happens. With a malicious, or broken, USB device, this can cause drivers to act on data that they did not anticipate, and sometimes copy internal kernel data out to userspace. So let's fix this up by creating two new functions, usb_control_msg_send() and usb_control_msg_recv(). These functions either complete the full transation, or they return an error, a short send/recv is now an error. They also accept data off of the stack, saving individual drivers the pain of having to constantly allocate memory on their own for tiny messages, thereby saving overall kernel code space. The api also does not require a raw USB "pipe" to be sent to the function, as we know the direction, so just pass in the endpoint number instead, again making it easier on the USB driver author to use. This series first takes a helper function out of the sound core for verifying USB endpoints to be able to use internally, and then adds the new functions, converts over some internal USB code to use them, and then starts to clean up some drivers using these new functions, as an example of the savings that can happen by using these functions. Thanks to Dmitry and Himadri for the idea on how to do all of this. greg k-h Changes from v1: - added acks from Takashi Iwai - dropped changes to one function in patch 04 thanks to review from Alan Stern - typo fix in comment in patch 01 - added new patch 11 to remove some unneeded checks in the sound drivers for endpoint statuses that would always be true. *** BLURB HERE *** Greg Kroah-Hartman (11): USB: move snd_usb_pipe_sanity_check into the USB core USB: add usb_control_msg_send() and usb_control_msg_recv() USB: core: message.c: use usb_control_msg_send() in a few places USB: core: hub.c: use usb_control_msg_send() in a few places USB: legousbtower: use usb_control_msg_recv() sound: usx2y: move to use usb_control_msg_send() sound: 6fire: move to use usb_control_msg_send() and usb_control_msg_recv() sound: line6: move to use usb_control_msg_send() and usb_control_msg_recv() sound: hiface: move to use usb_control_msg_send() Bluetooth: ath3k: use usb_control_msg_send() and usb_control_msg_recv() ALSA: remove calls to usb_pipe_type_check for control endpoints drivers/bluetooth/ath3k.c | 90 +++++------------ drivers/usb/core/hub.c | 99 ++++++++---------- drivers/usb/core/message.c | 171 ++++++++++++++++++++++++++++---- drivers/usb/core/urb.c | 31 ++++-- drivers/usb/misc/legousbtower.c | 60 ++++------- include/linux/usb.h | 7 ++ sound/usb/6fire/firmware.c | 38 +++---- sound/usb/helper.c | 16 +-- sound/usb/helper.h | 1 - sound/usb/hiface/pcm.c | 14 ++- sound/usb/line6/driver.c | 69 +++++-------- sound/usb/line6/podhd.c | 17 ++-- sound/usb/line6/toneport.c | 8 +- sound/usb/mixer_scarlett_gen2.c | 2 +- sound/usb/quirks.c | 12 +-- sound/usb/usx2y/us122l.c | 42 ++------ 16 files changed, 335 insertions(+), 342 deletions(-)