Bug 214385 - L2TP control packets malformed [PATCH]
Summary: L2TP control packets malformed [PATCH]
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.3-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Bjoern A. Zeeb
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-11-10 11:44 UTC by Joe Jones
Modified: 2016-11-17 14:04 UTC (History)
3 users (show)

See Also:


Attachments
proposed fix (351 bytes, patch)
2016-11-10 11:44 UTC, Joe Jones
no flags Details | Diff
amended patch (423 bytes, patch)
2016-11-16 12:14 UTC, Joe Jones
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Jones 2016-11-10 11:44:45 UTC
Created attachment 176852 [details]
proposed fix

We noticed L2TP control packets having incorrect sequence numbers causing problems talking to Cisco routers. We traced this back to /usr/src/sys/netgraph/ng_l2tp.c The code is writing to what it thinks 12 bytes of continuous memory, however this can't be guaranteed as the mbuf may have been prepended to. A call to m_pullup is needed, see patch attached.

We believe this may have manifested its self as we are sending bigger packets than the MPD software would normally send due to the addition of proxy auth AVPs, which are not in the stock distribution.

This patch was against 10.3 but will work for 11.0 and probably head as well.
Comment 1 Bjoern A. Zeeb freebsd_committer 2016-11-10 12:23:02 UTC
Wouldn't it be enough to do that pullup in the "else" path right above it rather than outside the block?   In the first case of an empty ZLB it doesn't seem to be needed?
Comment 2 Joe Jones 2016-11-16 12:14:18 UTC
Created attachment 177064 [details]
amended patch

This version does not call m_pullup when the packet is a ZLB.
Comment 3 Joe Jones 2016-11-16 12:20:44 UTC
Hi Bjoern

yes I think you are right, a newly allocated mbup should always have enough space for a 12 byte header. We thought maybe m_pullup was meant to be called anytime the memory was to be accessed, can't see mention of this in the man page though. I have attached an amended patch where m_pullup is only called in the case where M_PREPEND has also been used.
Comment 4 commit-hook freebsd_committer 2016-11-17 14:04:15 UTC
A commit references this bug:

Author: bz
Date: Thu Nov 17 14:03:44 UTC 2016
New revision: 308748
URL: https://svnweb.freebsd.org/changeset/base/308748

Log:
  Writing out the L2TP control packet requires 12 bytes of
  contiguous memory but in one path we did not always guarantee this,
  thus do a m_pullup() there.

  PR:			214385
  Submitted by:		Joe Jones (joeknockando googlemail.com)
  MFC after:		3 days

Changes:
  head/sys/netgraph/ng_l2tp.c