LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2 0/3] wireless: fix division by zero in USB drivers
@ 2021-10-27  8:08 Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 1/3] ath10k: fix division by zero in send path Johan Hovold
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Johan Hovold @ 2021-10-27  8:08 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold

This series adds the missing endpoint sanity checks to the wireless USB
drivers that were doing packet-size calculations without first making
sure that the USB descriptors were sane.

Johan

Changes in v2:
 - tighten the sanity checks for the mwifiex firmware-download mode to
   handle also a missing bulk-out endpoint (noticed by Brian Norris)


Johan Hovold (3):
  ath10k: fix division by zero in send path
  ath6kl: fix division by zero in send path
  mwifiex: fix division by zero in fw download path

 drivers/net/wireless/ath/ath10k/usb.c      |  5 +++++
 drivers/net/wireless/ath/ath6kl/usb.c      |  5 +++++
 drivers/net/wireless/marvell/mwifiex/usb.c | 16 ++++++++++++++++
 3 files changed, 26 insertions(+)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] ath10k: fix division by zero in send path
  2021-10-27  8:08 [PATCH v2 0/3] wireless: fix division by zero in USB drivers Johan Hovold
@ 2021-10-27  8:08 ` Johan Hovold
  2021-10-28  7:34   ` Kalle Valo
  2021-10-27  8:08 ` [PATCH v2 2/3] ath6kl: " Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 3/3] mwifiex: fix division by zero in fw download path Johan Hovold
  2 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2021-10-27  8:08 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold, stable, Erik Stromdahl

Add the missing endpoint max-packet sanity check to probe() to avoid
division by zero in ath10k_usb_hif_tx_sg() in case a malicious device
has broken descriptors (or when doing descriptor fuzz testing).

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: 4db66499df91 ("ath10k: add initial USB support")
Cc: stable@vger.kernel.org      # 4.14
Cc: Erik Stromdahl <erik.stromdahl@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/ath/ath10k/usb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index 6d831b098cbb..3d98f19c6ec8 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -853,6 +853,11 @@ static int ath10k_usb_setup_pipe_resources(struct ath10k *ar,
 				   le16_to_cpu(endpoint->wMaxPacketSize),
 				   endpoint->bInterval);
 		}
+
+		/* Ignore broken descriptors. */
+		if (usb_endpoint_maxp(endpoint) == 0)
+			continue;
+
 		urbcount = 0;
 
 		pipe_num =
-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] ath6kl: fix division by zero in send path
  2021-10-27  8:08 [PATCH v2 0/3] wireless: fix division by zero in USB drivers Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 1/3] ath10k: fix division by zero in send path Johan Hovold
@ 2021-10-27  8:08 ` Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 3/3] mwifiex: fix division by zero in fw download path Johan Hovold
  2 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2021-10-27  8:08 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold, stable

Add the missing endpoint max-packet sanity check to probe() to avoid
division by zero in ath10k_usb_hif_tx_sg() in case a malicious device
has broken descriptors (or when doing descriptor fuzz testing).

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: 9cbee358687e ("ath6kl: add full USB support")
Cc: stable@vger.kernel.org      # 3.5
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/ath/ath6kl/usb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
index bd367b79a4d3..aba70f35e574 100644
--- a/drivers/net/wireless/ath/ath6kl/usb.c
+++ b/drivers/net/wireless/ath/ath6kl/usb.c
@@ -340,6 +340,11 @@ static int ath6kl_usb_setup_pipe_resources(struct ath6kl_usb *ar_usb)
 				   le16_to_cpu(endpoint->wMaxPacketSize),
 				   endpoint->bInterval);
 		}
+
+		/* Ignore broken descriptors. */
+		if (usb_endpoint_maxp(endpoint) == 0)
+			continue;
+
 		urbcount = 0;
 
 		pipe_num =
