Fix possible use-after-frees with m_cat(9) usage.
ClosedPublic
Actions

Authored by markj on Feb 29 2016, 8:50 PM.

Details

Reviewers

Group Reviewers

network
manpages

Commits

rS296304: Use m_catpkt(9) to ensure that pkthdr fields are updated properly.
rS296303: Set tres to NULL to avoid a double free if the m_pullup() below fails.
rS296302: Use m_catpkt(9) to avoid a possible use-after-free in ieee80211_defrag().
rS296301: Document m_catpkt(), and remove misinformation about m_cat(9).

Summary

m_cat(9) will copy mbufs from the beginning of the second chain into the
last mbuf in the first chain if space is available. In this case, mbufs
from the beginning of the second chain are freed. However, the mbuf man
page claims that the second mbuf chain is always valid on return from
m_cat(9), and there are several use-after-frees that result from this
false assumption.

This is most noticeable in ieee80211_defrag() when INVARIANTS is on: the
mbuf zone destructor overwrites the second chain's pktheader with
0xdeadc0de, resulting in a bogus length that gets added into a valid
mbuf header.

This change fixes the examples of this issue that I can find, documents
m_catpkt, and fixes the m_cat documentation. The code changes would be
committed individually.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj updated this revision to Diff 13901.Feb 29 2016, 8:50 PM

markj retitled this revision from to Fix possible use-after-frees with m_cat(9) usage..

markj edited the test plan for this revision. (Show Details)

markj updated this object.

Herald added a reviewer: manpages. · View Herald TranscriptFeb 29 2016, 8:50 PM

Herald added a subscriber: imp. · View Herald Transcript

markj added reviewers: glebius, network.Feb 29 2016, 8:51 PM

Thanks for doing that. The change to soreceive_stream() worries me, however. At first glance it definitely is not a noop. Reviewing...

In D5497#117229, @glebius wrote:

Thanks for doing that. The change to soreceive_stream() worries me, however. At first glance it definitely is not a noop. Reviewing...

Thanks. Note that the soreceive_stream() change only affects callers that pass in a non-empty mbuf to soreceive(), but I can't find any examples of that in the tree.

The code in soreceive_stream() is a problem. In original BSD code *mp0 was never dereferenced, being a write only variable. We still have places in kernel, where *mp0 is passed uninitalized. So, this m_cat() code is very strange. I'd delete it, but it did appeared quite recently. Commit by Andre in r242309. The revision log doesn't shed light to me.

Also, there is one more unhandled m_cat() with use of second argument down below.

My suggestion is to go on with checkin of m_cat(), but not touching soreceive_stream(). If you "fix" this place, you will add only extra confusion to subversion log. After checking in the main change, we can sit and investigate soreceive_stream(). I believe code can be deleted. But better first to understand what Andre tried to fix. There is also Mikolay referenced in commit. May be he is still reachable and can shed light.

This revision is now accepted and ready to land.Feb 29 2016, 10:42 PM