* [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
@ 2020-09-29 8:20 Anant Thazhemadam
2020-09-29 8:46 ` Anant Thazhemadam
2020-09-29 8:47 ` Petko Manolov
0 siblings, 2 replies; 4+ messages in thread
From: Anant Thazhemadam @ 2020-09-29 8:20 UTC (permalink / raw)
Cc: Anant Thazhemadam, Petko Manolov, syzbot+abbc768b560c84d92fd3,
netdev, linux-usb, linux-kernel, Jakub Kicinski,
linux-kernel-mentees, David S. Miller
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)) {
+ 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);
--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
2020-09-29 8:20 [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address Anant Thazhemadam
@ 2020-09-29 8:46 ` Anant Thazhemadam
2020-09-29 8:47 ` Petko Manolov
1 sibling, 0 replies; 4+ messages in thread
From: Anant Thazhemadam @ 2020-09-29 8:46 UTC (permalink / raw)
Cc: Petko Manolov, syzbot+abbc768b560c84d92fd3, netdev, linux-usb,
linux-kernel, Jakub Kicinski, linux-kernel-mentees,
David S. Miller
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
2020-09-29 8:20 [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address Anant Thazhemadam
2020-09-29 8:46 ` Anant Thazhemadam
@ 2020-09-29 8:47 ` Petko Manolov
2020-09-30 4:02 ` Anant Thazhemadam
1 sibling, 1 reply; 4+ messages in thread
From: Petko Manolov @ 2020-09-29 8:47 UTC (permalink / raw)
To: Anant Thazhemadam
Cc: Petko Manolov, syzbot+abbc768b560c84d92fd3, netdev, linux-usb,
linux-kernel, Jakub Kicinski, linux-kernel-mentees,
David S. Miller
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
2020-09-29 8:47 ` Petko Manolov
@ 2020-09-30 4:02 ` Anant Thazhemadam
0 siblings, 0 replies; 4+ messages in thread
From: Anant Thazhemadam @ 2020-09-30 4:02 UTC (permalink / raw)
To: linux-kernel-mentees, syzbot+abbc768b560c84d92fd3, Petko Manolov,
David S. Miller, Jakub Kicinski, linux-usb, netdev, linux-kernel
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-30 4:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 8:20 [Linux-kernel-mentees] [PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address Anant Thazhemadam
2020-09-29 8:46 ` Anant Thazhemadam
2020-09-29 8:47 ` Petko Manolov
2020-09-30 4:02 ` Anant Thazhemadam
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).