Bug 266442 - kernel page fault on packet with broken lengths if ipfilter is loaded
Summary: kernel page fault on packet with broken lengths if ipfilter is loaded
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Cy Schubert
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-16 10:10 UTC by Robert Morris
Modified: 2023-02-09 21:22 UTC (History)
4 users (show)

See Also:


Attachments
Inject a packet that causes a kernel page fault if ipfilter is loaded. (2.45 KB, text/plain)
2022-09-16 10:10 UTC, Robert Morris
no flags Details
Notify pfil of dropped mangled packet (3.86 KB, patch)
2023-02-02 01:09 UTC, Cy Schubert
no flags Details | Diff
Updated patch with DTrace probe (3.91 KB, patch)
2023-02-02 01:20 UTC, Cy Schubert
no flags Details | Diff
Also add to the fr_bad packet count. (3.94 KB, patch)
2023-02-02 02:47 UTC, Cy Schubert
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2022-09-16 10:10:59 UTC
Created attachment 236590 [details]
Inject a packet that causes a kernel page fault if ipfilter is loaded.

If ipfilter is loaded, and a packet arrives with IP hlen = 20 bytes
(the default), IP packet len = 67 bytes, and TCP th_off = 48 bytes,
ipf_pullup() will call m_pullup(len=68), which fails and causes
ipf_pullup() to free the mbuf and zero out the mbuf pointer in
*fin->fin_mp. But the information that the packet was discarded is
lost because ipf_pr_ipv4hdr() does not return an error to
ipf_makefrip(). So the calling code thinks everything is OK,
ip_tryforward() sees PFIL_PASS and uses m = *fin->fin_mp, and it
crashes.

Error indications should probably be made to flow up from ipf_pullup()
through ipf_pr_ipv4hdr() to ipf_makefrip(), so that callers know not
to try to use the freed mbuf.

I've attached a demo:

# cc -o pf7a pf7a.c
# ./pf7a
...
panic: Fatal page fault at 0xffffffc000488f14: 0x0000000000001d
panic() at panic+0x2a
page_fault_handler() at page_fault_handler+0x1d6
do_trap_supervisor() at do_trap_supervisor+0x76
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x70
--- exception 13, tval = 0x1d
ip_tryforward() at ip_tryforward+0x278
ip_input() at ip_input+0x356
netisr_dispatch_src() at netisr_dispatch_src+0xca
netisr_dispatch() at netisr_dispatch+0x10
tunwrite_l3() at tunwrite_l3+0x182
tunwrite() at tunwrite+0x128
devfs_write_f() at devfs_write_f+0xa6
fo_write() at fo_write+0xa
dofilewrite() at dofilewrite+0x66
kern_writev() at kern_writev+0x40
sys_write() at sys_write+0x54
syscallenter() at syscallenter+0xec
ecall_handler() at ecall_handler+0x18
do_trap_user() at do_trap_user+0xea
cpu_exception_handler_user() at cpu_exception_handler_user+0x72

Here's the call chain at the point where m_pullup() fails:

  ipf_pullup() at ipf_pullup+0x182
  ipf_pr_pullup() at ipf_pr_pullup+0x5c
  ipf_pr_tcpcommon() at ipf_pr_tcpcommon+0x28e
  ipf_pr_tcp() at ipf_pr_tcp+0x46
  ipf_pr_ipv4hdr() at ipf_pr_ipv4hdr+0x220
  ipf_makefrip() at ipf_makefrip+0x60
  ipf_check() at ipf_check+0x142
  ipf_check_wrapper() at ipf_check_wrapper+0x88
  pfil_mbuf_in() at pfil_mbuf_in+0x58
  ip_tryforward() at ip_tryforward+0x1c0
  ip_input() at ip_input+0x356
Comment 1 Cy Schubert freebsd_committer freebsd_triage 2023-01-20 17:06:20 UTC
Confirmed.
Comment 2 Cy Schubert freebsd_committer freebsd_triage 2023-01-20 18:31:31 UTC
Same page fault occurs without ipfilter loaded.
Comment 3 Cy Schubert freebsd_committer freebsd_triage 2023-02-02 01:09:25 UTC
Created attachment 239852 [details]
Notify pfil of dropped mangled packet

Can you try this patch, please. Though it fixes the problem the final patch may return a different return code or may increment a counter. For now it avoids the panic.
Comment 4 Cy Schubert freebsd_committer freebsd_triage 2023-02-02 01:20:06 UTC
Created attachment 239853 [details]
Updated patch with DTrace probe

