From: Mazin Al Haddad <hidden> Date: 2022-08-27 11:03:09
Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb()
due to the wrong endpoint type. There is no check for both the number
of endpoints and the type.
Fix it by adding a check for the number of endpoints and the
direction/type of the endpoints. If the endpoints do not match -ENODEV is
returned.
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
Modules linked in:
CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022
Workqueue: usb_hub_wq hub_event
Call Trace:
<TASK>
ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275
ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline]
ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline]
ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655
Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f
Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com
Signed-off-by: Mazin Al Haddad <redacted>
---
v2->v3 changes:
- Make use of helper functions instead of checking for direction
and type manually.
drivers/net/wireless/ath/ar5523/ar5523.c | 38 ++++++++++++++++++++++++
1 file changed, 38 insertions(+)
@@ -1581,8 +1581,46 @@ static int ar5523_probe(struct usb_interface *intf,structusb_device*dev=interface_to_usbdev(intf);structieee80211_hw*hw;structar5523*ar;+structusb_host_interface*host=intf->cur_altsetting;+structusb_endpoint_descriptor*cmd_tx,*cmd_rx,*data_tx,*data_rx;interror=-ENOMEM;+if(host->desc.bNumEndpoints!=4){+dev_err(&dev->dev,"Wrong number of endpoints\n");+return-ENODEV;+}++for(inti=0;i<host->desc.bNumEndpoints;++i){+structusb_endpoint_descriptor*ep=&host->endpoint[i].desc;++if(usb_endpoint_is_bulk_out(ep)){+if(!cmd_tx){+if(ep->bEndpointAddress==AR5523_CMD_TX_PIPE)+cmd_tx=ep;+}+if(!data_tx){+if(ep->bEndpointAddress==AR5523_DATA_TX_PIPE)+data_tx=ep;+}+}++if(usb_endpoint_is_bulk_in(ep)){+if(!cmd_rx){+if(ep->bEndpointAddress==AR5523_CMD_RX_PIPE)+cmd_rx=ep;+}+if(!data_rx){+if(ep->bEndpointAddress==AR5523_DATA_RX_PIPE)+data_rx=ep;+}+}+}++if(!cmd_tx||!data_tx||!cmd_rx||!data_rx){+dev_warn("wrong number of endpoints\n");+return-ENODEV;+}+/**Loadfirmwareifthedevicerequiresit.Thiswillreturn*-ENXIOonsuccessandwe'llgetcalledbackafertheusb
On Sat, Aug 27, 2022 at 02:01:49PM +0300, Mazin Al Haddad wrote:
quoted hunk
Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb()
due to the wrong endpoint type. There is no check for both the number
of endpoints and the type.
Fix it by adding a check for the number of endpoints and the
direction/type of the endpoints. If the endpoints do not match -ENODEV is
returned.
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
Modules linked in:
CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0
Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022
Workqueue: usb_hub_wq hub_event
Call Trace:
<TASK>
ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275
ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline]
ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline]
ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655
Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f
Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com
Signed-off-by: Mazin Al Haddad <redacted>
---
v2->v3 changes:
- Make use of helper functions instead of checking for direction
and type manually.
drivers/net/wireless/ath/ar5523/ar5523.c | 38 ++++++++++++++++++++++++
1 file changed, 38 insertions(+)
@@ -1581,8 +1581,46 @@ static int ar5523_probe(struct usb_interface *intf,structusb_device*dev=interface_to_usbdev(intf);structieee80211_hw*hw;structar5523*ar;+structusb_host_interface*host=intf->cur_altsetting;+structusb_endpoint_descriptor*cmd_tx,*cmd_rx,*data_tx,*data_rx;interror=-ENOMEM;+if(host->desc.bNumEndpoints!=4){+dev_err(&dev->dev,"Wrong number of endpoints\n");+return-ENODEV;+}++for(inti=0;i<host->desc.bNumEndpoints;++i){+structusb_endpoint_descriptor*ep=&host->endpoint[i].desc;++if(usb_endpoint_is_bulk_out(ep)){+if(!cmd_tx){+if(ep->bEndpointAddress==AR5523_CMD_TX_PIPE)+cmd_tx=ep;+}+if(!data_tx){+if(ep->bEndpointAddress==AR5523_DATA_TX_PIPE)+data_tx=ep;+}+}++if(usb_endpoint_is_bulk_in(ep)){+if(!cmd_rx){+if(ep->bEndpointAddress==AR5523_CMD_RX_PIPE)+cmd_rx=ep;+}+if(!data_rx){+if(ep->bEndpointAddress==AR5523_DATA_RX_PIPE)+data_rx=ep;+}+}+}++if(!cmd_tx||!data_tx||!cmd_rx||!data_rx){+dev_warn("wrong number of endpoints\n");+return-ENODEV;+}
So you save off all of these values, and then do not use them anywhere?
Why not properly save them to the device structure and then you can get
rid of the odd ar5523_cmd_tx_pipe() macros?
Also, I don't see why you can't use the USB core function for finding
endpoints, all you want is the first 2 bulk in and 2 bulk out, that
should be much simpler to use than the above long codebase.
thanks,
greg k-h
drivers/net/wireless/ath/ar5523/ar5523.c:1620:55: error: macro "dev_warn" requires 3 arguments, but only 1 given
1620 | dev_warn("wrong number of endpoints\n");
| ^
In file included from include/linux/device.h:15,
from include/linux/dma-mapping.h:7,
from include/linux/skbuff.h:31,
from drivers/net/wireless/ath/ar5523/ar5523.c:33:
include/linux/dev_printk.h:145: note: macro "dev_warn" defined here
145 | #define dev_warn(dev, fmt, ...) \
|
quoted
drivers/net/wireless/ath/ar5523/ar5523.c:1620:17: error: 'dev_warn' undeclared (first use in this function); did you mean '_dev_warn'?
1620 | dev_warn("wrong number of endpoints\n");
| ^~~~~~~~
| _dev_warn
drivers/net/wireless/ath/ar5523/ar5523.c:1620:17: note: each undeclared identifier is reported only once for each function it appears in
vim +/dev_warn +1620 drivers/net/wireless/ath/ar5523/ar5523.c
1577
1578 static int ar5523_probe(struct usb_interface *intf,
1579 const struct usb_device_id *id)
1580 {
1581 struct usb_device *dev = interface_to_usbdev(intf);
1582 struct ieee80211_hw *hw;
1583 struct ar5523 *ar;
1584 struct usb_host_interface *host = intf->cur_altsetting;
1585 struct usb_endpoint_descriptor *cmd_tx, *cmd_rx, *data_tx, *data_rx;
1586 int error = -ENOMEM;
1587
1588 if (host->desc.bNumEndpoints != 4) {
1589 dev_err(&dev->dev, "Wrong number of endpoints\n");
1590 return -ENODEV;
1591 }
1592
1593 for (int i = 0; i < host->desc.bNumEndpoints; ++i) {
1594 struct usb_endpoint_descriptor *ep = &host->endpoint[i].desc;
1595
1596 if (usb_endpoint_is_bulk_out(ep)) {
1597 if (!cmd_tx) {
1598 if (ep->bEndpointAddress == AR5523_CMD_TX_PIPE)
1599 cmd_tx = ep;
1600 }
1601 if (!data_tx) {
1602 if (ep->bEndpointAddress == AR5523_DATA_TX_PIPE)
1603 data_tx = ep;
1604 }
1605 }
1606
1607 if (usb_endpoint_is_bulk_in(ep)) {
1608 if (!cmd_rx) {
1609 if (ep->bEndpointAddress == AR5523_CMD_RX_PIPE)
1610 cmd_rx = ep;
1611 }
1612 if (!data_rx) {
1613 if (ep->bEndpointAddress == AR5523_DATA_RX_PIPE)
1614 data_rx = ep;
1615 }
1616 }
1617 }
1618
1619 if (!cmd_tx || !data_tx || !cmd_rx || !data_rx) {
1620 dev_warn("wrong number of endpoints\n");
1621 return -ENODEV;
1622 }
1623
1624 /*
1625 * Load firmware if the device requires it. This will return
1626 * -ENXIO on success and we'll get called back afer the usb
1627 * id changes to indicate that the firmware is present.
1628 */
1629 if (id->driver_info & AR5523_FLAG_PRE_FIRMWARE)
1630 return ar5523_load_firmware(dev);
1631
1632
1633 hw = ieee80211_alloc_hw(sizeof(*ar), &ar5523_ops);
1634 if (!hw)
1635 goto out;
1636 SET_IEEE80211_DEV(hw, &intf->dev);
1637
1638 ar = hw->priv;
1639 ar->hw = hw;
1640 ar->dev = dev;
1641 mutex_init(&ar->mutex);
1642
1643 INIT_DELAYED_WORK(&ar->stat_work, ar5523_stat_work);
1644 timer_setup(&ar->tx_wd_timer, ar5523_tx_wd_timer, 0);
1645 INIT_WORK(&ar->tx_wd_work, ar5523_tx_wd_work);
1646 INIT_WORK(&ar->tx_work, ar5523_tx_work);
1647 INIT_LIST_HEAD(&ar->tx_queue_pending);
1648 INIT_LIST_HEAD(&ar->tx_queue_submitted);
1649 spin_lock_init(&ar->tx_data_list_lock);
1650 atomic_set(&ar->tx_nr_total, 0);
1651 atomic_set(&ar->tx_nr_pending, 0);
1652 init_waitqueue_head(&ar->tx_flush_waitq);
1653
1654 atomic_set(&ar->rx_data_free_cnt, 0);
1655 INIT_WORK(&ar->rx_refill_work, ar5523_rx_refill_work);
1656 INIT_LIST_HEAD(&ar->rx_data_free);
1657 INIT_LIST_HEAD(&ar->rx_data_used);
1658 spin_lock_init(&ar->rx_data_list_lock);
1659
1660 ar->wq = create_singlethread_workqueue("ar5523");
1661 if (!ar->wq) {
1662 ar5523_err(ar, "Could not create wq\n");
1663 goto out_free_ar;
1664 }
1665
1666 error = ar5523_alloc_rx_bufs(ar);
1667 if (error) {
1668 ar5523_err(ar, "Could not allocate rx buffers\n");
1669 goto out_free_wq;
1670 }
1671
1672 error = ar5523_alloc_rx_cmd(ar);
1673 if (error) {
1674 ar5523_err(ar, "Could not allocate rx command buffers\n");
1675 goto out_free_rx_bufs;
1676 }
1677
1678 error = ar5523_alloc_tx_cmd(ar);
1679 if (error) {
1680 ar5523_err(ar, "Could not allocate tx command buffers\n");
1681 goto out_free_rx_cmd;
1682 }
1683
1684 error = ar5523_submit_rx_cmd(ar);
1685 if (error) {
1686 ar5523_err(ar, "Failed to submit rx cmd\n");
1687 goto out_free_tx_cmd;
1688 }
1689
1690 /*
1691 * We're now ready to send/receive firmware commands.
1692 */
1693 error = ar5523_host_available(ar);
1694 if (error) {
1695 ar5523_err(ar, "could not initialize adapter\n");
1696 goto out_cancel_rx_cmd;
1697 }
1698
1699 error = ar5523_get_max_rxsz(ar);
1700 if (error) {
1701 ar5523_err(ar, "could not get caps from adapter\n");
1702 goto out_cancel_rx_cmd;
1703 }
1704
1705 error = ar5523_get_devcap(ar);
1706 if (error) {
1707 ar5523_err(ar, "could not get caps from adapter\n");
1708 goto out_cancel_rx_cmd;
1709 }
1710
1711 error = ar5523_get_devstatus(ar);
1712 if (error != 0) {
1713 ar5523_err(ar, "could not get device status\n");
1714 goto out_cancel_rx_cmd;
1715 }
1716
1717 ar5523_info(ar, "MAC/BBP AR5523, RF AR%c112\n",
1718 (id->driver_info & AR5523_FLAG_ABG) ? '5' : '2');
1719
1720 ar->vif = NULL;
1721 ieee80211_hw_set(hw, HAS_RATE_CONTROL);
1722 ieee80211_hw_set(hw, RX_INCLUDES_FCS);
1723 ieee80211_hw_set(hw, SIGNAL_DBM);
1724 hw->extra_tx_headroom = sizeof(struct ar5523_tx_desc) +
1725 sizeof(struct ar5523_chunk);
1726 hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
1727 hw->queues = 1;
1728
1729 error = ar5523_init_modes(ar);
1730 if (error)
1731 goto out_cancel_rx_cmd;
1732
1733 wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
1734
1735 usb_set_intfdata(intf, hw);
1736
1737 error = ieee80211_register_hw(hw);
1738 if (error) {
1739 ar5523_err(ar, "could not register device\n");
1740 goto out_cancel_rx_cmd;
1741 }
1742
1743 ar5523_info(ar, "Found and initialized AR5523 device\n");
1744 return 0;
1745
1746 out_cancel_rx_cmd:
1747 ar5523_cancel_rx_cmd(ar);
1748 out_free_tx_cmd:
1749 ar5523_free_tx_cmd(ar);
1750 out_free_rx_cmd:
1751 ar5523_free_rx_cmd(ar);
1752 out_free_rx_bufs:
1753 ar5523_free_rx_bufs(ar);
1754 out_free_wq:
1755 destroy_workqueue(ar->wq);
1756 out_free_ar:
1757 ieee80211_free_hw(hw);
1758 out:
1759 return error;
1760 }
1761
--
0-DAY CI Kernel Test Service
https://01.org/lkp