From: bluhm <bluhm@openbsd.org>
Date: Mon, 13 Nov 2023 17:18:27 +0000 (+0000)
Subject: Fix rt_setgate() error handling.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a08e228de7c095ff2203bb7331b22dd57a62c9c5;p=openbsd

Fix rt_setgate() error handling.

In revision 1.424 the logic in rt_setgate() has changed.  The old
code entered a value into rt_gateway also if rt_setgwroute() returned
an error.  Now if rt_setgwroute() fails, rt_gateway is NULL and
ROUNDUP(rt->rt_gateway->sa_len) crashes.

Put back the old logic in rt_setgate().  Setting rt_gateway and
rt_gwroute are actually independent.

If malloc(9) in rt_setgate() fails, rt_gateway can still be NULL.
The subsequent crash in free(rt->rt_gateway, M_RTABLE,
ROUNDUP(rt->rt_gateway->sa_len)) was just never observed.  Add a
NULL check around these free(9).

Reported-by: syzbot+2e79dd9db712d3c5ade9@syzkaller.appspotmail.com
OK mvs@
---

diff --git a/sys/net/route.c b/sys/net/route.c
index f6930ba73ee..0b97254765f 100644
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: route.c,v 1.425 2023/11/12 17:51:40 bluhm Exp $	*/
+/*	$OpenBSD: route.c,v 1.426 2023/11/13 17:18:27 bluhm Exp $	*/
 /*	$NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $	*/
 
 /*
@@ -517,7 +517,10 @@ rtfree(struct rtentry *rt)
 #ifdef MPLS
 	rt_mpls_clear(rt);
 #endif
-	free(rt->rt_gateway, M_RTABLE, ROUNDUP(rt->rt_gateway->sa_len));
+	if (rt->rt_gateway != NULL) {
+		free(rt->rt_gateway, M_RTABLE,
+		    ROUNDUP(rt->rt_gateway->sa_len));
+	}
 	free(rt_key(rt), M_RTABLE, rt_key(rt)->sa_len);
 
 	pool_put(&rtentry_pool, rt);
@@ -937,8 +940,10 @@ rtrequest(int req, struct rt_addrinfo *info, u_int8_t prio,
 			ifafree(ifa);
 			rtfree(rt->rt_parent);
 			rt_putgwroute(rt, NULL);
-			free(rt->rt_gateway, M_RTABLE,
-			    ROUNDUP(rt->rt_gateway->sa_len));
+			if (rt->rt_gateway != NULL) {
+				free(rt->rt_gateway, M_RTABLE,
+				    ROUNDUP(rt->rt_gateway->sa_len));
+			}
 			free(ndst, M_RTABLE, ndst->sa_len);
 			pool_put(&rtentry_pool, rt);
 			return (error);
@@ -970,8 +975,10 @@ rtrequest(int req, struct rt_addrinfo *info, u_int8_t prio,
 			ifafree(ifa);
 			rtfree(rt->rt_parent);
 			rt_putgwroute(rt, NULL);
-			free(rt->rt_gateway, M_RTABLE,
-			    ROUNDUP(rt->rt_gateway->sa_len));
+			if (rt->rt_gateway != NULL) {
+				free(rt->rt_gateway, M_RTABLE,
+				    ROUNDUP(rt->rt_gateway->sa_len));
+			}
 			free(ndst, M_RTABLE, ndst->sa_len);
 			pool_put(&rtentry_pool, rt);
 			return (EEXIST);
@@ -996,6 +1003,7 @@ rt_setgate(struct rtentry *rt, const struct sockaddr *gate, u_int rtableid)
 {
 	int glen = ROUNDUP(gate->sa_len);
 	struct sockaddr *sa, *osa;
+	int error = 0;
 
 	KASSERT(gate != NULL);
 	if (rt->rt_gateway == gate) {
@@ -1009,23 +1017,17 @@ rt_setgate(struct rtentry *rt, const struct sockaddr *gate, u_int rtableid)
 	memcpy(sa, gate, gate->sa_len);
 
 	KERNEL_LOCK(); /* see [X] in route.h */
-	if (ISSET(rt->rt_flags, RTF_GATEWAY)) {
-		int error = rt_setgwroute(rt, gate, rtableid);
-		if (error != 0) {
-			KERNEL_UNLOCK();
-			free(sa, M_RTABLE, glen);
-			return (error);
-		}
-	}
-
 	osa = rt->rt_gateway;
 	rt->rt_gateway = sa;
+
+	if (ISSET(rt->rt_flags, RTF_GATEWAY))
+		error = rt_setgwroute(rt, gate, rtableid);
 	KERNEL_UNLOCK();
 
 	if (osa != NULL)
 		free(osa, M_RTABLE, ROUNDUP(osa->sa_len));
 
-	return (0);
+	return (error);
 }
 
 /*