| Message ID | 20211026095214.26375-3-johan@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/3] ath10k: fix division by zero in send path | expand |
On Tue, Oct 26, 2021 at 2:53 AM Johan Hovold <johan@kernel.org> wrote: > > Add the missing endpoint max-packet sanity check 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). > > 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> > --- Seems like you're missing a changelog and a version number, since you've already sent previous versions of this patch. > drivers/net/wireless/marvell/mwifiex/usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c > index 426e39d4ccf0..2826654907d9 100644 > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > @@ -502,6 +502,9 @@ static int mwifiex_usb_probe(struct usb_interface *intf, > atomic_set(&card->tx_cmd_urb_pending, 0); > card->bulk_out_maxpktsize = > le16_to_cpu(epd->wMaxPacketSize); > + /* Reject broken descriptors. */ > + if (card->bulk_out_maxpktsize == 0) > + return -ENODEV; If we're really talking about malicious devices, I'm still not 100% sure this is sufficient -- what if the device doesn't advertise the right endpoints? Might we get through the surrounding loop without ever even reaching this code? Seems like the right thing to do would be to pull the validation outside the loop. Brian
On Tue, Oct 26, 2021 at 10:35:37AM -0700, Brian Norris wrote: > On Tue, Oct 26, 2021 at 2:53 AM Johan Hovold <johan@kernel.org> wrote: > > > > Add the missing endpoint max-packet sanity check 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). > > > > 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> > > --- > > Seems like you're missing a changelog and a version number, since > you've already sent previous versions of this patch. Seems like you're confusing me with someone else. > > drivers/net/wireless/marvell/mwifiex/usb.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c > > index 426e39d4ccf0..2826654907d9 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > > @@ -502,6 +502,9 @@ static int mwifiex_usb_probe(struct usb_interface *intf, > > atomic_set(&card->tx_cmd_urb_pending, 0); > > card->bulk_out_maxpktsize = > > le16_to_cpu(epd->wMaxPacketSize); > > + /* Reject broken descriptors. */ > > + if (card->bulk_out_maxpktsize == 0) > > + return -ENODEV; > > If we're really talking about malicious devices, I'm still not 100% > sure this is sufficient -- what if the device doesn't advertise the > right endpoints? Might we get through the surrounding loop without > ever even reaching this code? Seems like the right thing to do would > be to pull the validation outside the loop. But you're right about this. The driver looks up its resources but still proceeds if they're not there (and will eventually try to submit URBs for the default pipe). I'll send a v2. Johan
On Wed, Oct 27, 2021 at 12:40 AM Johan Hovold <johan@kernel.org> wrote: > On Tue, Oct 26, 2021 at 10:35:37AM -0700, Brian Norris wrote: > > Seems like you're missing a changelog and a version number, since > > you've already sent previous versions of this patch. > > Seems like you're confusing me with someone else. Oops, you're correct :( It was only a week or two ago someone else was trying to patch this, and I didn't remember the "From" correctly. Sorry! > I'll send a v2. Looks better, thanks. Brian
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c index 426e39d4ccf0..2826654907d9 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.c +++ b/drivers/net/wireless/marvell/mwifiex/usb.c @@ -502,6 +502,9 @@ static int mwifiex_usb_probe(struct usb_interface *intf, atomic_set(&card->tx_cmd_urb_pending, 0); card->bulk_out_maxpktsize = le16_to_cpu(epd->wMaxPacketSize); + /* Reject broken descriptors. */ + if (card->bulk_out_maxpktsize == 0) + return -ENODEV; } }
Add the missing endpoint max-packet sanity check 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). 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 | 3 +++ 1 file changed, 3 insertions(+)