* [PATCH] net: ax88179: add proper error handling of usb read errors
@ 2022-04-04 15:10 David Kahurani
2022-04-04 15:31 ` Dan Carpenter
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: David Kahurani @ 2022-04-04 15:10 UTC (permalink / raw)
To: netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, paskripkin,
David Kahurani
Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
such reads as errors and handle them correctly.
ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -71
ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0002: -71
=====================================================
BUG: KMSAN: uninit-value in ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
BUG: KMSAN: uninit-value in ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
usbnet_probe+0x1284/0x4140 drivers/net/usb/usbnet.c:1747
usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396
really_probe+0x67d/0x1510 drivers/base/dd.c:596
__driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
driver_probe_device drivers/base/dd.c:781 [inline]
__device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
__device_attach+0x593/0x8e0 drivers/base/dd.c:969
device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487
device_add+0x1d3e/0x2400 drivers/base/core.c:3394
usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170
usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238
usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293
really_probe+0x67d/0x1510 drivers/base/dd.c:596
__driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
driver_probe_device drivers/base/dd.c:781 [inline]
__device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
__device_attach+0x593/0x8e0 drivers/base/dd.c:969
device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
Signed-off-by: David Kahurani <k.kahurani@gmail.com>
Reported-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
---
drivers/net/usb/ax88179_178a.c | 255 +++++++++++++++++++++++++++------
1 file changed, 213 insertions(+), 42 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e2fa56b92..b5e114bed 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -185,8 +185,9 @@ static const struct {
{7, 0xcc, 0x4c, 0x18, 8},
};
-static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data, int in_pm)
+static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data, int in_pm)
{
int ret;
int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
@@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
value, index, data, size);
- if (unlikely(ret < 0))
+ if (unlikely(ret < size)) {
+ ret = ret < 0 ? ret : -ENODATA;
+
netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
index, ret);
+ }
return ret;
}
@@ -249,19 +253,26 @@ static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
}
}
-static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
- u16 index, u16 size, void *data)
+static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data)
{
int ret;
if (2 == size) {
u16 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+
+ if (ret < 0)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+
+ if (ret < 0)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
@@ -290,19 +301,23 @@ static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
return ret;
}
-static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data)
+static int __must_check ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+ u16 index, u16 size, void *data)
{
int ret;
if (2 == size) {
u16 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret < 0)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret < 0)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
@@ -354,8 +369,15 @@ static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
{
struct usbnet *dev = netdev_priv(netdev);
u16 res;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
return res;
}
@@ -427,19 +449,31 @@ static int ax88179_suspend(struct usb_interface *intf, pm_message_t message)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_suspend(intf, message);
/* Disable RX path */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read MEDIUM_STATUS_MODE: %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
/* Force bulk-in zero length */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
+ 2, 2, &tmp16);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d\n", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_BZ | AX_PHYPWR_RSTCTL_IPRL;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
@@ -462,6 +496,7 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
{
u16 tmp16;
u8 tmp8;
+ int ret;
int (*fnr)(struct usbnet *, u8, u16, u16, u16, void *);
int (*fnw)(struct usbnet *, u8, u16, u16, u16, const void *);
@@ -481,11 +516,19 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
/* Enable Auto Detach bit */
tmp8 = 0;
- fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ if (ret < 0) {
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT: %d", ret);
+ return ret;
+ }
tmp8 |= AX_CLK_SELECT_ULR;
fnw(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
- fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ if (ret < 0) {
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_AT;
fnw(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
@@ -497,6 +540,7 @@ static int ax88179_resume(struct usb_interface *intf)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_link_change(dev, 0, 0);
@@ -515,7 +559,14 @@ static int ax88179_resume(struct usb_interface *intf)
ax88179_auto_detach(dev, 1);
/* Enable clock */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT %d\n", ret);
+
+ return ret;
+ }
+
tmp8 |= AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
msleep(100);
@@ -951,23 +1002,45 @@ static int
ax88179_set_features(struct net_device *net, netdev_features_t features)
{
u8 tmp;
+ int ret;
struct usbnet *dev = netdev_priv(net);
netdev_features_t changed = net->features ^ features;
if (changed & NETIF_F_IP_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_IPV6_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_RXCSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
@@ -980,19 +1053,36 @@ static int ax88179_change_mtu(struct net_device *net, int new_mtu)
{
struct usbnet *dev = netdev_priv(net);
u16 tmp16;
+ int ret;
net->mtu = new_mtu;
dev->hard_mtu = net->mtu + net->hard_header_len;
if (net->mtu > 1500) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret < 0){
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 |= AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret < 0){
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
@@ -1045,6 +1135,7 @@ static int ax88179_check_eeprom(struct usbnet *dev)
u8 i, buf, eeprom[20];
u16 csum, delay = HZ / 10;
unsigned long jtimeout;
+ int ret;
/* Read EEPROM content */
for (i = 0; i < 6; i++) {
@@ -1060,16 +1151,30 @@ static int ax88179_check_eeprom(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &buf);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &buf);
+
+ if (ret < 0) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (buf & EEP_BUSY);
- __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 2, 2, &eeprom[i * 2], 0);
+ ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 2, 2, &eeprom[i * 2], 0);
+
+ if (ret < 0) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_DATA_LOW: %d\n",
+ ret);
+ return ret;
+ }
if ((i == 0) && (eeprom[0] == 0xFF))
return -EINVAL;
@@ -1149,12 +1254,19 @@ static int ax88179_convert_old_led(struct usbnet *dev, u16 *ledvalue)
static int ax88179_led_setting(struct usbnet *dev)
{
+ int ret;
u8 ledfd, value = 0;
u16 tmp, ledact, ledlink, ledvalue = 0, delay = HZ / 10;
unsigned long jtimeout;
/* Check AX88179 version. UA1 or UA2*/
- ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read GENERAL_STATUS: %d\n",
+ ret);
+ return ret;
+ }
if (!(value & AX_SECLD)) { /* UA1 */
value = AX_GPIO_CTRL_GPIO3EN | AX_GPIO_CTRL_GPIO2EN |
@@ -1178,20 +1290,40 @@ static int ax88179_led_setting(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &value);
+ if (ret < 0){
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (value & EEP_BUSY);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
1, 1, &value);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_HIGH: %d\n",
+ ret);
+ return ret;
+ }
+
+
ledvalue = (value << 8);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 1, 1, &value);
+
+ if (ret < 0) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_LOW: %d",
+ ret);
+ return ret;
+ }
+
ledvalue |= value;
/* load internal ROM for defaule setting */
@@ -1213,12 +1345,22 @@ static int ax88179_led_setting(struct usbnet *dev)
ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
GMII_PHYPAGE, 2, &tmp);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_LED_ACT, 2, &ledact);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_LED_ACT, 2, &ledact);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
GMII_LED_LINK, 2, &ledlink);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
+
ledact &= GMII_LED_ACTIVE_MASK;
ledlink &= GMII_LED_LINK_MASK;
@@ -1295,6 +1437,7 @@ static int ax88179_led_setting(struct usbnet *dev)
static void ax88179_get_mac_addr(struct usbnet *dev)
{
u8 mac[ETH_ALEN];
+ int ret;
memset(mac, 0, sizeof(mac));
@@ -1303,8 +1446,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
netif_dbg(dev, ifup, dev->net,
"MAC address read from device tree");
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
ETH_ALEN, mac);
+
+ if (ret < 0)
+ netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
+
netif_dbg(dev, ifup, dev->net,
"MAC address read from ASIX chip");
}
@@ -1572,6 +1719,7 @@ static int ax88179_link_reset(struct usbnet *dev)
u16 mode, tmp16, delay = HZ / 10;
u32 tmp32 = 0x40000000;
unsigned long jtimeout;
+ int ret;
jtimeout = jiffies + delay;
while (tmp32 & 0x40000000) {
@@ -1581,7 +1729,13 @@ static int ax88179_link_reset(struct usbnet *dev)
&ax179_data->rxctl);
/*link up, check the usb device control TX FIFO full or empty*/
- ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+ ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+
+ if (ret < 0) {
+ netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return 0;
@@ -1590,11 +1744,21 @@ static int ax88179_link_reset(struct usbnet *dev)
mode = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
AX_MEDIUM_RXFLOW_CTRLEN;
- ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
- 1, 1, &link_sts);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
+ 1, 1, &link_sts);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_PHY_PHYSR, 2, &tmp16);
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read LINK_STATUS: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_PHY_PHYSR, 2, &tmp16);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
return 0;
@@ -1732,9 +1896,16 @@ static int ax88179_reset(struct usbnet *dev)
static int ax88179_stop(struct usbnet *dev)
{
u16 tmp16;
+ int ret;
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
+
+ if (ret < 0){
+ netdev_dbg(dev->net, "Failed to read STATUS_MODE: %d\n", ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 15:10 [PATCH] net: ax88179: add proper error handling of usb read errors David Kahurani
@ 2022-04-04 15:31 ` Dan Carpenter
2022-04-13 12:36 ` David Kahurani
2022-04-04 16:50 ` Pavel Skripkin
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2022-04-04 15:31 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba,
linux-kernel, linux-usb, phil, syzkaller-bugs, arnd, paskripkin
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index e2fa56b92..b5e114bed 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -185,8 +185,9 @@ static const struct {
> {7, 0xcc, 0x4c, 0x18, 8},
> };
>
> -static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> - u16 size, void *data, int in_pm)
> +static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
> + u16 value, u16 index, u16 size,
> + void *data, int in_pm)
> {
> int ret;
> int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> value, index, data, size);
>
> - if (unlikely(ret < 0))
> + if (unlikely(ret < size)) {
> + ret = ret < 0 ? ret : -ENODATA;
> +
> netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> index, ret);
> + }
>
> return ret;
It would be better to make __ax88179_read_cmd() return 0 on success
instead of returning size on success. Non-standard returns lead to bugs.
> @@ -1060,16 +1151,30 @@ static int ax88179_check_eeprom(struct usbnet *dev)
>
> jtimeout = jiffies + delay;
> do {
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> - 1, 1, &buf);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> + 1, 1, &buf);
> +
> + if (ret < 0) {
> + netdev_dbg(dev->net,
> + "Failed to read SROM_CMD: %d\n",
> + ret);
> + return ret;
> + }
>
> if (time_after(jiffies, jtimeout))
> return -EINVAL;
The indenting here is wrong. Run scripts/checkpatch.pl on your patches.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 15:10 [PATCH] net: ax88179: add proper error handling of usb read errors David Kahurani
2022-04-04 15:31 ` Dan Carpenter
@ 2022-04-04 16:50 ` Pavel Skripkin
2022-04-11 15:11 ` Andy Shevchenko
2022-04-05 9:44 ` Paolo Abeni
2022-04-05 20:44 ` kernel test robot
3 siblings, 1 reply; 23+ messages in thread
From: Pavel Skripkin @ 2022-04-04 16:50 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd
[-- Attachment #1.1: Type: text/plain, Size: 5116 bytes --]
Hi David,
On 4/4/22 18:10, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
> such reads as errors and handle them correctly.
>
> ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -71
> ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0002: -71
> =====================================================
> BUG: KMSAN: uninit-value in ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
> BUG: KMSAN: uninit-value in ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
> ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
> ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
> ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
> usbnet_probe+0x1284/0x4140 drivers/net/usb/usbnet.c:1747
> usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396
> really_probe+0x67d/0x1510 drivers/base/dd.c:596
> __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
> driver_probe_device drivers/base/dd.c:781 [inline]
> __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
> bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
> __device_attach+0x593/0x8e0 drivers/base/dd.c:969
> device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
> bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487
> device_add+0x1d3e/0x2400 drivers/base/core.c:3394
> usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170
> usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238
> usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293
> really_probe+0x67d/0x1510 drivers/base/dd.c:596
> __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
> driver_probe_device drivers/base/dd.c:781 [inline]
> __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
> bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
> __device_attach+0x593/0x8e0 drivers/base/dd.c:969
> device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
>
I'd personally cut this log a bit and would add this part of the initial
report
Local variable eeprom.i created at:
ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1045 [inline]
ax88179_led_setting+0x2e2/0x30b0 drivers/net/usb/ax88179_178a.c:1168
ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
Since it shows exactly where problem comes from.
I do not insist, just IMO
> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> Reported-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> ---
It's indeed a bug fix, so fixes tag is needed
Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
gigabit ethernet adapter driver")
> drivers/net/usb/ax88179_178a.c | 255 +++++++++++++++++++++++++++------
> 1 file changed, 213 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index e2fa56b92..b5e114bed 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -185,8 +185,9 @@ static const struct {
> {7, 0xcc, 0x4c, 0x18, 8},
> };
>
> -static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> - u16 size, void *data, int in_pm)
> +static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
> + u16 value, u16 index, u16 size,
> + void *data, int in_pm)
> {
> int ret;
> int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> value, index, data, size);
>
> - if (unlikely(ret < 0))
> + if (unlikely(ret < size)) {
> + ret = ret < 0 ? ret : -ENODATA;
> +
> netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> index, ret);
> + }
>
> return ret;
> }
> @@ -249,19 +253,26 @@ static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
> }
> }
>
> -static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> - u16 index, u16 size, void *data)
> +static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
> + u16 value, u16 index, u16 size,
> + void *data)
> {
> int ret;
>
> if (2 == size) {
> u16 buf;
> ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
> +
> + if (ret < 0)
> + return ret;
Empty line after assignment and before check looks redundant.
> le16_to_cpus(&buf);
> *((u16 *)data) = buf;
> } else if (4 == size) {
> u32 buf;
> ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
> +
> + if (ret < 0)
> + return ret;
Same here
In general, looks good, but, please, fix up indenting and other small
issues.
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 15:10 [PATCH] net: ax88179: add proper error handling of usb read errors David Kahurani
2022-04-04 15:31 ` Dan Carpenter
2022-04-04 16:50 ` Pavel Skripkin
@ 2022-04-05 9:44 ` Paolo Abeni
2022-04-05 20:44 ` kernel test robot
3 siblings, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2022-04-05 9:44 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, paskripkin
On Mon, 2022-04-04 at 18:10 +0300, David Kahurani wrote:
> @@ -354,8 +369,15 @@ static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
> {
> struct usbnet *dev = netdev_priv(netdev);
> u16 res;
> + int ret;
> +
> + ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
> +
> + if (ret < 0){
For the records: another recurring indentation issue is the lack of a
whitespace after the ')'.
Please address all the issues reported by scritps/checkpatch.pl before
submitting the next version, thanks!
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 15:10 [PATCH] net: ax88179: add proper error handling of usb read errors David Kahurani
` (2 preceding siblings ...)
2022-04-05 9:44 ` Paolo Abeni
@ 2022-04-05 20:44 ` kernel test robot
3 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2022-04-05 20:44 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: kbuild-all, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba,
linux-kernel, linux-usb, phil, syzkaller-bugs, arnd, paskripkin,
David Kahurani
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
[also build test WARNING on net-next/master horms-ipvs/master linus/master v5.18-rc1 next-20220405]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/David-Kahurani/net-ax88179-add-proper-error-handling-of-usb-read-errors/20220404-231226
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 692930cc435099580a4b9e32fa781b0688c18439
config: i386-randconfig-m021-20220404 (https://download.01.org/0day-ci/archive/20220406/202204060410.mWlznHB9-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
drivers/net/usb/ax88179_178a.c:1164 ax88179_check_eeprom() warn: inconsistent indenting
vim +1164 drivers/net/usb/ax88179_178a.c
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1132
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1133 static int ax88179_check_eeprom(struct usbnet *dev)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1134 {
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1135 u8 i, buf, eeprom[20];
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1136 u16 csum, delay = HZ / 10;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1137 unsigned long jtimeout;
c006257c3aa36f6 David Kahurani 2022-04-04 1138 int ret;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1139
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1140 /* Read EEPROM content */
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1141 for (i = 0; i < 6; i++) {
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1142 buf = i;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1143 if (ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_SROM_ADDR,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1144 1, 1, &buf) < 0)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1145 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1146
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1147 buf = EEP_RD;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1148 if (ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1149 1, 1, &buf) < 0)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1150 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1151
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1152 jtimeout = jiffies + delay;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1153 do {
c006257c3aa36f6 David Kahurani 2022-04-04 1154 ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1155 1, 1, &buf);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1156
c006257c3aa36f6 David Kahurani 2022-04-04 1157 if (ret < 0) {
c006257c3aa36f6 David Kahurani 2022-04-04 1158 netdev_dbg(dev->net,
c006257c3aa36f6 David Kahurani 2022-04-04 1159 "Failed to read SROM_CMD: %d\n",
c006257c3aa36f6 David Kahurani 2022-04-04 1160 ret);
c006257c3aa36f6 David Kahurani 2022-04-04 1161 return ret;
c006257c3aa36f6 David Kahurani 2022-04-04 1162 }
c006257c3aa36f6 David Kahurani 2022-04-04 1163
e2ca90c276e1fc4 Freddy Xin 2013-03-02 @1164 if (time_after(jiffies, jtimeout))
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1165 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1166
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1167 } while (buf & EEP_BUSY);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1168
c006257c3aa36f6 David Kahurani 2022-04-04 1169 ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1170 2, 2, &eeprom[i * 2], 0);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1171
c006257c3aa36f6 David Kahurani 2022-04-04 1172 if (ret < 0) {
c006257c3aa36f6 David Kahurani 2022-04-04 1173 netdev_dbg(dev->net,
c006257c3aa36f6 David Kahurani 2022-04-04 1174 "Failed to read SROM_DATA_LOW: %d\n",
c006257c3aa36f6 David Kahurani 2022-04-04 1175 ret);
c006257c3aa36f6 David Kahurani 2022-04-04 1176 return ret;
c006257c3aa36f6 David Kahurani 2022-04-04 1177 }
c006257c3aa36f6 David Kahurani 2022-04-04 1178
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1179 if ((i == 0) && (eeprom[0] == 0xFF))
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1180 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1181 }
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1182
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1183 csum = eeprom[6] + eeprom[7] + eeprom[8] + eeprom[9];
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1184 csum = (csum >> 8) + (csum & 0xff);
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1185 if ((csum + eeprom[10]) != 0xff)
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1186 return -EINVAL;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1187
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1188 return 0;
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1189 }
e2ca90c276e1fc4 Freddy Xin 2013-03-02 1190
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 16:50 ` Pavel Skripkin
@ 2022-04-11 15:11 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2022-04-11 15:11 UTC (permalink / raw)
To: Pavel Skripkin
Cc: David Kahurani, netdev, syzbot+d3dbdf31fbe9d8f5f311,
David S. Miller, Jason Gunthorpe, Jakub Kicinski,
Linux Kernel Mailing List, USB, Phillip Potter, syzkaller-bugs,
Arnd Bergmann
On Tue, Apr 5, 2022 at 3:05 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
> On 4/4/22 18:10, David Kahurani wrote:
> > Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
> > such reads as errors and handle them correctly.
> I'd personally cut this log a bit and would add this part of the initial
> report
>
> Local variable eeprom.i created at:
> ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1045 [inline]
> ax88179_led_setting+0x2e2/0x30b0 drivers/net/usb/ax88179_178a.c:1168
> ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
>
> Since it shows exactly where problem comes from.
>
> I do not insist, just IMO
I insist though. It will reduce the resource consumption (i.e. storage
and network load on cloning / pulling) a lot (taking into account
multiplier of how many Linux kernel source copies are around the
globe) and hence become more environmentally friendly.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-04 15:31 ` Dan Carpenter
@ 2022-04-13 12:36 ` David Kahurani
2022-04-13 15:32 ` Dan Carpenter
0 siblings, 1 reply; 23+ messages in thread
From: David Kahurani @ 2022-04-13 12:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Pavel Skripkin
On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
Hi Dan
> > int ret;
> > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> > @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> > ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > value, index, data, size);
> >
> > - if (unlikely(ret < 0))
> > + if (unlikely(ret < size)) {
> > + ret = ret < 0 ? ret : -ENODATA;
> > +
> > netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> > index, ret);
> > + }
> >
> > return ret;
>
> It would be better to make __ax88179_read_cmd() return 0 on success
> instead of returning size on success. Non-standard returns lead to bugs.
>
I don't suppose this would have much effect on the structure of the
code and indeed plan to do this but just some minor clarification.
Isn't it standard for reader functions to return the number of bytes read?
Regards,
David.
>
> > @@ -1060,16 +1151,30 @@ static int ax88179_check_eeprom(struct usbnet *dev)
> >
> > jtimeout = jiffies + delay;
> > do {
> > - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> > - 1, 1, &buf);
> > + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> > + 1, 1, &buf);
> > +
> > + if (ret < 0) {
> > + netdev_dbg(dev->net,
> > + "Failed to read SROM_CMD: %d\n",
> > + ret);
> > + return ret;
> > + }
> >
> > if (time_after(jiffies, jtimeout))
> > return -EINVAL;
>
> The indenting here is wrong. Run scripts/checkpatch.pl on your patches.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-13 12:36 ` David Kahurani
@ 2022-04-13 15:32 ` Dan Carpenter
2022-04-14 7:31 ` Oliver Neukum
0 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2022-04-13 15:32 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Pavel Skripkin
On Wed, Apr 13, 2022 at 03:36:57PM +0300, David Kahurani wrote:
> On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hi Dan
>
> > > int ret;
> > > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> > > @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> > > ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > > value, index, data, size);
> > >
> > > - if (unlikely(ret < 0))
> > > + if (unlikely(ret < size)) {
> > > + ret = ret < 0 ? ret : -ENODATA;
> > > +
> > > netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> > > index, ret);
> > > + }
> > >
> > > return ret;
> >
> > It would be better to make __ax88179_read_cmd() return 0 on success
> > instead of returning size on success. Non-standard returns lead to bugs.
> >
>
> I don't suppose this would have much effect on the structure of the
> code and indeed plan to do this but just some minor clarification.
>
> Isn't it standard for reader functions to return the number of bytes read?
>
Not really.
There are some functions that do it, but it has historically lead to bug
after bug. For example, see commit 719b8f2850d3 ("USB: add
usb_control_msg_send() and usb_control_msg_recv()") where USB is moving
away from that to avoid bugs.
If you return zero on success then it's simple:
if (ret)
return ret;
If you return the bytes people will try call kinds of things:
if (ret)
return ret;
Bug: Now the driver is broken. (Not everyone can test the hardware).
if (ret != size)
return ret;
Bug: returns a positive.
if (ret != size)
return -EIO;
Bug: forgot to propagate the error code.
if (ret < sizeof(foo))
return -EIO;
Bug: because of type promotion negative error codes are treated as
success.
if (ret < 0)
return ret;
Bug: buffer partially filled. Information leak.
If you return the bytes then the only correct way to write error
handling is:
if (ret < 0)
return ret;
if (ret != size)
return -EIO;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-13 15:32 ` Dan Carpenter
@ 2022-04-14 7:31 ` Oliver Neukum
2022-04-14 8:21 ` Dan Carpenter
0 siblings, 1 reply; 23+ messages in thread
From: Oliver Neukum @ 2022-04-14 7:31 UTC (permalink / raw)
To: Dan Carpenter, David Kahurani
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Pavel Skripkin
On 13.04.22 17:32, Dan Carpenter wrote:
>
> Bug: buffer partially filled. Information leak.
>
> If you return the bytes then the only correct way to write error
> handling is:
>
> if (ret < 0)
> return ret;
> if (ret != size)
> return -EIO;
>
You have to make up your mind on whether you ever need to read
answer of a length not known before you try it. The alternative of
passing a pointer to an integer for length is worse.
Regards
Oliver
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-14 7:31 ` Oliver Neukum
@ 2022-04-14 8:21 ` Dan Carpenter
2022-04-14 9:13 ` Oliver Neukum
0 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2022-04-14 8:21 UTC (permalink / raw)
To: Oliver Neukum
Cc: David Kahurani, netdev, syzbot, davem, jgg, kuba, linux-kernel,
linux-usb, Phillip Potter, syzkaller-bugs, arnd, Pavel Skripkin
On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
>
>
> On 13.04.22 17:32, Dan Carpenter wrote:
> >
> > Bug: buffer partially filled. Information leak.
> >
> > If you return the bytes then the only correct way to write error
> > handling is:
> >
> > if (ret < 0)
> > return ret;
> > if (ret != size)
> > return -EIO;
> >
> You have to make up your mind on whether you ever need to read
> answer of a length not known before you try it. The alternative of
> passing a pointer to an integer for length is worse.
How is it worse? Can you give an example, so I will write a static
checker rule for it?
There used to be more APIs that consistently caused bug after bug where
we mixed positives success values with negative error codes. We
converted some bad offenders to return the positive as a parameter
and I was really happy about that.
Another example I used to see a lot is request_irq() saved to an
unsigned. These days I think GCC warns about that? Maybe the build
bots get to it before I do.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-14 8:21 ` Dan Carpenter
@ 2022-04-14 9:13 ` Oliver Neukum
0 siblings, 0 replies; 23+ messages in thread
From: Oliver Neukum @ 2022-04-14 9:13 UTC (permalink / raw)
To: Dan Carpenter, Oliver Neukum
Cc: David Kahurani, netdev, syzbot, davem, jgg, kuba, linux-kernel,
linux-usb, Phillip Potter, syzkaller-bugs, arnd, Pavel Skripkin
On 14.04.22 10:21, Dan Carpenter wrote:
> On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
>>
>> On 13.04.22 17:32, Dan Carpenter wrote:
>>> Bug: buffer partially filled. Information leak.
>>>
>>> If you return the bytes then the only correct way to write error
>>> handling is:
>>>
>>> if (ret < 0)
>>> return ret;
>>> if (ret != size)
>>> return -EIO;
>>>
>> You have to make up your mind on whether you ever need to read
>> answer of a length not known before you try it. The alternative of
>> passing a pointer to an integer for length is worse.
> How is it worse? Can you give an example, so I will write a static
> checker rule for it?
My favorite example would be:
int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe,
void *data, int len, int *actual_length, int timeout)
The actual_length parameter is horrible. Now, there are situations like this
where this pattern is unavoidable, because in addition to an error you
can get a partial completion of an operation.
Do I see it correctly that you would prefer this pattern even if
you could report either an error or a result in the function?
> There used to be more APIs that consistently caused bug after bug where
> we mixed positives success values with negative error codes. We
> converted some bad offenders to return the positive as a parameter
> and I was really happy about that.
>
> Another example I used to see a lot is request_irq() saved to an
> unsigned. These days I think GCC warns about that? Maybe the build
> bots get to it before I do.
>
That needs to be checked for.. In fact while we are at it, do we check
for integer overflows?
Regards
Oliver
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-05-14 13:32 David Kahurani
2022-05-14 16:51 ` Pavel Skripkin
@ 2022-05-16 19:31 ` Jakub Kicinski
1 sibling, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2022-05-16 19:31 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
On Sat, 14 May 2022 16:32:34 +0300 David Kahurani wrote:
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> - 1, 1, &buf);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> + 1, 1, &buf);
> + if (ret) {
> + netdev_dbg(dev->net,
> + "Failed to read SROM_CMD: %d\n",
> + ret);
> + return ret;
> + }
>
> if (time_after(jiffies, jtimeout))
> return -EINVAL;
>
> } while (buf & EEP_BUSY);
I agree with Pavel, this seems unnecessarily strict. If the error is
not ENODEV we can keep looping.
> @@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
> &ax179_data->rxctl);
>
> /*link up, check the usb device control TX FIFO full or empty*/
> - ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> + ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +
> + if (ret) {
> + netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
> + ret);
> + return ret;
> + }
Please don't add any empty lines within the error checking.
Empty lines are supposed to separate logically separate blocks of code.
Error checking is very much logically part of the call. And no empty
line betwee netdev_dbg() and return ret; either. In this submission you
have all possible configurations of having or not having empty lines
before the if or before the return. None of them should be there, and
please be consistent.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-05-14 18:54 ` Dan Carpenter
@ 2022-05-14 18:57 ` Pavel Skripkin
0 siblings, 0 replies; 23+ messages in thread
From: Pavel Skripkin @ 2022-05-14 18:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Kahurani, netdev, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg,
kuba, linux-kernel, linux-usb, phil, syzkaller-bugs, arnd
[-- Attachment #1.1: Type: text/plain, Size: 563 bytes --]
Hi Dan,
On 5/14/22 21:54, Dan Carpenter wrote:
> On Sat, May 14, 2022 at 07:51:12PM +0300, Pavel Skripkin wrote:
>> And you can send new version as reply using --in-reply= option of git
>> send-email. It helps a lot with finding previous version, since all version
>> are linked in one thread
>
> This is discouraged these days. It makes for long confusing threads.
> But more importantly, it apparently messes up patchwork.
>
Ok, but at least leaving a link to the previous version(s) seems
reasonable.
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-05-14 16:51 ` Pavel Skripkin
@ 2022-05-14 18:54 ` Dan Carpenter
2022-05-14 18:57 ` Pavel Skripkin
0 siblings, 1 reply; 23+ messages in thread
From: Dan Carpenter @ 2022-05-14 18:54 UTC (permalink / raw)
To: Pavel Skripkin
Cc: David Kahurani, netdev, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg,
kuba, linux-kernel, linux-usb, phil, syzkaller-bugs, arnd
On Sat, May 14, 2022 at 07:51:12PM +0300, Pavel Skripkin wrote:
> And you can send new version as reply using --in-reply= option of git
> send-email. It helps a lot with finding previous version, since all version
> are linked in one thread
This is discouraged these days. It makes for long confusing threads.
But more importantly, it apparently messes up patchwork.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-05-14 13:32 David Kahurani
@ 2022-05-14 16:51 ` Pavel Skripkin
2022-05-14 18:54 ` Dan Carpenter
2022-05-16 19:31 ` Jakub Kicinski
1 sibling, 1 reply; 23+ messages in thread
From: Pavel Skripkin @ 2022-05-14 16:51 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
[-- Attachment #1.1: Type: text/plain, Size: 2908 bytes --]
Hi David,
On 5/14/22 16:32, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> ---
<--- here (*)
> drivers/net/usb/ax88179_178a.c | 281 ++++++++++++++++++++++++++-------
> 1 file changed, 227 insertions(+), 54 deletions(-)
>
I don't see any error in that patch, but I had to find previous versions
of that patch in my inbox.
Usually new versions of single patches are linked in one thread and have
a version number in a title. You can generate patch with version using
-v option of git format-patch like:
$ git format-patch -v2 HEAD~
And you can send new version as reply using --in-reply= option of git
send-email. It helps a lot with finding previous version, since all
version are linked in one thread
And all updates from version to version should be put under --- (*),
since it's hard to remember why previous version was rejected.
> jtimeout = jiffies + delay;
> do {
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> - 1, 1, &buf);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> + 1, 1, &buf);
> + if (ret) {
> + netdev_dbg(dev->net,
> + "Failed to read SROM_CMD: %d\n",
> + ret);
> + return ret;
> + }
>
> if (time_after(jiffies, jtimeout))
> return -EINVAL;
>
> } while (buf & EEP_BUSY);
I think, this change might be dangerous. Maybe it should be done in the
same way as in asix driver [1]. Code polls for some register after a
write and maybe non-fatal read error might occur here.
Just my thoughts, I don't know anything about that device :)
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> + ETH_ALEN, mac);
> +
> + if (ret)
> + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> + else
> + netif_dbg(dev, ifup, dev->net,
> + "MAC address read from ASIX chip");
Maybe also use `netif_dbg` here?... There should be a reason why it was
used here in the first place. Or should not :)
Anyway, if someone will say that bailing out from while loop on any
error is OK feel free to add
Reviewed-by: Pavel Skripkin <paskripkin@gmail.com>
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/asix_common.c#L78
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] net: ax88179: add proper error handling of usb read errors
@ 2022-05-14 13:32 David Kahurani
2022-05-14 16:51 ` Pavel Skripkin
2022-05-16 19:31 ` Jakub Kicinski
0 siblings, 2 replies; 23+ messages in thread
From: David Kahurani @ 2022-05-14 13:32 UTC (permalink / raw)
To: netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter,
David Kahurani
Reads that are lesser than the requested size lead to uninit-value bugs.
In this particular case a variable which was supposed to be initialized
after a read is left uninitialized after a partial read.
Qualify such reads as errors and handle them correctly and while at it
convert the reader functions to return zero on success for easier error
handling.
Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: David Kahurani <k.kahurani@gmail.com>
Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
---
drivers/net/usb/ax88179_178a.c | 281 ++++++++++++++++++++++++++-------
1 file changed, 227 insertions(+), 54 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e2fa56b92..ae6fa10ff 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -185,8 +185,9 @@ static const struct {
{7, 0xcc, 0x4c, 0x18, 8},
};
-static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data, int in_pm)
+static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data, int in_pm)
{
int ret;
int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
@@ -201,11 +202,15 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
value, index, data, size);
- if (unlikely(ret < 0))
+ if (unlikely(ret < size)) {
+ ret = ret < 0 ? ret : -ENODATA;
+
netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
index, ret);
+ return ret;
+ }
- return ret;
+ return 0;
}
static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -249,26 +254,33 @@ static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
}
}
-static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
- u16 index, u16 size, void *data)
+static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data)
{
int ret;
if (2 == size) {
u16 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+ if (ret)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+ if (ret)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 1);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
@@ -290,26 +302,32 @@ static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
return ret;
}
-static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data)
+static int __must_check ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+ u16 index, u16 size, void *data)
{
int ret;
if (2 == size) {
u16 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 0);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -354,8 +372,15 @@ static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
{
struct usbnet *dev = netdev_priv(netdev);
u16 res;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
return res;
}
@@ -427,19 +452,31 @@ static int ax88179_suspend(struct usb_interface *intf, pm_message_t message)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_suspend(intf, message);
/* Disable RX path */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read MEDIUM_STATUS_MODE: %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
/* Force bulk-in zero length */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d\n", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_BZ | AX_PHYPWR_RSTCTL_IPRL;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
@@ -462,6 +499,7 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
{
u16 tmp16;
u8 tmp8;
+ int ret;
int (*fnr)(struct usbnet *, u8, u16, u16, u16, void *);
int (*fnw)(struct usbnet *, u8, u16, u16, u16, const void *);
@@ -481,11 +519,19 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
/* Enable Auto Detach bit */
tmp8 = 0;
- fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT: %d", ret);
+ return ret;
+ }
tmp8 |= AX_CLK_SELECT_ULR;
fnw(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
- fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_AT;
fnw(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
@@ -497,6 +543,7 @@ static int ax88179_resume(struct usb_interface *intf)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_link_change(dev, 0, 0);
@@ -515,7 +562,14 @@ static int ax88179_resume(struct usb_interface *intf)
ax88179_auto_detach(dev, 1);
/* Enable clock */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT %d\n", ret);
+
+ return ret;
+ }
+
tmp8 |= AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
msleep(100);
@@ -601,7 +655,7 @@ ax88179_get_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
&eeprom_buff[i - first_word],
0);
- if (ret < 0) {
+ if (ret) {
kfree(eeprom_buff);
return -EIO;
}
@@ -645,7 +699,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if (eeprom->offset & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, first_word, 1, 2,
&eeprom_buff[0]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", first_word);
goto free;
}
@@ -654,7 +708,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if ((eeprom->offset + eeprom->len) & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, last_word, 1, 2,
&eeprom_buff[last_word - first_word]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", last_word);
goto free;
}
@@ -951,23 +1005,45 @@ static int
ax88179_set_features(struct net_device *net, netdev_features_t features)
{
u8 tmp;
+ int ret;
struct usbnet *dev = netdev_priv(net);
netdev_features_t changed = net->features ^ features;
if (changed & NETIF_F_IP_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_IPV6_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_RXCSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
@@ -980,19 +1056,36 @@ static int ax88179_change_mtu(struct net_device *net, int new_mtu)
{
struct usbnet *dev = netdev_priv(net);
u16 tmp16;
+ int ret;
net->mtu = new_mtu;
dev->hard_mtu = net->mtu + net->hard_header_len;
if (net->mtu > 1500) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 |= AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
@@ -1045,6 +1138,7 @@ static int ax88179_check_eeprom(struct usbnet *dev)
u8 i, buf, eeprom[20];
u16 csum, delay = HZ / 10;
unsigned long jtimeout;
+ int ret;
/* Read EEPROM content */
for (i = 0; i < 6; i++) {
@@ -1060,16 +1154,29 @@ static int ax88179_check_eeprom(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &buf);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &buf);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (buf & EEP_BUSY);
- __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 2, 2, &eeprom[i * 2], 0);
+ ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 2, 2, &eeprom[i * 2], 0);
+
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_DATA_LOW: %d\n",
+ ret);
+ return ret;
+ }
if ((i == 0) && (eeprom[0] == 0xFF))
return -EINVAL;
@@ -1149,12 +1256,20 @@ static int ax88179_convert_old_led(struct usbnet *dev, u16 *ledvalue)
static int ax88179_led_setting(struct usbnet *dev)
{
+ int ret;
u8 ledfd, value = 0;
u16 tmp, ledact, ledlink, ledvalue = 0, delay = HZ / 10;
unsigned long jtimeout;
/* Check AX88179 version. UA1 or UA2*/
- ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1,
+ &value);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read GENERAL_STATUS: %d\n",
+ ret);
+ return ret;
+ }
if (!(value & AX_SECLD)) { /* UA1 */
value = AX_GPIO_CTRL_GPIO3EN | AX_GPIO_CTRL_GPIO2EN |
@@ -1178,20 +1293,39 @@ static int ax88179_led_setting(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &value);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (value & EEP_BUSY);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
+ 1, 1, &value);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_HIGH: %d\n",
+ ret);
+ return ret;
+ }
+
ledvalue = (value << 8);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 1, 1, &value);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_LOW: %d",
+ ret);
+ return ret;
+ }
+
ledvalue |= value;
/* load internal ROM for defaule setting */
@@ -1213,11 +1347,21 @@ static int ax88179_led_setting(struct usbnet *dev)
ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
GMII_PHYPAGE, 2, &tmp);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_LED_ACT, 2, &ledact);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_LED_ACT, 2, &ledact);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_LED_LINK, 2, &ledlink);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_LED_LINK, 2, &ledlink);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
ledact &= GMII_LED_ACTIVE_MASK;
ledlink &= GMII_LED_LINK_MASK;
@@ -1295,6 +1439,7 @@ static int ax88179_led_setting(struct usbnet *dev)
static void ax88179_get_mac_addr(struct usbnet *dev)
{
u8 mac[ETH_ALEN];
+ int ret;
memset(mac, 0, sizeof(mac));
@@ -1303,10 +1448,14 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
netif_dbg(dev, ifup, dev->net,
"MAC address read from device tree");
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
- ETH_ALEN, mac);
- netif_dbg(dev, ifup, dev->net,
- "MAC address read from ASIX chip");
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
+ ETH_ALEN, mac);
+
+ if (ret)
+ netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
+ else
+ netif_dbg(dev, ifup, dev->net,
+ "MAC address read from ASIX chip");
}
if (is_valid_ether_addr(mac)) {
@@ -1572,6 +1721,7 @@ static int ax88179_link_reset(struct usbnet *dev)
u16 mode, tmp16, delay = HZ / 10;
u32 tmp32 = 0x40000000;
unsigned long jtimeout;
+ int ret;
jtimeout = jiffies + delay;
while (tmp32 & 0x40000000) {
@@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
&ax179_data->rxctl);
/*link up, check the usb device control TX FIFO full or empty*/
- ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+ ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return 0;
@@ -1590,11 +1746,21 @@ static int ax88179_link_reset(struct usbnet *dev)
mode = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
AX_MEDIUM_RXFLOW_CTRLEN;
- ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
- 1, 1, &link_sts);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
+ 1, 1, &link_sts);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read LINK_STATUS: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_PHY_PHYSR, 2, &tmp16);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_PHY_PHYSR, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
return 0;
@@ -1732,9 +1898,16 @@ static int ax88179_reset(struct usbnet *dev)
static int ax88179_stop(struct usbnet *dev)
{
u16 tmp16;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read STATUS_MODE: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 7:48 David Kahurani
2022-04-16 11:05 ` Pavel Skripkin
2022-04-16 11:10 ` Pavel Skripkin
@ 2022-04-19 13:41 ` Paolo Abeni
2 siblings, 0 replies; 23+ messages in thread
From: Paolo Abeni @ 2022-04-19 13:41 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
On Sat, 2022-04-16 at 10:48 +0300, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
In the next version, please additionally fix the above tag: it must use
a single line, even if that means exceeding the 72 chars length limit.
Thanks!
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 11:53 ` Pavel Skripkin
@ 2022-04-16 11:57 ` Pavel Skripkin
0 siblings, 0 replies; 23+ messages in thread
From: Pavel Skripkin @ 2022-04-16 11:57 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Dan Carpenter
[-- Attachment #1.1: Type: text/plain, Size: 344 bytes --]
On 4/16/22 14:53, Pavel Skripkin wrote:
> No, this will break the driver. This function should set mac address in
> netdev structure and if read from device fails code calls
>
> eth_hw_addr_set(dev->net, mac);
>
Woops, I mean eth_hw_addr_random(dev->net) of course
I am sorry for confusion
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 11:49 ` David Kahurani
@ 2022-04-16 11:53 ` Pavel Skripkin
2022-04-16 11:57 ` Pavel Skripkin
0 siblings, 1 reply; 23+ messages in thread
From: Pavel Skripkin @ 2022-04-16 11:53 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Dan Carpenter
[-- Attachment #1.1: Type: text/plain, Size: 1500 bytes --]
Hi David,
On 4/16/22 14:49, David Kahurani wrote:
>> > @@ -1303,8 +1448,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
>> > netif_dbg(dev, ifup, dev->net,
>> > "MAC address read from device tree");
>> > } else {
>> > - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
>> > - ETH_ALEN, mac);
>> > + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
>> > + ETH_ALEN, mac);
>> > +
>> > + if (ret)
>> > + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
>> > +
>> > netif_dbg(dev, ifup, dev->net,
>> > "MAC address read from ASIX chip");
>> > }
>>
>>
>> This message sequence is confusing.
>>
>> In case of ax88179_read_cmd() failure mac read from device actually
>> failed, but message says, that it was successfully finished.
>
> I suppose the code should return in case of an error that way the next
> message does not get executed.
>
No, this will break the driver. This function should set mac address in
netdev structure and if read from device fails code calls
eth_hw_addr_set(dev->net, mac);
which will generate random mac addr. I.e. read failure is not critical
in that function
So, no need to return with an error, just don't print confusing messages :)
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 11:10 ` Pavel Skripkin
@ 2022-04-16 11:49 ` David Kahurani
2022-04-16 11:53 ` Pavel Skripkin
0 siblings, 1 reply; 23+ messages in thread
From: David Kahurani @ 2022-04-16 11:49 UTC (permalink / raw)
To: Pavel Skripkin
Cc: netdev, syzbot, davem, jgg, kuba, linux-kernel, linux-usb,
Phillip Potter, syzkaller-bugs, arnd, Dan Carpenter
On Sat, Apr 16, 2022 at 2:10 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Hi David,
Hi Pavel.
>
> one more small comment
>
> On 4/16/22 10:48, David Kahurani wrote:
> > Reads that are lesser than the requested size lead to uninit-value bugs.
> > In this particular case a variable which was supposed to be initialized
> > after a read is left uninitialized after a partial read.
> >
> > Qualify such reads as errors and handle them correctly and while at it
> > convert the reader functions to return zero on success for easier error
> > handling.
> >
> > Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> > gigabit ethernet adapter driver")
> > Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> > Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> > ---
>
> [code snip]
>
> > @@ -1295,6 +1439,7 @@ static int ax88179_led_setting(struct usbnet *dev)
> > static void ax88179_get_mac_addr(struct usbnet *dev)
> > {
> > u8 mac[ETH_ALEN];
> > + int ret;
> >
> > memset(mac, 0, sizeof(mac));
> >
> > @@ -1303,8 +1448,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
> > netif_dbg(dev, ifup, dev->net,
> > "MAC address read from device tree");
> > } else {
> > - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> > - ETH_ALEN, mac);
> > + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> > + ETH_ALEN, mac);
> > +
> > + if (ret)
> > + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> > +
> > netif_dbg(dev, ifup, dev->net,
> > "MAC address read from ASIX chip");
> > }
>
>
> This message sequence is confusing.
>
> In case of ax88179_read_cmd() failure mac read from device actually
> failed, but message says, that it was successfully finished.
I suppose the code should return in case of an error that way the next
message does not get executed.
Thanks for the review! Will fix it and the other issue in the next version.
>
>
>
>
>
> With regards,
> Pavel Skripkin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 7:48 David Kahurani
2022-04-16 11:05 ` Pavel Skripkin
@ 2022-04-16 11:10 ` Pavel Skripkin
2022-04-16 11:49 ` David Kahurani
2022-04-19 13:41 ` Paolo Abeni
2 siblings, 1 reply; 23+ messages in thread
From: Pavel Skripkin @ 2022-04-16 11:10 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
[-- Attachment #1.1: Type: text/plain, Size: 1726 bytes --]
Hi David,
one more small comment
On 4/16/22 10:48, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> ---
[code snip]
> @@ -1295,6 +1439,7 @@ static int ax88179_led_setting(struct usbnet *dev)
> static void ax88179_get_mac_addr(struct usbnet *dev)
> {
> u8 mac[ETH_ALEN];
> + int ret;
>
> memset(mac, 0, sizeof(mac));
>
> @@ -1303,8 +1448,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
> netif_dbg(dev, ifup, dev->net,
> "MAC address read from device tree");
> } else {
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> - ETH_ALEN, mac);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
> + ETH_ALEN, mac);
> +
> + if (ret)
> + netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
> +
> netif_dbg(dev, ifup, dev->net,
> "MAC address read from ASIX chip");
> }
This message sequence is confusing.
In case of ax88179_read_cmd() failure mac read from device actually
failed, but message says, that it was successfully finished.
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
2022-04-16 7:48 David Kahurani
@ 2022-04-16 11:05 ` Pavel Skripkin
2022-04-16 11:10 ` Pavel Skripkin
2022-04-19 13:41 ` Paolo Abeni
2 siblings, 0 replies; 23+ messages in thread
From: Pavel Skripkin @ 2022-04-16 11:05 UTC (permalink / raw)
To: David Kahurani, netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
[-- Attachment #1.1: Type: text/plain, Size: 1242 bytes --]
Hi David,
Almost perfect, please see one comment inline
On 4/16/22 10:48, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs.
> In this particular case a variable which was supposed to be initialized
> after a read is left uninitialized after a partial read.
>
> Qualify such reads as errors and handle them correctly and while at it
> convert the reader functions to return zero on success for easier error
> handling.
>
> Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
> gigabit ethernet adapter driver")
> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> ---
[code snip]
>
> @@ -416,7 +441,7 @@ ax88179_phy_write_mmd_indirect(struct usbnet *dev, u16 prtad, u16 devad,
> ret = ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
> MII_MMD_DATA, 2, &data);
>
> - if (ret < 0)
> + if (ret)
> return ret;
>
I think, you didn't mean to do so. ax88179_write_cmd() returns number of
bytes written, so you are changing the logic here. And write call is not
related to uninit value bugs
With regards,
Pavel Skripkin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] net: ax88179: add proper error handling of usb read errors
@ 2022-04-16 7:48 David Kahurani
2022-04-16 11:05 ` Pavel Skripkin
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: David Kahurani @ 2022-04-16 7:48 UTC (permalink / raw)
To: netdev
Cc: syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, kuba, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter,
David Kahurani
Reads that are lesser than the requested size lead to uninit-value bugs.
In this particular case a variable which was supposed to be initialized
after a read is left uninitialized after a partial read.
Qualify such reads as errors and handle them correctly and while at it
convert the reader functions to return zero on success for easier error
handling.
Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to
gigabit ethernet adapter driver")
Signed-off-by: David Kahurani <k.kahurani@gmail.com>
Reported-and-tested-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
---
drivers/net/usb/ax88179_178a.c | 279 ++++++++++++++++++++++++++-------
1 file changed, 226 insertions(+), 53 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e2fa56b92..5113ed63f 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -185,8 +185,9 @@ static const struct {
{7, 0xcc, 0x4c, 0x18, 8},
};
-static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data, int in_pm)
+static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data, int in_pm)
{
int ret;
int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
@@ -201,11 +202,15 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
value, index, data, size);
- if (unlikely(ret < 0))
+ if (unlikely(ret < size)) {
+ ret = ret < 0 ? ret : -ENODATA;
+
netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
index, ret);
+ return ret;
+ }
- return ret;
+ return 0;
}
static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -249,26 +254,33 @@ static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
}
}
-static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
- u16 index, u16 size, void *data)
+static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
+ u16 value, u16 index, u16 size,
+ void *data)
{
int ret;
if (2 == size) {
u16 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+ if (ret)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
+ if (ret)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 1);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
@@ -290,26 +302,32 @@ static int ax88179_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
return ret;
}
-static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
- u16 size, void *data)
+static int __must_check ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+ u16 index, u16 size, void *data)
{
int ret;
if (2 == size) {
u16 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret)
+ return ret;
le16_to_cpus(&buf);
*((u16 *)data) = buf;
} else if (4 == size) {
u32 buf = 0;
ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 0);
+ if (ret)
+ return ret;
le32_to_cpus(&buf);
*((u32 *)data) = buf;
} else {
ret = __ax88179_read_cmd(dev, cmd, value, index, size, data, 0);
+ if (ret)
+ return ret;
}
- return ret;
+ return 0;
}
static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
@@ -354,8 +372,15 @@ static int ax88179_mdio_read(struct net_device *netdev, int phy_id, int loc)
{
struct usbnet *dev = netdev_priv(netdev);
u16 res;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)loc, 2, &res);
return res;
}
@@ -416,7 +441,7 @@ ax88179_phy_write_mmd_indirect(struct usbnet *dev, u16 prtad, u16 devad,
ret = ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
MII_MMD_DATA, 2, &data);
- if (ret < 0)
+ if (ret)
return ret;
return 0;
@@ -427,19 +452,31 @@ static int ax88179_suspend(struct usb_interface *intf, pm_message_t message)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_suspend(intf, message);
/* Disable RX path */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read MEDIUM_STATUS_MODE: %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
/* Force bulk-in zero length */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d\n", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_BZ | AX_PHYPWR_RSTCTL_IPRL;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL,
@@ -462,6 +499,7 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
{
u16 tmp16;
u8 tmp8;
+ int ret;
int (*fnr)(struct usbnet *, u8, u16, u16, u16, void *);
int (*fnw)(struct usbnet *, u8, u16, u16, u16, const void *);
@@ -481,11 +519,19 @@ static int ax88179_auto_detach(struct usbnet *dev, int in_pm)
/* Enable Auto Detach bit */
tmp8 = 0;
- fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT: %d", ret);
+ return ret;
+ }
tmp8 |= AX_CLK_SELECT_ULR;
fnw(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
- fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ ret = fnr(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHYPWR_RSTCTL: %d", ret);
+ return ret;
+ }
tmp16 |= AX_PHYPWR_RSTCTL_AT;
fnw(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16);
@@ -497,6 +543,7 @@ static int ax88179_resume(struct usb_interface *intf)
struct usbnet *dev = usb_get_intfdata(intf);
u16 tmp16;
u8 tmp8;
+ int ret;
usbnet_link_change(dev, 0, 0);
@@ -515,7 +562,14 @@ static int ax88179_resume(struct usb_interface *intf)
ax88179_auto_detach(dev, 1);
/* Enable clock */
- ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+ ret = ax88179_read_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read CLK_SELECT %d\n", ret);
+
+ return ret;
+ }
+
tmp8 |= AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
ax88179_write_cmd_nopm(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp8);
msleep(100);
@@ -601,7 +655,7 @@ ax88179_get_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
&eeprom_buff[i - first_word],
0);
- if (ret < 0) {
+ if (ret) {
kfree(eeprom_buff);
return -EIO;
}
@@ -645,7 +699,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if (eeprom->offset & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, first_word, 1, 2,
&eeprom_buff[0]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", first_word);
goto free;
}
@@ -654,7 +708,7 @@ ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom,
if ((eeprom->offset + eeprom->len) & 1) {
ret = ax88179_read_cmd(dev, AX_ACCESS_EEPROM, last_word, 1, 2,
&eeprom_buff[last_word - first_word]);
- if (ret < 0) {
+ if (ret) {
netdev_err(net, "Failed to read EEPROM at offset 0x%02x.\n", last_word);
goto free;
}
@@ -951,23 +1005,45 @@ static int
ax88179_set_features(struct net_device *net, netdev_features_t features)
{
u8 tmp;
+ int ret;
struct usbnet *dev = netdev_priv(net);
netdev_features_t changed = net->features ^ features;
if (changed & NETIF_F_IP_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_IPV6_CSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, &tmp);
}
if (changed & NETIF_F_RXCSUM) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL,
+ 1, 1, &tmp);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TXCOE_CTL: %d\n",
+ ret);
+ return ret;
+ }
+
tmp ^= AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
@@ -980,19 +1056,36 @@ static int ax88179_change_mtu(struct net_device *net, int new_mtu)
{
struct usbnet *dev = netdev_priv(net);
u16 tmp16;
+ int ret;
net->mtu = new_mtu;
dev->hard_mtu = net->mtu + net->hard_header_len;
if (net->mtu > 1500) {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 |= AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
+ AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read MEDIUM_STATUS_MODE %d\n",
+ ret);
+ return ret;
+ }
+
tmp16 &= ~AX_MEDIUM_JUMBO_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
@@ -1045,6 +1138,7 @@ static int ax88179_check_eeprom(struct usbnet *dev)
u8 i, buf, eeprom[20];
u16 csum, delay = HZ / 10;
unsigned long jtimeout;
+ int ret;
/* Read EEPROM content */
for (i = 0; i < 6; i++) {
@@ -1060,16 +1154,29 @@ static int ax88179_check_eeprom(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &buf);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &buf);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (buf & EEP_BUSY);
- __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 2, 2, &eeprom[i * 2], 0);
+ ret = __ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 2, 2, &eeprom[i * 2], 0);
+
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_DATA_LOW: %d\n",
+ ret);
+ return ret;
+ }
if ((i == 0) && (eeprom[0] == 0xFF))
return -EINVAL;
@@ -1149,12 +1256,20 @@ static int ax88179_convert_old_led(struct usbnet *dev, u16 *ledvalue)
static int ax88179_led_setting(struct usbnet *dev)
{
+ int ret;
u8 ledfd, value = 0;
u16 tmp, ledact, ledlink, ledvalue = 0, delay = HZ / 10;
unsigned long jtimeout;
/* Check AX88179 version. UA1 or UA2*/
- ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, GENERAL_STATUS, 1, 1,
+ &value);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read GENERAL_STATUS: %d\n",
+ ret);
+ return ret;
+ }
if (!(value & AX_SECLD)) { /* UA1 */
value = AX_GPIO_CTRL_GPIO3EN | AX_GPIO_CTRL_GPIO2EN |
@@ -1178,20 +1293,39 @@ static int ax88179_led_setting(struct usbnet *dev)
jtimeout = jiffies + delay;
do {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
+ 1, 1, &value);
+ if (ret) {
+ netdev_dbg(dev->net,
+ "Failed to read SROM_CMD: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return -EINVAL;
} while (value & EEP_BUSY);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_HIGH,
+ 1, 1, &value);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_HIGH: %d\n",
+ ret);
+ return ret;
+ }
+
ledvalue = (value << 8);
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
- 1, 1, &value);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_DATA_LOW,
+ 1, 1, &value);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read SROM_DATA_LOW: %d",
+ ret);
+ return ret;
+ }
+
ledvalue |= value;
/* load internal ROM for defaule setting */
@@ -1213,11 +1347,21 @@ static int ax88179_led_setting(struct usbnet *dev)
ax88179_write_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
GMII_PHYPAGE, 2, &tmp);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_LED_ACT, 2, &ledact);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_LED_ACT, 2, &ledact);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_LED_LINK, 2, &ledlink);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_LED_LINK, 2, &ledlink);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d", ret);
+ return ret;
+ }
ledact &= GMII_LED_ACTIVE_MASK;
ledlink &= GMII_LED_LINK_MASK;
@@ -1295,6 +1439,7 @@ static int ax88179_led_setting(struct usbnet *dev)
static void ax88179_get_mac_addr(struct usbnet *dev)
{
u8 mac[ETH_ALEN];
+ int ret;
memset(mac, 0, sizeof(mac));
@@ -1303,8 +1448,12 @@ static void ax88179_get_mac_addr(struct usbnet *dev)
netif_dbg(dev, ifup, dev->net,
"MAC address read from device tree");
} else {
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
- ETH_ALEN, mac);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN,
+ ETH_ALEN, mac);
+
+ if (ret)
+ netdev_dbg(dev->net, "Failed to read NODE_ID: %d", ret);
+
netif_dbg(dev, ifup, dev->net,
"MAC address read from ASIX chip");
}
@@ -1572,6 +1721,7 @@ static int ax88179_link_reset(struct usbnet *dev)
u16 mode, tmp16, delay = HZ / 10;
u32 tmp32 = 0x40000000;
unsigned long jtimeout;
+ int ret;
jtimeout = jiffies + delay;
while (tmp32 & 0x40000000) {
@@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
&ax179_data->rxctl);
/*link up, check the usb device control TX FIFO full or empty*/
- ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+ ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
+ ret);
+ return ret;
+ }
if (time_after(jiffies, jtimeout))
return 0;
@@ -1590,11 +1746,21 @@ static int ax88179_link_reset(struct usbnet *dev)
mode = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
AX_MEDIUM_RXFLOW_CTRLEN;
- ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
- 1, 1, &link_sts);
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, PHYSICAL_LINK_STATUS,
+ 1, 1, &link_sts);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read LINK_STATUS: %d", ret);
+ return ret;
+ }
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
+ GMII_PHY_PHYSR, 2, &tmp16);
- ax88179_read_cmd(dev, AX_ACCESS_PHY, AX88179_PHY_ID,
- GMII_PHY_PHYSR, 2, &tmp16);
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read PHY_ID: %d\n", ret);
+ return ret;
+ }
if (!(tmp16 & GMII_PHY_PHYSR_LINK)) {
return 0;
@@ -1732,9 +1898,16 @@ static int ax88179_reset(struct usbnet *dev)
static int ax88179_stop(struct usbnet *dev)
{
u16 tmp16;
+ int ret;
+
+ ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
+ 2, 2, &tmp16);
+
+ if (ret) {
+ netdev_dbg(dev->net, "Failed to read STATUS_MODE: %d\n", ret);
+ return ret;
+ }
- ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
- 2, 2, &tmp16);
tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
2, 2, &tmp16);
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-05-16 19:31 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04 15:10 [PATCH] net: ax88179: add proper error handling of usb read errors David Kahurani
2022-04-04 15:31 ` Dan Carpenter
2022-04-13 12:36 ` David Kahurani
2022-04-13 15:32 ` Dan Carpenter
2022-04-14 7:31 ` Oliver Neukum
2022-04-14 8:21 ` Dan Carpenter
2022-04-14 9:13 ` Oliver Neukum
2022-04-04 16:50 ` Pavel Skripkin
2022-04-11 15:11 ` Andy Shevchenko
2022-04-05 9:44 ` Paolo Abeni
2022-04-05 20:44 ` kernel test robot
2022-04-16 7:48 David Kahurani
2022-04-16 11:05 ` Pavel Skripkin
2022-04-16 11:10 ` Pavel Skripkin
2022-04-16 11:49 ` David Kahurani
2022-04-16 11:53 ` Pavel Skripkin
2022-04-16 11:57 ` Pavel Skripkin
2022-04-19 13:41 ` Paolo Abeni
2022-05-14 13:32 David Kahurani
2022-05-14 16:51 ` Pavel Skripkin
2022-05-14 18:54 ` Dan Carpenter
2022-05-14 18:57 ` Pavel Skripkin
2022-05-16 19:31 ` Jakub Kicinski
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).