| Message ID | 20211027080819.6675-4-johan@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 89f8765a11d8df49296d92c404067f9b5c58ee26 |
| Headers | show |
| Series | wireless: fix division by zero in USB drivers | expand |
On Wed, Oct 27, 2021 at 1:12 AM Johan Hovold <johan@kernel.org> wrote: > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > @@ -505,6 +505,22 @@ static int mwifiex_usb_probe(struct usb_interface *intf, > } > } > > + switch (card->usb_boot_state) { > + case USB8XXX_FW_DNLD: > + /* Reject broken descriptors. */ > + if (!card->rx_cmd_ep || !card->tx_cmd_ep) > + return -ENODEV; ^^ These two conditions are applicable to USB8XXX_FW_READY too, right? > + if (card->bulk_out_maxpktsize == 0) > + return -ENODEV; > + break; > + case USB8XXX_FW_READY: > + /* Assume the driver can handle missing endpoints for now. */ > + break; > + default: > + WARN_ON(1); > + return -ENODEV; > + } > + Anyway, looks pretty good, thanks: Reviewed-by: Brian Norris <briannorris@chromium.org>
On Wed, Oct 27, 2021 at 11:22:39AM -0700, Brian Norris wrote: > On Wed, Oct 27, 2021 at 1:12 AM Johan Hovold <johan@kernel.org> wrote: > > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > > @@ -505,6 +505,22 @@ static int mwifiex_usb_probe(struct usb_interface *intf, > > } > > } > > > > + switch (card->usb_boot_state) { > > + case USB8XXX_FW_DNLD: > > + /* Reject broken descriptors. */ > > + if (!card->rx_cmd_ep || !card->tx_cmd_ep) > > + return -ENODEV; > > ^^ These two conditions are applicable to USB8XXX_FW_READY too, right? Right, but I didn't want to add an incomplete set of constraints. I couldn't find any documentation (e.g. lsusb -v) for what the descriptors are supposed to look like, but judging from the code, something like if (!card->rx_cmd_ep || !card->tx_cmd_ep) return -ENODEV; if (!card->rx_data_ep || !card->port[0].tx_data_ep) return -ENODEV; should do. But I'm not sure about the second tx endpoint, card->port[1].tx_data_ep, for which support was added later and which the driver appears to be able to manage without. Either way it has nothing to do with the division-by-zero and should be added separately. > > + if (card->bulk_out_maxpktsize == 0) > > + return -ENODEV; > > + break; > > + case USB8XXX_FW_READY: > > + /* Assume the driver can handle missing endpoints for now. */ > > + break; > > + default: > > + WARN_ON(1); > > + return -ENODEV; > > + } Johan
Johan Hovold <johan@kernel.org> wrote: > Add the missing endpoint sanity checks to probe() to avoid division by > zero in mwifiex_write_data_sync() in case a malicious device has broken > descriptors (or when doing descriptor fuzz testing). > > Only add checks for the firmware-download boot stage, which require both > command endpoints, for now. The driver looks like it will handle a > missing endpoint during normal operation without oopsing, albeit not > very gracefully as it will try to submit URBs to the default pipe and > fail. > > Note that USB core will reject URBs submitted for endpoints with zero > wMaxPacketSize but that drivers doing packet-size calculations still > need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip > endpoint descriptors with maxpacket=0")). > > Fixes: 4daffe354366 ("mwifiex: add support for Marvell USB8797 chipset") > Cc: stable@vger.kernel.org # 3.5 > Cc: Amitkumar Karwar <akarwar@marvell.com> > Signed-off-by: Johan Hovold <johan@kernel.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> Patch applied to wireless-drivers-next.git, thanks. 89f8765a11d8 mwifiex: fix division by zero in fw download path
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c index 426e39d4ccf0..9736aa0ab7fd 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.c +++ b/drivers/net/wireless/marvell/mwifiex/usb.c @@ -505,6 +505,22 @@ static int mwifiex_usb_probe(struct usb_interface *intf, } } + switch (card->usb_boot_state) { + case USB8XXX_FW_DNLD: + /* Reject broken descriptors. */ + if (!card->rx_cmd_ep || !card->tx_cmd_ep) + return -ENODEV; + if (card->bulk_out_maxpktsize == 0) + return -ENODEV; + break; + case USB8XXX_FW_READY: + /* Assume the driver can handle missing endpoints for now. */ + break; + default: + WARN_ON(1); + return -ENODEV; + } + usb_set_intfdata(intf, card); ret = mwifiex_add_card(card, &card->fw_done, &usb_ops,
Add the missing endpoint sanity checks to probe() to avoid division by zero in mwifiex_write_data_sync() in case a malicious device has broken descriptors (or when doing descriptor fuzz testing). Only add checks for the firmware-download boot stage, which require both command endpoints, for now. The driver looks like it will handle a missing endpoint during normal operation without oopsing, albeit not very gracefully as it will try to submit URBs to the default pipe and fail. Note that USB core will reject URBs submitted for endpoints with zero wMaxPacketSize but that drivers doing packet-size calculations still need to handle this (cf. commit 2548288b4fb0 ("USB: Fix: Don't skip endpoint descriptors with maxpacket=0")). Fixes: 4daffe354366 ("mwifiex: add support for Marvell USB8797 chipset") Cc: stable@vger.kernel.org # 3.5 Cc: Amitkumar Karwar <akarwar@marvell.com> Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/net/wireless/marvell/mwifiex/usb.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)