[PATCH] net: mctp: hold a reference to the route device in mctp_route_lookup()

[PATCH] net: mctp: hold a reference to the route device in mctp_route_lookup()

[Aldo Ariel Panzardo]

mctp_route_lookup() uses rt->dev without holding a reference on it.
mctp_route_lookup_single() returns the route under RCU only, so the
route's device can be torn down concurrently: mctp_dev_put() drops the
last reference and synchronously kfree()s mdev->addrs.  mctp_dev_saddr()
then reads rt->dev->addrs[0], giving a use-after-free reachable by an
unprivileged local AF_MCTP user on the receive/forwarding path (no
CAP_NET_RAW required):

  BUG: KASAN: slab-use-after-free in mctp_route_lookup
  Read of size 1 at addr ... by task mctp_uaf/...
   mctp_route_lookup
   mctp_pkttype_receive
  Freed by task ...:
   kfree
   mctp_dev_put
   mctp_dev_notify

In the same window mctp_dst_from_route() -> mctp_dev_hold() also
increments a refcount that has already reached zero
("refcount_t: addition on 0 ... mctp_dev_hold").

This reintroduces the use-after-free class of CVE-2023-3439: the source
address lookup was moved ahead of the point where the destination takes
its device reference.

Take a reference with refcount_inc_not_zero() before touching rt->dev,
skip a device that is already dead, and drop the reference once the
destination has taken its own.

Fixes: 22cb45afd221 ("net: mctp: perform source address lookups when we populate our dst")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
A reproducer is available on request.

 net/mctp/route.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/mctp/route.c b/net/mctp/route.c
index 1f3dccbb7a..b19c63a569 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -998,14 +998,29 @@ int mctp_route_lookup(struct net *net, unsigned int dnet,
 			mtu = mtu ?: rt->mtu;
 
 		if (rt->dst_type == MCTP_ROUTE_DIRECT) {
-			mctp_eid_t saddr = mctp_dev_saddr(rt->dev);
+			mctp_eid_t saddr;
+
+			/* rt->dev may be going away concurrently: its last
+			 * reference is dropped in mctp_dev_put(), which frees
+			 * mdev->addrs that mctp_dev_saddr() reads, and
+			 * mctp_dst_from_route() takes a reference on it.  Pin
+			 * it before use, and skip a device that is already
+			 * dead rather than resurrecting it.
+			 */
+			if (!refcount_inc_not_zero(&rt->dev->refs))
+				break;
+
+			saddr = mctp_dev_saddr(rt->dev);
 
 			/* cannot do gateway-ed routes without a src  */
-			if (saddr == MCTP_ADDR_NULL && depth != 0)
+			if (saddr == MCTP_ADDR_NULL && depth != 0) {
+				mctp_dev_put(rt->dev);
 				break;
+			}
 
 			if (dst)
 				mctp_dst_from_route(dst, daddr, saddr, mtu, rt);
+			mctp_dev_put(rt->dev);
 			rc = 0;
 			break;
 
-- 
2.43.0

Re: [PATCH] net: mctp: hold a reference to the route device in mctp_route_lookup()

[Jeremy Kerr]

Hi Aldo,

> mctp_route_lookup() uses rt->dev without holding a reference on it.

[...]

> Take a reference with refcount_inc_not_zero() before touching rt->dev,
> skip a device that is already dead, and drop the reference once the
> destination has taken its own.

This looks the same as Yiqi Sun's analysis:

  https://lore.kernel.org/netdev/20260731022926.913865-1-sunyiqixm@gmail.com/

I had requested some simplifications for the proposed fix, which would
likely result in something very close to your contribution here.

Yiqi had not followed up from that review as yet, though; so if we don't
see any updated patch shortly, then your implementation looks reasonable.

Regards,


Jeremy