Bug 240135

Summary:	Correctness issue in IPv6 extension headers input processing routines
Product:	Base System	Reporter:	Prabhakar Lakhera <prabhakar.lakhera>
Component:	kern	Assignee:	Bjoern A. Zeeb <bz>
Status:	Closed FIXED
Severity:	Affects Many People	CC:	bz, cem, pi, ross
Priority:	---
Version:	CURRENT
Hardware:	Any
OS:	Any

Description Prabhakar Lakhera 2019-08-26 21:35:01 UTC

There seems to be a correctness issue in pr_inputs defined for various extension header processing routines for IPv6.
The routines call IP6_EXTHDR_* macros which may end up releasing the mbuf passed to the routine.
Even though the functions are passed pointer to the pointer to mbuf, the pointer is not updated before returning from the routine even for the cases that may not return IPPROTO_DONE.
Change would be to simply update the mbuf pointer along with updating the offset.

Please refer to implementation of route6_input/dest6_input

Comment 1 Bjoern A. Zeeb freebsd_committer

2019-11-06 13:21:58 UTC

I think I have fixed this in a local tree as part of removing most of the PULLDOWN_TEST and IP6_EXTHDR* foo.

I'll hopefully upload a clean patch for review later today and will point you at it.  It's really helpful to have another pair of eyes on these things.

Comment 2 commit-hook freebsd_committer

2019-11-12 15:47:29 UTC

A commit references this bug:

Author: bz
Date: Tue Nov 12 15:46:30 UTC 2019
New revision: 354643
URL: https://svnweb.freebsd.org/changeset/base/354643

Log:
  netinet*: update *mp to pass the proper value back

  In ip6_[direct_]input() we are looping over the extension headers
  to deal with the next header.  We pass a pointer to an mbuf pointer
  to the handling functions.  In certain cases the mbuf can be updated
  there and we need to pass the new one back.  That missing in
  dest6_input() and route6_input().  In tcp6_input() we should also
  update it before we call tcp_input().

  In addition to that mark the mbuf NULL all the times when we return
  that we are done with handling the packet and no next header should
  be checked (IPPROTO_DONE).  This will eventually allow us to assert
  proper behaviour and catch the above kind of errors more easily,
  expecting *mp to always be set.

  This change is extracted from a larger patch and not an exhaustive
  change across the entire stack yet.

  PR:			240135
  Reported by:		prabhakar.lakhera gmail.com
  MFC after:		3 weeks
  Sponsored by:		Netflix

Changes:
  head/sys/netinet/tcp_input.c
  head/sys/netinet6/dest6.c
  head/sys/netinet6/frag6.c
  head/sys/netinet6/icmp6.c
  head/sys/netinet6/ip6_input.c
  head/sys/netinet6/route6.c
  head/sys/netinet6/udp6_usrreq.c

Comment 3 commit-hook freebsd_committer

2020-01-10 23:46:36 UTC

A commit references this bug:

Author: bz
Date: Fri Jan 10 23:46:13 UTC 2020
New revision: 356619
URL: https://svnweb.freebsd.org/changeset/base/356619

Log:
  MFC r354462,354643,354680,354731,354748-354750,354757,354831-354832,
      354861-354863,354865,355254,355450,355452

    netinet*: variable cleanup
    netinet*: update *mp to pass the proper value back
    nd6 defrouter: consolidate nd_defrouter manipulations in nd6_rtr.c
      (excluding some changes to keep public KPI)
    nd6: simplify code
    netinet6: Remove PULLDOWN_TESTs.
    netinet*: replace IP6_EXTHDR_GET()
    IP6_EXTHDR_CHECK(): remove the last instances
    nd6_rtr: consollidation and more consistent naming
    ipv6 tests: Add a simple ping6 test as well.
    icmpv6: Fix mbuf change in mld
    nd6_rtr: re-sort functions
    nd6: make nd6_timer_ch static
    nd6: sysctl
    in6: move include
    Fix m_pullup() problem after removing PULLDOWN_TESTs and KAME EXT* macros.
      (IP6_EXTHDR* macros are preserved in stable).
    ip6_input: remove redundant v4mapped check
    Update comment.

  PR:		240135

Changes:
_U  stable/12/
  stable/12/sys/netinet/ip_carp.c
  stable/12/sys/netinet/tcp_input.c
  stable/12/sys/netinet/udp_usrreq.c
  stable/12/sys/netinet6/dest6.c
  stable/12/sys/netinet6/frag6.c
  stable/12/sys/netinet6/icmp6.c
  stable/12/sys/netinet6/in6.c
  stable/12/sys/netinet6/ip6_input.c
  stable/12/sys/netinet6/ip6_mroute.c
  stable/12/sys/netinet6/mld6.c
  stable/12/sys/netinet6/mld6_var.h
  stable/12/sys/netinet6/nd6.c
  stable/12/sys/netinet6/nd6.h
  stable/12/sys/netinet6/nd6_nbr.c
  stable/12/sys/netinet6/nd6_rtr.c
  stable/12/sys/netinet6/route6.c
  stable/12/sys/netinet6/sctp6_usrreq.c
  stable/12/sys/netinet6/udp6_usrreq.c
  stable/12/sys/netipsec/xform_ah.c
  stable/12/sys/netipsec/xform_esp.c
  stable/12/tests/sys/netinet6/Makefile
  stable/12/tests/sys/netinet6/exthdr.sh
  stable/12/tests/sys/netinet6/mld.py
  stable/12/tests/sys/netinet6/mld.sh

Comment 4 commit-hook freebsd_committer

2020-01-11 01:15:47 UTC

A commit references this bug:

Author: bz
Date: Sat Jan 11 01:15:40 UTC 2020
New revision: 356623
URL: https://svnweb.freebsd.org/changeset/base/356623

Log:
  MFC r354643:

    netinet*: update *mp to pass the proper value back

    In ip6_[direct_]input() we are looping over the extension headers
    to deal with the next header.  We pass a pointer to an mbuf pointer
    to the handling functions.  In certain cases the mbuf can be updated
    there and we need to pass the new one back.  That missing in
    dest6_input() and route6_input().  In tcp6_input() we should also
    update it before we call tcp_input().

    The merge is extracted of a larger change in head.

    PR:			240135
    Reported by:		prabhakar.lakhera gmail.com

Changes:
_U  stable/11/
  stable/11/sys/netinet/tcp_input.c
  stable/11/sys/netinet6/dest6.c
  stable/11/sys/netinet6/icmp6.c
  stable/11/sys/netinet6/route6.c

Comment 5 Bjoern A. Zeeb freebsd_committer

2020-01-11 01:17:41 UTC

Thanks a lot for reporting.  The problems are hopefully fixed in head and the supported stable branches and will be fixed in the next releases.