netdev - Re: [PATCH net-next v2] bridge: set is_local and is_static before fdb entry is added to the fdb hashtable

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]

Date:	Tue, 27 Oct 2015 07:02:55 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	nikolay@...ulusnetworks.com
Cc:	roopa@...ulusnetworks.com, stephen@...workplumber.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] bridge: set is_local and is_static before
 fdb entry is added to the fdb hashtable

From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Date: Tue, 27 Oct 2015 14:31:48 +0100

> On 10/27/2015 01:12 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@...ulusnetworks.com>
>> 
>> Problem Description:
>> We can add fdbs pointing to the bridge with NULL ->dst but that has a
>> few race conditions because br_fdb_insert() is used which first creates
>> the fdb and then, after the fdb has been published/linked, sets
>> "is_local" to 1 and in that time frame if a packet arrives for that fdb
>> it may see it as non-local and either do a NULL ptr dereference in
>> br_forward() or attach the fdb to the port where it arrived, and later
>> br_fdb_insert() will make it local thus getting a wrong fdb entry.
>> Call chain br_handle_frame_finish() -> br_forward():
>> But in br_handle_frame_finish() in order to call br_forward() the dst
>> should not be local i.e. skb != NULL, whenever the dst is
>> found to be local skb is set to NULL so we can't forward it,
>> and here comes the problem since it's running only
>> with RCU when forwarding packets it can see the entry before "is_local"
>> is set to 1 and actually try to dereference NULL.
>> The main issue is that if someone sends a packet to the switch while
>> it's adding the entry which points to the bridge device, it may
>> dereference NULL ptr. This is needed now after we can add fdbs
>> pointing to the bridge.  This poses a problem for
>> br_fdb_update() as well, while someone's adding a bridge fdb, but
>> before it has is_local == 1, it might get moved to a port if it comes
>> as a source mac and then it may get its "is_local" set to 1
>> 
>> This patch changes fdb_create to take is_local and is_static as
>> arguments to set these values in the fdb entry before it is added to the
>> hash. Also adds null check for port in br_forward.
>> 
>> Reported-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>> ---
>> v2 - fix compile error reported by kbuild test robot.
>>      Accidently i had posted an older version of the patch
>> 
> 
> Thanks for fixing this,
> 
> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
> Reviewed-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>

This still doesn't compile:

net/bridge/br_fdb.c: In function ‘br_fdb_external_learn_add’:
net/bridge/br_fdb.c:1103:3: error: too few arguments to function ‘fdb_create’
net/bridge/br_fdb.c:495:37: note: declared here