netdev - Re: [Patch net 3/3] team: use a larger struct for mac address

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7103fb6b-2483-37c0-48e5-19aa9fd4f386@redhat.com>
Date:   Wed, 26 Apr 2017 11:55:42 -0400
From:   Jarod Wilson <jarod@...hat.com>
To:     Jiri Pirko <jiri@...nulli.us>, Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, andreyknvl@...gle.com
Subject: Re: [Patch net 3/3] team: use a larger struct for mac address

On 2017-04-26 1:40 AM, Jiri Pirko wrote:
> Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@...il.com wrote:
>> IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
>> but in many places especially bonding, we use struct sockaddr
>> to copy and set mac addr, this could lead to stack out-of-bounds
>> access.
>>
>> Fix it by using a larger address storage.
>>
>> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
>> Cc: Jiri Pirko <jiri@...nulli.us>
>> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>> ---
>> drivers/net/team/team.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> index 85c0124..88878f1 100644
>> --- a/drivers/net/team/team.c
>> +++ b/drivers/net/team/team.c
>> @@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev)
>> static int __set_port_dev_addr(struct net_device *port_dev,
>> 			       const unsigned char *dev_addr)
>> {
>> -	struct sockaddr addr;
>> +	struct {
>> +		unsigned short type;
>> +		unsigned char addr[MAX_ADDR_LEN];
>> +	} addr;
> 
> Wouldn't it make sense to define this struct somewhere in the core
> headers?

We already have struct sockaddr_storage that could be used throughout 
this set as well. We just converted a few pieces of the bonding driver 
over to using it for better support of ipoib bonds, via commit 
faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use 
that in both bonding and team, rather than having different per-driver 
structs, or Yet Another Address Storage implementation.

-- 
Jarod Wilson
jarod@...hat.com