[PATCH net 0/2] net: nexthop: multipath null ptr deref fixes

[PATCH net 0/2] net: nexthop: multipath null ptr deref fixes

[Nikolay Aleksandrov]

Hi,
Recently I found that we can cause a null ptr dereference when using
nexthop groups due to .nh getting set to null or due to num_nh going to
0 while we're selecting a path and the nexthop multipath group is being
modified. The reason is that the .nh ptr is set and read without any sync
primitives so in theory it can become null at any point (being cleared on
nh group removal), and also the nh count in a group (num_nh), when it
becomes == 0 while destroying a nh group and we hit it then in
nexthop_select_path() rc would remain == NULL and we'll deref a null ptr.
So there are 2 separate issues, first is nexthop_select_path
dereferencing .nh without any checks and the second is nexthop_select_path
users dereferencing its return value without checking it first. This set
fixes both issues as they are closely related and caused by the same
actions: nexthop delete or replace.

The null ptr deref can be caused by running traffic to a route using a
multipath nexthop and in parallel doing replaces of one of the nexthop
members or while deleting the nexthop group (again in parallel).
I've run the above test for a few hours with these fixes in place.

Here's one such trace:
	[  322.517290] BUG: kernel NULL pointer dereference, address: 0000000000000070
	[  322.517670] #PF: supervisor read access in kernel mode
	[  322.517935] #PF: error_code(0x0000) - not-present page
	[  322.518213] PGD 0 P4D 0 
	[  322.518388] Oops: 0000 [#1] SMP PTI
	[  322.518601] CPU: 1 PID: 58185 Comm: ping Not tainted 5.7.0-rc5+ #190
	[  322.518911] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
	[  322.519490] RIP: 0010:fib_select_multipath+0x5a/0x2ac
	[  322.519776] Code: 85 db 48 89 44 24 08 40 0f 95 c6 31 c9 31 d2 e8 29 13 93 ff 48 85 db 74 58 48 8b 45 18 8b 74 24 04 48 8b 78 68 e8 81 b7 00 00 <48> 8b 58 70 e8 6c 04 8b ff 85 c0 74 31 80 3d cd 89 d4 00 00 75 28
	[  322.520536] RSP: 0018:ffff888228f6bac0 EFLAGS: 00010286
	[  322.520813] RAX: 0000000000000000 RBX: ffff888228cc3c00 RCX: 0000000000000000
	[  322.521152] RDX: ffff888222215080 RSI: ffff888222215930 RDI: ffff888222215080
	[  322.521478] RBP: ffff888228f6bbd8 R08: ffff888222215930 R09: 0000000000020377
	[  322.521815] R10: 0000000000000000 R11: 784deca9f66dea1e R12: 0000000000000000
	[  322.522143] R13: ffff88822a099000 R14: ffff888228f6bbd8 R15: ffffffff8258cc80
	[  322.522491] FS:  00007fc5ee6a8000(0000) GS:ffff88822bc80000(0000) knlGS:0000000000000000
	[  322.522862] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	[  322.523236] CR2: 0000000000000070 CR3: 0000000222954001 CR4: 0000000000360ee0
	[  322.523657] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
	[  322.524060] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
	[  322.524461] Call Trace:
	[  322.524707]  fib_select_path+0x5a/0x2c8
	[  322.524998]  ip_route_output_key_hash_rcu+0x2d6/0x636
	[  322.525372]  ip_route_output_key_hash+0x9f/0xb6
	[  322.525697]  ip_route_output_flow+0x1c/0x58
	[  322.525990]  raw_sendmsg+0x5e9/0xca4
	[  322.526261]  ? mark_lock+0x68/0x24d
	[  322.526536]  ? lock_acquire+0x233/0x24f
	[  322.526823]  ? raw_abort+0x3f/0x3f
	[  322.527086]  ? inet_send_prepare+0x3b/0x3b
	[  322.527418]  ? sock_sendmsg_nosec+0x4f/0x9b
	[  322.527721]  ? raw_abort+0x3f/0x3f
	[  322.527984]  sock_sendmsg_nosec+0x4f/0x9b
	[  322.528274]  __sys_sendto+0xdd/0x100
	[  322.528551]  ? sockfd_lookup_light+0x72/0x96
	[  322.528851]  ? trace_hardirqs_on_thunk+0x1a/0x1c
	[  322.529159]  __x64_sys_sendto+0x25/0x28
	[  322.529442]  do_syscall_64+0xd1/0xe1
	[  322.529719]  entry_SYSCALL_64_after_hwframe+0x49/0xb3

Thanks,
 Nik

Nikolay Aleksandrov (2):
  net: nexthop: dereference nh only once in nexthop_select_path
  net: nexthop: check for null return by nexthop_select_path()

 include/net/nexthop.h |  5 ++++-
 net/ipv4/nexthop.c    | 13 +++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.25.2

[PATCH net 1/2] net: nexthop: dereference nh only once in nexthop_select_path

[Nikolay Aleksandrov]

the ->nh pointer might become suddenly null while we're selecting the
path and we may dereference it. Dereference it only once in the
beginning and use that if it's not null, we rely on the refcounting and
rcu to protect against use-after-free.

Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/ipv4/nexthop.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 2a31c4af845e..a6ffdb067253 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -490,28 +490,33 @@ struct nexthop *nexthop_select_path(struct nexthop *nh, int hash)
 	nhg = rcu_dereference(nh->nh_grp);
 	for (i = 0; i < nhg->num_nh; ++i) {
 		struct nh_grp_entry *nhge = &nhg->nh_entries[i];
+		struct nexthop *nhge_nh;
 		struct nh_info *nhi;
 
 		if (hash > atomic_read(&nhge->upper_bound))
 			continue;
 
+		nhge_nh = READ_ONCE(nhge->nh);
+		if (unlikely(!nhge_nh))
+			continue;
+
 		/* nexthops always check if it is good and does
 		 * not rely on a sysctl for this behavior
 		 */
-		nhi = rcu_dereference(nhge->nh->nh_info);
+		nhi = rcu_dereference(nhge_nh->nh_info);
 		switch (nhi->family) {
 		case AF_INET:
 			if (ipv4_good_nh(&nhi->fib_nh))
-				return nhge->nh;
+				return nhge_nh;
 			break;
 		case AF_INET6:
 			if (ipv6_good_nh(&nhi->fib6_nh))
-				return nhge->nh;
+				return nhge_nh;
 			break;
 		}
 
 		if (!rc)
-			rc = nhge->nh;
+			rc = nhge_nh;
 	}
 
 	return rc;
-- 
2.25.2

Re: [PATCH net 1/2] net: nexthop: dereference nh only once in nexthop_select_path

[David Ahern]

On 5/19/20 5:04 AM, Nikolay Aleksandrov wrote:
> the ->nh pointer might become suddenly null while we're selecting the
> path and we may dereference it. Dereference it only once in the
> beginning and use that if it's not null, we rely on the refcounting and
> rcu to protect against use-after-free.

the num_nh is also affected. I think an rcu update of the entire nh_grp
is the better solution. Dataplane should always see a valid nh_grp via rcu.

[PATCH net 2/2] net: nexthop: check for null return by nexthop_select_path()

[Nikolay Aleksandrov]

nexthop_select_path() may return null if either .nh is null or the
number of nexthops is 0 (rc == NULL). We need to check its return value
before use to avoid deferencing a null ptr.

Fixes: 4c7e8084fd46 ("ipv4: Plumb support for nexthop object in a fib_info")
Fixes: f88d8ea67fbd ("ipv6: Plumb support for nexthop object in a fib6_info")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Could you please confirm that simply returning in the IPv6 case is ok?
AFAICT it's fine, I've also tested it, but I'm a bit worried about
ip6_pol_route_lookup -> ip6_create_rt_rcu and the second directly
deferencing res->nh. I think rt6_device_match() should take care of
that case, but I'd appreciate more eyes on that. :)

 include/net/nexthop.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net/nexthop.h b/include/net/nexthop.h
index c440ccc861fc..7cc4343cdbfc 100644
--- a/include/net/nexthop.h
+++ b/include/net/nexthop.h
@@ -203,6 +203,8 @@ static inline void nexthop_path_fib_result(struct fib_result *res, int hash)
 	struct nexthop *nh;
 
 	nh = nexthop_select_path(res->fi->nh, hash);
+	if (unlikely(!nh))
+		return;
 	nhi = rcu_dereference(nh->nh_info);
 	res->nhc = &nhi->fib_nhc;
 }
@@ -290,7 +292,8 @@ static inline void nexthop_path_fib6_result(struct fib6_result *res, int hash)
 	struct nh_info *nhi;
 
 	nh = nexthop_select_path(nh, hash);
-
+	if (unlikely(!nh))
+		return;
 	nhi = rcu_dereference_rtnl(nh->nh_info);
 	if (nhi->reject_nh) {
 		res->fib6_type = RTN_BLACKHOLE;
-- 
2.25.2

Re: [PATCH net 0/2] net: nexthop: multipath null ptr deref fixes

[David Ahern]

Dave: just talked to Nik. He is going to look at an rcu replace of the
nh_grp struct versus this change. Please drop this set for now.
Thanks,