netdev - Re: A bug in team driver

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: <1475636928.28155.196.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Wed, 05 Oct 2016 12:08:48 +0900
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Alex Sidorenko <alexandre.sidorenko@....com>
Cc:     netdev@...r.kernel.org
Subject: Re: A bug in team driver

On Tue, 2016-10-04 at 18:30 -0400, Alex Sidorenko wrote:
> The problem was found on RHEL7.2 but is still present in the latest
> upstream kernel (according to visual sources inspection).
> 
> While using roundrobin runner we have noticed that after sending on
> team0 about 2.1 billion packets we started seeing 50% packet drop on
> team0 
> (according to 'netstat -i'). This number suggested 'signed int'
> overflow and indeed, inspecting the sources I have noticed the
> following in 
> 
> drivers/net/team/team_mode_roundrobin.c
> ---------------------------------------
> struct rr_priv {
> 	unsigned int sent_packets;                        <--------- unsigned
> int
> };
> 
> static struct rr_priv *rr_priv(struct team *team)
> {
> 	return (struct rr_priv *) &team->mode_priv;
> }
> 
> static bool rr_transmit(struct team *team, struct sk_buff *skb)
> {
> 	struct team_port *port;
> 	int port_index;
> 
> 	port_index = team_num_to_port_index(team,
> 					    rr_priv(team)->sent_packets++);
> ---
> 
> 
> we have 'unsigned int sent_packets' but we call team_num_to_port_index
> where 'num' is 'int'
> 
> include/linux/if_team.h
> -----------------------
> static inline int team_num_to_port_index(struct team *team, int num)
> <-- signed int
> {
> 	int en_port_count = ACCESS_ONCE(team->en_port_count);
> 
> 	if (unlikely(!en_port_count))
> 		return 0;
> 	return num % en_port_count;
> }
> 
> 
> As soon as sent_packets becomes larger than MAXINT (=2**31-1),
> team_num_to_port_index() can return negative number as num becomes
> negative and remainder 
> (num % en_port_count) is either 0 or negative. This leads to looking
> up incorrect hash-bucket and dropping packets.
> 
> We have easily duplicated this in roundrobin mode with two ports.
> After reaching 2**31 packets sent on team0 every second packet was
> dropped.
> 
> Rebuilding the kernel after changing 
> 
> team_num_to_port_index(struct team *team, int num) ->
> team_num_to_port_index(struct team *team, unsigned int num)
> 
> and running the test again does not show packet drop anymore.
> 
> The same subroutine is used in
> team_mode_loadbalance.c:lb_hash_select_tx_port but we pass 'unsigned
> char hash' to team_num_to_port_index(), so there should be no
> overflow. I did not test that mode in my tests.

Good catch ! Can you send an official patch to fix this ?

Thanks.

>