-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] mwifiex: fix division by zero in fw download path
  2021-10-27  8:08 [PATCH v2 0/3] wireless: fix division by zero in USB drivers Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 1/3] ath10k: fix division by zero in send path Johan Hovold
  2021-10-27  8:08 ` [PATCH v2 2/3] ath6kl: " Johan Hovold
@ 2021-10-27  8:08 ` Johan Hovold
  2021-10-27 18:22   ` Brian Norris
  2021-10-28 13:28   ` Kalle Valo
  2 siblings, 2 replies; 8+ messages in thread
From: Johan Hovold @ 2021-10-27  8:08 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold, stable, Amitkumar Karwar

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(+)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] mwifiex: fix division by zero in fw download path
  2021-10-27  8:08 ` [PATCH v2 3/3] mwifiex: fix division by zero in fw download path Johan Hovold
@ 2021-10-27 18:22   ` Brian Norris
  2021-10-28  7:20     ` Johan Hovold
  2021-10-28 13:28   ` Kalle Valo
  1 sibling, 1 reply; 8+ messages in thread
From: Brian Norris @ 2021-10-27 18:22 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Kalle Valo, Amitkumar Karwar, Ganapathi Bhat,
	Sharvari Harisangam, Xinming Hu, linux-wireless, netdev,
	linux-usb, linux-kernel, stable, Amitkumar Karwar

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] mwifiex: fix division by zero in fw download path
  2021-10-27 18:22   ` Brian Norris
@ 2021-10-28  7:20     ` Johan Hovold
  0 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2021-10-28  7:20 UTC (permalink / raw)
  To: Brian Norris
  Cc: Kalle Valo, Amitkumar Karwar, Ganapathi Bhat,
	Sharvari Harisangam, Xinming Hu, linux-wireless, netdev,
	linux-usb, linux-kernel, stable, Amitkumar Karwar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] ath10k: fix division by zero in send path
  2021-10-27  8:08 ` [PATCH v2 1/3] ath10k: fix division by zero in send path Johan Hovold
@ 2021-10-28  7:34   ` Kalle Valo
  0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2021-10-28  7:34 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold, stable, Erik Stromdahl

Johan Hovold <johan@kernel.org> wrote:

> Add the missing endpoint max-packet sanity check to probe() to avoid
> division by zero in ath10k_usb_hif_tx_sg() in case a malicious device
> has broken descriptors (or when doing descriptor fuzz testing).
> 
> 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: 4db66499df91 ("ath10k: add initial USB support")
> Cc: stable@vger.kernel.org      # 4.14
> Cc: Erik Stromdahl <erik.stromdahl@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

2 patches applied to ath-next branch of ath.git, thanks.

a006acb93131 ath10k: fix division by zero in send path
c1b9ca365dea ath6kl: fix division by zero in send path

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20211027080819.6675-2-johan@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] mwifiex: fix division by zero in fw download path
  2021-10-27  8:08 ` [PATCH v2 3/3] mwifiex: fix division by zero in fw download path Johan Hovold
  2021-10-27 18:22   ` Brian Norris
@ 2021-10-28 13:28   ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2021-10-28 13:28 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Brian Norris, linux-wireless, netdev, linux-usb,
	linux-kernel, Johan Hovold, stable, Amitkumar Karwar

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20211027080819.6675-4-johan@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-28 13:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-27  8:08 [PATCH v2 0/3] wireless: fix division by zero in USB drivers Johan Hovold
2021-10-27  8:08 ` [PATCH v2 1/3] ath10k: fix division by zero in send path Johan Hovold
2021-10-28  7:34   ` Kalle Valo
2021-10-27  8:08 ` [PATCH v2 2/3] ath6kl: " Johan Hovold
2021-10-27  8:08 ` [PATCH v2 3/3] mwifiex: fix division by zero in fw download path Johan Hovold
2021-10-27 18:22   ` Brian Norris
2021-10-28  7:20     ` Johan Hovold
2021-10-28 13:28   ` Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).