Re: [Linux-kernel-mentees][PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
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+abbc768b560c84d92...@syzkaller.appspotmail.com
>> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam
>> ---
>> 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
Re: [Linux-kernel-mentees][PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
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+abbc768b560c84d92...@syzkaller.appspotmail.com
> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam
> ---
> 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
Re: [Linux-kernel-mentees][PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
A sample crash report can be found here.
https://syzkaller.appspot.com/text?tag=CrashReport=1748691190
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][PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
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+abbc768b560c84d92...@syzkaller.appspotmail.com
Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam
---
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(>dev, "couldn't set the ethernet address for the
device\n");
+ goto out2;
+ }
usb_set_intfdata(intf, dev);
SET_NETDEV_DEV(netdev, >dev);
if (register_netdev(netdev) != 0) {
dev_err(>dev, "couldn't register the device\n");
- goto out2;
+ goto out3;
}
dev_info(>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
