Bug 243675 - netinet6: Incorrect IPv6 checksum output packets with extension headers
Summary: netinet6: Incorrect IPv6 checksum output packets with extension headers
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Bjoern A. Zeeb
URL:
Keywords: ipv6, patch
Depends on:
Blocks:
 
Reported: 2020-01-28 17:05 UTC by Francis Dupont
Modified: 2020-03-03 08:26 UTC (History)
2 users (show)

See Also:
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
fix (1.56 KB, patch)
2020-01-28 17:05 UTC, Francis Dupont
no flags Details | Diff
tool triggering the problem (1.87 KB, text/plain)
2020-01-28 17:09 UTC, Francis Dupont
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Francis Dupont 2020-01-28 17:05:18 UTC
Created attachment 211135 [details]
fix

sys/netinet6/ip6_output.c ip6_output() lines ~1000

'sw_csum &= ~CSUM_DELAY_DATA_IPV6;' does not make sense as the 'sw_csum' variable is not used after. Obviously it should be replaced by 'm->m_pkthdr.csum_flags'.

'in6_delayed_cksum(m, plen, sizeof(struct ip6_hdr));' plen includes the extension headers so it should be 'plen - option', the start point should be 'sizeof(struct ip6_hdr) + option' i.e. the in6_delayed_cksum() call should be:

'in6_delayed_cksum(m, plen - optlen,
    sizeof(struct ip6_hdr) + optlen);'


I suspect a similar issue with SCTP code lines ~1018

I left line 1029:

`m->m_pkthdr.csum_flags &= ifp->if_hwassist;`

which seems to have no effect (nor sense).

The in6_delayed_cksum() call line 1119 should be fixed the same way (and SCTP code just after too).

I am attaching a diff and a small program triggering the bug (it makes checksum errors on the target and/or tcpdump on outgoing traffic).
Comment 1 Francis Dupont 2020-01-28 17:09:55 UTC
Created attachment 211136 [details]
tool triggering the problem

sendudp+opt.c is a small tool sending UDP packets to the argument destination address with a destination header. Content is taken from the standard input.
Comment 2 Bjoern A. Zeeb freebsd_committer freebsd_triage 2020-01-28 21:47:05 UTC
Thanks Francis.  Great find and thank you for the detailed explanation and the patch.  Made it a lot easier to understand the problem!
I've confirmed the length issues including the extension header length and your patch seems fine for those.
I'll have a look at SCTP tomorrow and also include tests for the FreeBSD test suite and get the fix in.
Comment 3 Michael Tuexen freebsd_committer freebsd_triage 2020-01-28 22:27:08 UTC
(In reply to Bjoern A. Zeeb from comment #2)
What is the issue with SCTP?
Comment 4 Bjoern A. Zeeb freebsd_committer freebsd_triage 2020-02-19 18:44:17 UTC
I've submitted a further cleaned up (though not tested) version to the review system at:
https://reviews.freebsd.org/D23760

Any comments will be welcome there or here.
Comment 5 commit-hook freebsd_committer freebsd_triage 2020-02-24 19:12:45 UTC
A commit references this bug:

Author: bz
Date: Mon Feb 24 19:12:21 UTC 2020
New revision: 358297
URL: https://svnweb.freebsd.org/changeset/base/358297

Log:
  Fix IPv6 checksums when exthdrs are present.

  In two places in ip6_output we are doing (delayed) checksum calculations.
  The initial logic came from SCTP in r205075,205104 and later I copied
  and adjusted it for the TCP|UDP case in r235958.
  The problem was that the original SCTP offsets were already wrong for any
  case with extension headers present given IPv6 extension headers are not
  part of the pseudo checksum calculations.
  The later changes do not help in case there is checksum offloading as for
  extension headers (incl. fragments) we do currrently never offload as we
  have no infrastructure to know whether the NIC can handle these cases.

  Correct the offsets for delayed checksum calculations and properly handle
  mbuf flags.  In addition harmonize the almost identical duplicate code.

  While here eliminate the now unneeded variable hlen and add an always
  missing mtod() call in the 1-b and 3 cases after the introduction of
  the mb_unmapped_to_ext() calls.

  Reported by:	Francis Dupont (fdupont isc.org)
  PR:		243675
  MFC after:	6 days
  Reviewed by:	markj (earlier version), gallatin
  Differential Revision:	https://reviews.freebsd.org/D23760

Changes:
  head/sys/netinet6/ip6_output.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2020-03-02 22:54:49 UTC
A commit references this bug:

Author: bz
Date: Mon Mar  2 22:54:32 UTC 2020
New revision: 358557
URL: https://svnweb.freebsd.org/changeset/base/358557

Log:
  MFC r358297: Fix IPv6 checksums when exthdrs are present.

    In two places in ip6_output we are doing (delayed) checksum calculations.
    The initial logic came from SCTP in r205075,205104 and later I copied
    and adjusted it for the TCP|UDP case in r235958.
    The problem was that the original SCTP offsets were already wrong for any
    case with extension headers present given IPv6 extension headers are not
    part of the pseudo checksum calculations.
    The later changes do not help in case there is checksum offloading as for
    extension headers (incl. fragments) we do currrently never offload as we
    have no infrastructure to know whether the NIC can handle these cases.

    Correct the offsets for delayed checksum calculations and properly handle
    mbuf flags.  In addition harmonize the almost identical duplicate code.

    While here eliminate the now unneeded variable hlen and add an always
    missing mtod() call in the 1-b and 3 cases after the introduction of
    the mb_unmapped_to_ext() calls. [Keep code in sync with head]

  Reported by:	Francis Dupont (fdupont isc.org)
  PR:		243675

Changes:
_U  stable/12/
  stable/12/sys/netinet6/ip6_output.c
Comment 7 commit-hook freebsd_committer freebsd_triage 2020-03-03 08:24:25 UTC
A commit references this bug:

Author: bz
Date: Tue Mar  3 08:24:09 UTC 2020
New revision: 358566
URL: https://svnweb.freebsd.org/changeset/base/358566

Log:
  MFC r358297: Fix IPv6 checksums when exthdrs are present.

      In two places in ip6_output we are doing (delayed) checksum calculations.
      The initial logic came from SCTP in r205075,205104 and later I copied
      and adjusted it for the TCP|UDP case in r235958.
      The problem was that the original SCTP offsets were already wrong for any
      case with extension headers present given IPv6 extension headers are not
      part of the pseudo checksum calculations.
      The later changes do not help in case there is checksum offloading as for
      extension headers (incl. fragments) we do currrently never offload as we
      have no infrastructure to know whether the NIC can handle these cases.

      Correct the offsets for delayed checksum calculations and properly handle
      mbuf flags.  In addition harmonize the almost identical duplicate code.

      While here eliminate the now unneeded variable hlen and add an always
      missing mtod() call in the 1-b and 3 cases after the introduction of
      the mb_unmapped_to_ext() calls. [Keep code in sync with head]

  Reported by:	Francis Dupont (fdupont isc.org)
  PR:		243675

Changes:
_U  stable/11/
  stable/11/sys/netinet6/ip6_output.c
Comment 8 Bjoern A. Zeeb freebsd_committer freebsd_triage 2020-03-03 08:26:40 UTC
Fixed in stable/12 and stable/11 and will be part of all future releases.
I might trigger an EN (Errata Notice) for all currently supported releases as well.