This patch adds a DTrace SDT.
Comment 5 Cy Schubert freebsd_committer freebsd_triage 2023-02-02 02:47:28 UTC
Created attachment 239857 [details]
Also add to the fr_bad packet count.

In addition to a DTrace probe, add to the bad packet count. These are the proposed two commits.
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-02-02 17:42:34 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=79f7745c098a766d34a4e072cdd1a06e6d0829d5

commit 79f7745c098a766d34a4e072cdd1a06e6d0829d5
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2023-02-02 00:49:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2023-02-02 17:41:22 +0000

    ipfilter: Fix use after free on packet with broken lengths

    Under the scenario with a packet with length of 67 bytes, a header length
    using the default of 20 bytes and a TCP data offset (th_off) of 48 will
    cause m_pullup() to fail to make sure bytes are arragned contiguously.
    m_pullup() will free the mbuf chain and return a null. ipfilter stores
    the resultant mbuf address (or the resulting NULL) in its fr_info_t
    structure. Unfortuntely the eroneous packet is not flagged for drop.
    This results in a kernel page fault at line 410 of sys/netinet/ip_fastfwd.c
    as it tries to use a now previously freed, by m_pullup(), mbuf.

    PR:             266442
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week

 sys/netpfil/ipfilter/netinet/fil.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Comment 7 Robert Morris 2023-02-03 11:15:05 UTC
(In reply to Cy Schubert from comment #3)
The latest kernel source (as of Feb 3) makes the reported crash
go away when I try it.
Comment 8 Cy Schubert freebsd_committer freebsd_triage 2023-02-03 15:46:44 UTC
It was indeed an IP Filter bug. It was not telling pfil to drop the packet when m_pullup() had freed the mbuf chain while failing. The bug likely exists in NetBSD too. I've sent them an email.

This ticket will be closed after the patch has been MFCed to stable.
Comment 9 commit-hook freebsd_committer freebsd_triage 2023-02-09 21:20:24 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1da7a8a066150bf132b3e1a48fad009212a0010a

commit 1da7a8a066150bf132b3e1a48fad009212a0010a
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2023-02-02 00:49:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2023-02-09 21:19:41 +0000

    ipfilter: Fix use after free on packet with broken lengths

    Under the scenario with a packet with length of 67 bytes, a header length
    using the default of 20 bytes and a TCP data offset (th_off) of 48 will
    cause m_pullup() to fail to make sure bytes are arragned contiguously.
    m_pullup() will free the mbuf chain and return a null. ipfilter stores
    the resultant mbuf address (or the resulting NULL) in its fr_info_t
    structure. Unfortuntely the eroneous packet is not flagged for drop.
    This results in a kernel page fault at line 410 of sys/netinet/ip_fastfwd.c
    as it tries to use a now previously freed, by m_pullup(), mbuf.

    PR:             266442
    Reported by:    Robert Morris <rtm@lcs.mit.edu>

    (cherry picked from commit 79f7745c098a766d34a4e072cdd1a06e6d0829d5)

 sys/netpfil/ipfilter/netinet/fil.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2023-02-09 21:21:26 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=351f2f68852ac3e1d0ef745dc024dd745b07a34f

commit 351f2f68852ac3e1d0ef745dc024dd745b07a34f
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2023-02-02 00:49:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2023-02-09 21:20:51 +0000

    ipfilter: Fix use after free on packet with broken lengths

    Under the scenario with a packet with length of 67 bytes, a header length
    using the default of 20 bytes and a TCP data offset (th_off) of 48 will
    cause m_pullup() to fail to make sure bytes are arragned contiguously.
    m_pullup() will free the mbuf chain and return a null. ipfilter stores
    the resultant mbuf address (or the resulting NULL) in its fr_info_t
    structure. Unfortuntely the eroneous packet is not flagged for drop.
    This results in a kernel page fault at line 410 of sys/netinet/ip_fastfwd.c
    as it tries to use a now previously freed, by m_pullup(), mbuf.

    PR:             266442
    Reported by:    Robert Morris <rtm@lcs.mit.edu>

    (cherry picked from commit 79f7745c098a766d34a4e072cdd1a06e6d0829d5)

 sys/netpfil/ipfilter/netinet/fil.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)