Page MenuHomeFreeBSD

Free rights in truncated control messages.
ClosedPublic

Authored by markj on Aug 1 2018, 11:38 PM.

Details

Summary

Control messages may be lost in some cases: if copyout() fails, or if
the receiving process does not supply sufficient buffer space. In the
process of externalizing these control messages, their data may now
reference kernel resources; if these references cannot be transferred to
the recipient, they must be properly disposed of. This occurs
specifically when transmitting SCM_RIGHTS messages over a unix socket,
and results in a file descriptor leak when the control message mbuf
chain is freed.

Fix the problem using a new function, m_dispose_extcontrolm(), which is
responsible for releasing any resources contained in an externalized
control message. Such messages are stored in mbufs with a distinguished
type; once the resources are disposed, the type changes back to
MT_CONTROL. Right now, the only source of MT_EXTCONTROL messages will
use a separate mbuf for each control message, but
m_dispose_extcontrolm() attempts to handle multiple control messages in
a single mbuf.

If an externalized message would be truncated part way through, drop the
whole message. This makes disposal much simpler and follows NetBSD's
behaviour. I can't see any reason to try a partial copy as we do today,
and it would be tricky to support that.

Also fix a related bug in kern_recvit(): if truncation happens at an
mbuf boundary, we were failing to set MSG_CTRUNC.

Test Plan

Regression tests in D16562.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18510
Build 18215: arc lint + arc unit

Event Timeline

markj created this revision.Aug 1 2018, 11:38 PM
markj edited the test plan for this revision. (Show Details)Aug 1 2018, 11:40 PM
markj added reviewers: ed, network.
ed accepted this revision.Aug 2 2018, 6:13 AM

Looks good. Thanks a lot for working on this!

That said, I'm never a true expert when it comes to our network stack. Maybe best if some in addition to me reviews the changes to sys/kern/uipc_syscalls.c?

sys/compat/freebsd32/freebsd32_misc.c
1197

Nit: extraneous parentheses.

This revision is now accepted and ready to land.Aug 2 2018, 6:13 AM
markj marked an inline comment as done.Aug 2 2018, 2:30 PM
In D16561#351555, @ed wrote:

Looks good. Thanks a lot for working on this!
That said, I'm never a true expert when it comes to our network stack. Maybe best if some in addition to me reviews the changes to sys/kern/uipc_syscalls.c?

Hopefully someone from network will chime in. :) I just wanted to make sure you were happy with the cloudabi changes.

markj added inline comments.Aug 2 2018, 3:15 PM
sys/sys/mbuf.h
637

This header is probably the wrong place for a function defined in uipc_syscalls.c, but I'm not sure where it would logically go. Suggestions are welcome.

markj updated this revision to Diff 46355.Aug 6 2018, 8:05 PM

Fix bugs in freebsd32.

  • Set MSG_CTRUNC if truncation occurs at a control message boundary.
  • Only update cmsg_len when copying out to avoid later confusion in m_dispose_extcontrolm().
This revision now requires review to proceed.Aug 6 2018, 8:05 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 7 2018, 4:36 PM
This revision was automatically updated to reflect the committed changes.