| Message ID | 20200929114039.26866-1-petko.manolov@konsulko.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: pegasus: Proper error handing when setting pegasus' MAC address | expand |
From: Petko Manolov <petko.manolov@konsulko.com> Date: Tue, 29 Sep 2020 14:40:39 +0300 > -static void set_ethernet_addr(pegasus_t *pegasus) > +static int set_ethernet_addr(pegasus_t *pegasus) > { You change this to return an 'int' but no callers were updated to check it. Furthermore, failure to probe a MAC address can be resolved by choosing a random MAC address. This handling is preferrable because it allows the interface to still come up successfully.
On 20-10-01 18:42:18, David Miller wrote: > From: Petko Manolov <petko.manolov@konsulko.com> > Date: Tue, 29 Sep 2020 14:40:39 +0300 > > > -static void set_ethernet_addr(pegasus_t *pegasus) > > +static int set_ethernet_addr(pegasus_t *pegasus) > > { > > You change this to return an 'int' but no callers were updated to check it. > > Furthermore, failure to probe a MAC address can be resolved by choosing a > random MAC address. This handling is preferrable because it allows the > interface to still come up successfully. Thanks for looking into this. V2 already sent.
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c index e92cb51a2c77..25855f976c1b 100644 --- a/drivers/net/usb/pegasus.c +++ b/drivers/net/usb/pegasus.c @@ -360,28 +360,45 @@ static int write_eprom_word(pegasus_t *pegasus, __u8 index, __u16 data) } #endif /* PEGASUS_WRITE_EEPROM */ -static inline void get_node_id(pegasus_t *pegasus, __u8 *id) +static inline int get_node_id(pegasus_t *pegasus, u8 *id) { - int i; - __u16 w16; + int i, ret; + u16 w16; for (i = 0; i < 3; i++) { - read_eprom_word(pegasus, i, &w16); + ret = read_eprom_word(pegasus, i, &w16); + if (ret < 0) + return ret; ((__le16 *) id)[i] = cpu_to_le16(w16); } + + return 0; } -static void set_ethernet_addr(pegasus_t *pegasus) +static int set_ethernet_addr(pegasus_t *pegasus) { - __u8 node_id[6]; + int ret; + u8 node_id[6]; if (pegasus->features & PEGASUS_II) { - get_registers(pegasus, 0x10, sizeof(node_id), node_id); + ret = get_registers(pegasus, 0x10, sizeof(node_id), node_id); + if (ret < 0) + goto err; } else { - get_node_id(pegasus, node_id); - set_registers(pegasus, EthID, sizeof(node_id), node_id); + ret = get_node_id(pegasus, node_id); + if (ret < 0) + goto err; + ret = set_registers(pegasus, EthID, sizeof(node_id), node_id); + if (ret < 0) + goto err; } + memcpy(pegasus->net->dev_addr, node_id, sizeof(node_id)); + + return 0; +err: + dev_err(&pegasus->intf->dev, "device's MAC address not set.\n"); + return ret; } static inline int reset_mac(pegasus_t *pegasus)
Fix a bug in set_ethernet_addr() which does not take into account possible errors (or partial reads) returned by its helpers. This can potentially lead to writing random data into device's MAC address registers. Signed-off-by: Petko Manolov <petko.manolov@konsulko.com> --- drivers/net/usb/pegasus.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)