Bug 224922 - ip_reass should reset M_PKTHDR bit of fragmented packet for NIC which can produce !WRITEABLE mbuf
Summary: ip_reass should reset M_PKTHDR bit of fragmented packet for NIC which can pr...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Navdeep Parhar
Depends on:
Reported: 2018-01-05 05:01 UTC by Harsh Jain
Modified: 2018-01-24 05:10 UTC (History)
4 users (show)

See Also:

Purposed patch by Navdeep. (401 bytes, patch)
2018-01-09 04:52 UTC, Harsh Jain
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Harsh Jain 2018-01-05 05:01:59 UTC
m_unshare() panic in IPSec transport mode when try to ping with size 6000.

panic: m_unshare: m0 0xfffff80020f82600, m 0xfffff8005d054100 has M_PKTHDR
cpuid = 15
time = 1495578455
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2c/frame 0xfffffe044e9bb890
kdb_backtrace() at kdb_backtrace+0x53/frame 0xfffffe044e9bb960
vpanic() at vpanic+0x269/frame 0xfffffe044e9bba30
kassert_panic() at kassert_panic+0xc7/frame 0xfffffe044e9bbac0
m_unshare() at m_unshare+0x578/frame 0xfffffe044e9bbbc0
esp_output() at esp_output+0x44c/frame 0xfffffe044e9bbe40
ipsec4_perform_request() at ipsec4_perform_request+0x5df/frame 0xfffffe044e9bbff0

Refer below mail Chain for more details and possible solution

Comment 1 Eugene Grosbein freebsd_committer 2018-01-05 14:08:06 UTC
(In reply to Harsh Jain from comment #0)

Problem Reports should be self-contained, if possible. Please include relevant information here.
Comment 2 Harsh Jain 2018-01-08 10:40:07 UTC
(In reply to Eugene Grosbein from comment #1)

Ping of size greater than MTU size can panic in m_unshare() when IPSec (transport mode) is configured if NIC can produce !WRITEABLE mbuf for received packet.
Comment 3 Eugene Grosbein freebsd_committer 2018-01-08 12:51:44 UTC
(In reply to Harsh Jain from comment #2)

Care to include diagnostics and attach a diff to the PR?
Comment 4 Harsh Jain 2018-01-09 04:52:22 UTC
Created attachment 189555 [details]
Purposed patch by Navdeep.

Add m_demote_pkthdr() in ip_reass.
Comment 5 Harsh Jain 2018-01-09 04:53:33 UTC
(In reply to Eugene Grosbein from comment #3)

Below is Conversation from Mail Chain

>>> It is not clear to me why it helps. The panic happens on outbound path,
>>> where mbuf should be allocated by network stack and should be writeable.
>>> ip_reass() usually used on inbound path. I think the patch just hides
>>> the problem in another place.
>>> Do you mean that cxgbe can produce !WRITEABLE mbuf for received packet
>>> and then pass it to the network stack?
>> Yes, cxgbe does that.  But I think the real bug here is in ip_reass
>> because it doesn't properly get rid of the pkthdr of the fragments while
>> creating the reassembled datagram.  cxgbe happens to trip on this easily
>> because it often creates !WRITEABLE mbufs.
>  From the quick look, I don't see the code in netipsec and in crypto,
> that does check mbuf is WRITEABLE. It is expected that in most cases for
> received mbuf the data will be decrypted and copied back into the given
> buffer. Can this lead to memory corruption?
>> This should fix it:
>> https://people.freebsd.org/~np/ip_reass_demotehdr.diff
>> It will also fix leaks in configurations where mbuf tags are in use by
>> default (for example with MAC), ip_reass is involved during rx, and the
>> mbuf chain never gets m_demote'd elsewhere (meaning ip_reass should have
>> freed the tags itself).
> I think such chain with several mbufs with M_PKTHDR flag is created with
> m_cat() due to !WRITEABLE mbufs. And when mbuf chain will be freed, the
> tags chain will be also destroyed by mbuf zone destructor.

I see m_freem/m_free will do the right thing but such a chain isn't 
legal.  m_unshare is complaining about it here.  m_sanity on the chain 
will fail too.

m_cat says it will leave the pkthdr alone so it is working as 
advertised.  It's the caller's job to clean up headers etc. to keep the 
mbuf chain valid.

> If you think it solves the problem, the IPv6 fragment reassembly
> probably needs the same code. But I think that M_WRITEABLE flag is not
> properly handled is the problem too.

I think M_WRITEABLE is being handled properly here.  m_unshare deals 
with the chain just fine apart from this assert about multiple M_PKTHDR.

I'll fix IP6 reassembly too and post to phabricator if the change looks ok?
Comment 6 commit-hook freebsd_committer 2018-01-24 05:10:16 UTC
A commit references this bug:

Author: np
Date: Wed Jan 24 05:09:22 UTC 2018
New revision: 328314
URL: https://svnweb.freebsd.org/changeset/base/328314

  Do not generate illegal mbuf chains during IP fragment reassembly.  Only
  the first mbuf of the reassembled datagram should have a pkthdr.

  This was discovered with cxgbe(4) + IPSEC + ping with payload more than
  interface MTU.  cxgbe can generate !M_WRITEABLE mbufs and this results
  in m_unshare being called on the reassembled datagram, and it complains:

  panic: m_unshare: m0 0xfffff80020f82600, m 0xfffff8005d054100 has M_PKTHDR

  PR:		224922
  Reviewed by:	ae@
  MFC after:	1 week
  Sponsored by:	Chelsio Communications
  Differential Revision:	https://reviews.freebsd.org/D14009