[net] ipv6: Unlink sibling route in case of failure
diff mbox series

Message ID 20190717203933.3073-1-idosch@idosch.org
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net] ipv6: Unlink sibling route in case of failure
Related show

Commit Message

Ido Schimmel July 17, 2019, 8:39 p.m. UTC
From: Ido Schimmel <idosch@mellanox.com>

When a route needs to be appended to an existing multipath route,
fib6_add_rt2node() first appends it to the siblings list and increments
the number of sibling routes on each sibling.

Later, the function notifies the route via call_fib6_entry_notifiers().
In case the notification is vetoed, the route is not unlinked from the
siblings list, which can result in a use-after-free.

Fix this by unlinking the route from the siblings list before returning
an error.

Audited the rest of the call sites from which the FIB notification chain
is called and could not find more problems.

Fixes: 2233000cba40 ("net/ipv6: Move call_fib6_entry_notifiers up for route adds")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Alexander Petrovskiy <alexpe@mellanox.com>
---
Dave, this will not apply cleanly to stable trees due to recent changes
in net-next. I can prepare another patch for stable if needed.
---
 net/ipv6/ip6_fib.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

David Ahern July 18, 2019, 2:21 p.m. UTC | #1
On 7/17/19 2:39 PM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> When a route needs to be appended to an existing multipath route,
> fib6_add_rt2node() first appends it to the siblings list and increments
> the number of sibling routes on each sibling.
> 
> Later, the function notifies the route via call_fib6_entry_notifiers().
> In case the notification is vetoed, the route is not unlinked from the
> siblings list, which can result in a use-after-free.
> 
> Fix this by unlinking the route from the siblings list before returning
> an error.
> 
> Audited the rest of the call sites from which the FIB notification chain
> is called and could not find more problems.
> 
> Fixes: 2233000cba40 ("net/ipv6: Move call_fib6_entry_notifiers up for route adds")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Alexander Petrovskiy <alexpe@mellanox.com>
> ---
> Dave, this will not apply cleanly to stable trees due to recent changes
> in net-next. I can prepare another patch for stable if needed.
> ---
>  net/ipv6/ip6_fib.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 

Thanks for the fix, Ido. I can help with the ports as well.

Reviewed-by: David Ahern <dsahern@gmail.com>
David Miller July 18, 2019, 7:03 p.m. UTC | #2
From: David Ahern <dsahern@gmail.com>
Date: Thu, 18 Jul 2019 08:21:01 -0600

> On 7/17/19 2:39 PM, Ido Schimmel wrote:
>> From: Ido Schimmel <idosch@mellanox.com>
>> 
>> When a route needs to be appended to an existing multipath route,
>> fib6_add_rt2node() first appends it to the siblings list and increments
>> the number of sibling routes on each sibling.
>> 
>> Later, the function notifies the route via call_fib6_entry_notifiers().
>> In case the notification is vetoed, the route is not unlinked from the
>> siblings list, which can result in a use-after-free.
>> 
>> Fix this by unlinking the route from the siblings list before returning
>> an error.
>> 
>> Audited the rest of the call sites from which the FIB notification chain
>> is called and could not find more problems.
>> 
>> Fixes: 2233000cba40 ("net/ipv6: Move call_fib6_entry_notifiers up for route adds")
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> Reported-by: Alexander Petrovskiy <alexpe@mellanox.com>
>> ---
>> Dave, this will not apply cleanly to stable trees due to recent changes
>> in net-next. I can prepare another patch for stable if needed.
>> ---
>>  net/ipv6/ip6_fib.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
> 
> Thanks for the fix, Ido. I can help with the ports as well.
> 
> Reviewed-by: David Ahern <dsahern@gmail.com>

Applied and queued up for -stable.

Since I've done a bunch of ipv6 routing fix backports through David's
refactoring and cleanups, I'll give this one a try too :-) If I run
into problems I'll ask for help.

Thanks.

Patch
diff mbox series

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 49884f96232b..87f47bc55c5e 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1151,8 +1151,24 @@  static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
 			err = call_fib6_entry_notifiers(info->nl_net,
 							FIB_EVENT_ENTRY_ADD,
 							rt, extack);
-			if (err)
+			if (err) {
+				struct fib6_info *sibling, *next_sibling;
+
+				/* If the route has siblings, then it first
+				 * needs to be unlinked from them.
+				 */
+				if (!rt->fib6_nsiblings)
+					return err;
+
+				list_for_each_entry_safe(sibling, next_sibling,
+							 &rt->fib6_siblings,
+							 fib6_siblings)
+					sibling->fib6_nsiblings--;
+				rt->fib6_nsiblings = 0;
+				list_del_init(&rt->fib6_siblings);
+				rt6_multipath_rebalance(next_sibling);
 				return err;
+			}
 		}
 
 		rcu_assign_pointer(rt->fib6_next, iter);