Bug 271573 - panic: ip_output->in_ifaddr_broadcast NULL pointer dereference after route change
Summary: panic: ip_output->in_ifaddr_broadcast NULL pointer dereference after route ch...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.4-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Eric van Gyzen
URL:
Keywords: crash
Depends on:
Blocks:
 
Reported: 2023-05-22 21:30 UTC by Eric van Gyzen
Modified: 2023-05-30 17:13 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric van Gyzen freebsd_committer freebsd_triage 2023-05-22 21:30:08 UTC
FreeBSD 12.4-STABLE #3 stable/12-n236157-886d82afb03

With a busy TCP flow using a non-RTF_HOST route on an IFF_BROADCAST interface, a "route change" operation on that route that changes the interface address can leave "ia" NULL, triggering a NULL pointer dereference.

#6  0xffffffff811020df in trap_pfault (frame=0xfffffe002c1e8420, 
    usermode=<optimized out>, signo=<optimized out>, ucode=<optimized out>)
    at /usr/src/sys/amd64/amd64/trap.c:739
#7  <signal handler called>
#8  0xffffffff80d63c64 in in_ifaddr_broadcast (in=..., ia=0x0)
    at /usr/src/sys/netinet/in.c:1003
#9  0xffffffff80d75ee6 in ip_output (m=<optimized out>, opt=<optimized out>, 
    ro=<optimized out>, flags=<optimized out>, imo=0x0, inp=<optimized out>)
    at /usr/src/sys/netinet/ip_output.c:404
#10 0xffffffff80e03787 in tcp_output (tp=0xfffff8000c922000)
    at /usr/src/sys/netinet/tcp_output.c:1444

There may be multiple changes that contribute to this, but one in particular is commit 1ebec5faf41f, which creates a window in rtrequest1_fib_change() when rt->rt_ifa is NULL.  In my opinion, the right fix would close that window.  This might use the wrong interface address, but it won't panic.  It also won't use freed memory because that's prevented by the net_epoch.

This probably does not affect 13.x or later, which use nexthop.
Comment 1 Eric van Gyzen freebsd_committer freebsd_triage 2023-05-23 14:38:09 UTC
I thought I could simply remove the rt->rt_ifa = NULL assignment in rtrequest1_fib_change, and net_epoch would prevent use-after-free.  However, ip6_output is not fully covered by the net_epoch, so it could use a freed ifa.  It's also not vulnerable to the NULL dereference, so the simple fix would exchange one problem for another.  Maybe I need a fix in ip_output instead.
Comment 2 Eric van Gyzen freebsd_committer freebsd_triage 2023-05-23 15:28:11 UTC
https://reviews.freebsd.org/D40236
Comment 3 Graham Perrin freebsd_committer freebsd_triage 2023-05-27 18:06:55 UTC
Thank you. 

----
^Triage: 

* status
* keyword
* avoidance of [tags] in summary lines

<https://wiki.freebsd.org/Bugzilla/DosAndDonts>
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-05-30 17:13:17 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8fa89d8b190472778ed07db9d8937cb1ce7b44fc

commit 8fa89d8b190472778ed07db9d8937cb1ce7b44fc
Author:     Eric van Gyzen <vangyzen@FreeBSD.org>
AuthorDate: 2023-05-23 09:46:42 +0000
Commit:     Eric van Gyzen <vangyzen@FreeBSD.org>
CommitDate: 2023-05-30 12:10:03 +0000

    Fix NULL deref in ip_output during route change

    When changing the interface address during a route change,
    the rtentry's rt_ifa will be NULL briefly.  Some parts of
    ip_output do not handle that NULL.  In such case, re-validate
    the rtentry.  That validation does not check the rt_ifa, but
    it does lock the route, which will synchronize with
    rtrequest1_fib_change.

    I would prefer to leave the rt_ifa pointer intact during
    the route change, but ip6_output is not fully protected
    by the net_epoch, so that could allow a use-after-free.
    ip6_output already handles a NULL rt_ifa.

    This is a direct commit to stable/12 because later branches
    have nexthop and do not appear to have this bug.

    PR:             271573
    Reported by:    Gaurav.Gandhi@dell.com
    Sponsored by:   Dell EMC Isilon
    Differential Revision:  https://reviews.freebsd.org/D40236

 sys/netinet/ip_output.c | 4 ++++
 1 file changed, 4 insertions(+)