| Message ID | 20200929082028.50540-1-anant.thazhemadam@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Linux-kernel-mentees] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address | expand |
A sample crash report can be found here.
https://syzkaller.appspot.com/text?tag=CrashReport&x=17486911900000
The line where the bug seems to get triggered is,
if (!batadv_compare_eth(hard_iface->net_dev->dev_addr,
net_dev->dev_addr))
Looks like it goes through the list of ethernet interfaces, and
compares it with the address of the new device; which can
end up going uninitialized too.
The address should have been set by set_ethernet_addr:
static inline void set_ethernet_addr(rtl8150_t * dev)
{
u8 node_id[6];
get_registers(dev, IDR, sizeof(node_id), node_id);
memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
}
However, when get_registers() fails (when ret <= 0 or ret > size),
no memory is copied back into node_id, which remains uninitialized.
The address is then set to be this uninitialized node_id value.
Checking for the return value of get_registers() in set_ethernet_addr()
and further checking the value of set_ethernet_addr() where ever it has
been invoked, and handling the condition wherein get_registers() fails
appropriately helps solve this issue.
Thank you for your time.
Thanks,
Anant
On 20-09-29 13:50:28, Anant Thazhemadam wrote: > When get_registers() fails (which happens when usb_control_msg() fails) > in set_ethernet_addr(), the uninitialized value of node_id gets copied > as the address. > > Checking for the return values appropriately, and handling the case > wherein set_ethernet_addr() fails like this, helps in avoiding the > mac address being incorrectly set in this manner. > > Reported-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com > Tested-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> > --- > drivers/net/usb/rtl8150.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c > index 733f120c852b..e542a9ab2ff8 100644 > --- a/drivers/net/usb/rtl8150.c > +++ b/drivers/net/usb/rtl8150.c > @@ -150,7 +150,7 @@ static const char driver_name [] = "rtl8150"; > ** device related part of the code > ** > */ > -static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) > +static int get_registers(rtl8150_t *dev, u16 indx, u16 size, void *data) > { > void *buf; > int ret; > @@ -274,12 +274,17 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 indx, u16 reg) > return 1; > } > > -static inline void set_ethernet_addr(rtl8150_t * dev) > +static bool set_ethernet_addr(rtl8150_t *dev) > { > u8 node_id[6]; > + int ret; > > - get_registers(dev, IDR, sizeof(node_id), node_id); > - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id)); > + ret = get_registers(dev, IDR, sizeof(node_id), node_id); > + if (ret > 0 && ret <= sizeof(node_id)) { get_registers() was recently modified to use usb_control_msg_recv() which does not return partial reads. IOW you'll either get negative value or sizeof(node_id). Since it is good to be paranoid i'd convert the above check to: if (ret == sizeof(node_id)) { and fail in any other case. Apart from this minor detail the rest of the patch looks good to me. Acked-by: Petko Manolov
On 29/09/20 2:17 pm, Petko Manolov wrote: > On 20-09-29 13:50:28, Anant Thazhemadam wrote: >> When get_registers() fails (which happens when usb_control_msg() fails) >> in set_ethernet_addr(), the uninitialized value of node_id gets copied >> as the address. >> >> Checking for the return values appropriately, and handling the case >> wherein set_ethernet_addr() fails like this, helps in avoiding the >> mac address being incorrectly set in this manner. >> >> Reported-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com >> Tested-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com >> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> >> --- >> drivers/net/usb/rtl8150.c | 24 ++++++++++++++++-------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c >> index 733f120c852b..e542a9ab2ff8 100644 >> --- a/drivers/net/usb/rtl8150.c >> +++ b/drivers/net/usb/rtl8150.c >> @@ -150,7 +150,7 @@ static const char driver_name [] = "rtl8150"; >> ** device related part of the code >> ** >> */ >> -static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) >> +static int get_registers(rtl8150_t *dev, u16 indx, u16 size, void *data) >> { >> void *buf; >> int ret; >> @@ -274,12 +274,17 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 indx, u16 reg) >> return 1; >> } >> >> -static inline void set_ethernet_addr(rtl8150_t * dev) >> +static bool set_ethernet_addr(rtl8150_t *dev) >> { >> u8 node_id[6]; >> + int ret; >> >> - get_registers(dev, IDR, sizeof(node_id), node_id); >> - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id)); >> + ret = get_registers(dev, IDR, sizeof(node_id), node_id); >> + if (ret > 0 && ret <= sizeof(node_id)) { > get_registers() was recently modified to use usb_control_msg_recv() which does > not return partial reads. IOW you'll either get negative value or > sizeof(node_id). Since it is good to be paranoid i'd convert the above check > to: > > if (ret == sizeof(node_id)) { > > and fail in any other case. Apart from this minor detail the rest of the patch > looks good to me. > > Acked-by: Petko Manolov Got it. I'll be sure to include this in a v2, and send that in soon enough. Thanks for pointing that out. :) Thanks, Anant
diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c index 733f120c852b..e542a9ab2ff8 100644 --- a/drivers/net/usb/rtl8150.c +++ b/drivers/net/usb/rtl8150.c @@ -150,7 +150,7 @@ static const char driver_name [] = "rtl8150"; ** device related part of the code ** */ -static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data) +static int get_registers(rtl8150_t *dev, u16 indx, u16 size, void *data) { void *buf; int ret; @@ -274,12 +274,17 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 indx, u16 reg) return 1; } -static inline void set_ethernet_addr(rtl8150_t * dev) +static bool set_ethernet_addr(rtl8150_t *dev) { u8 node_id[6]; + int ret; - get_registers(dev, IDR, sizeof(node_id), node_id); - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id)); + ret = get_registers(dev, IDR, sizeof(node_id), node_id); + if (ret > 0 && ret <= sizeof(node_id)) { + memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id)); + return true; + } + return false; } static int rtl8150_set_mac_address(struct net_device *netdev, void *p) @@ -909,21 +914,24 @@ static int rtl8150_probe(struct usb_interface *intf, goto out1; } fill_skb_pool(dev); - set_ethernet_addr(dev); - + if (!set_ethernet_addr(dev)) { + dev_err(&intf->dev, "couldn't set the ethernet address for the device\n"); + goto out2; + } usb_set_intfdata(intf, dev); SET_NETDEV_DEV(netdev, &intf->dev); if (register_netdev(netdev) != 0) { dev_err(&intf->dev, "couldn't register the device\n"); - goto out2; + goto out3; } dev_info(&intf->dev, "%s: rtl8150 is detected\n", netdev->name); return 0; -out2: +out3: usb_set_intfdata(intf, NULL); +out2: free_skb_pool(dev); out1: free_all_urbs(dev);