Bug 236737 - recvmsg() under COMPAT_FREEBSD32 returns the wrong msg_controllen value
Summary: recvmsg() under COMPAT_FREEBSD32 returns the wrong msg_controllen value
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.0-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Jason A. Harmening
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-03-23 17:14 UTC by Yuval Pavel Zholkover
Modified: 2019-05-01 14:50 UTC (History)
4 users (show)

See Also:


Attachments
udp recvmsg with IP_RECVDSTADDR, IP_RECVTTL and IP_RECVIF setsockopts enabled (2.61 KB, text/plain)
2019-03-23 17:14 UTC, Yuval Pavel Zholkover
no flags Details
proposed patch (465 bytes, patch)
2019-03-26 08:11 UTC, Jason A. Harmening
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuval Pavel Zholkover 2019-03-23 17:14:17 UTC
Created attachment 203071 [details]
udp recvmsg with IP_RECVDSTADDR, IP_RECVTTL and IP_RECVIF setsockopts enabled

The returned msg_controllen is shorter under FreeBSD AMD64 12.0-RELEASE with COMPAT_FREEBSD32 mode compared to the same value on an actual i386 system.

While the returned msg_controllen is shorter, the corresponding cmsg_len values remain the same.
This causes the last cmsg to exceed the msg_control[0:msg_controllen] buffer.
This does not seem to be detected using the CMSG_NXTHDR/CMSG_DATA macros.

This behavior triggers unit test failures in the Go golang.org/x/net package.
A workaround has been applied in https://go-review.googlesource.com/c/net/+/168297 where the kernel returned msg_controllen is effectively extended to size passed by the caller.

Attached is a C demonstrator based on one of the unit tests.
Comment 1 Jason A. Harmening freebsd_committer freebsd_triage 2019-03-26 08:11:44 UTC
Created attachment 203155 [details]
proposed patch
Comment 2 Jason A. Harmening freebsd_committer freebsd_triage 2019-03-26 08:13:42 UTC
It looks like the 32-bit kernel compat shim for recvmsg() isn't correctly padding the data size for each control message to the required 4-byte alignment when calculating controllen for the output header.  I think the fix may be very simple; the attached patch worked for me.
Comment 3 Yuval Pavel Zholkover 2019-03-29 11:27:08 UTC
(In reply to Jason A. Harmening from comment #2)

Thanks! The golang.org/x/net tests with our workaround reverted now pass with your patch applied.
Will this be MFC'ed to 12.0? Would there be a way to detect whether a FreeBSD-12.0 system carries the patch? (we usually rely on the kern.osreldate sysctl).
Comment 4 commit-hook freebsd_committer freebsd_triage 2019-03-30 23:44:08 UTC
A commit references this bug:

Author: jah
Date: Sat Mar 30 23:43:58 UTC 2019
New revision: 345741
URL: https://svnweb.freebsd.org/changeset/base/345741

Log:
  freebsd32: fix padding of computed control message length for recvmsg()

  Each control message region must be aligned on a 4-byte boundary on 32-bit
  architectures. The 32-bit compat shim for recvmsg() gets the actual layout
  right, but doesn't pad the payload length when computing msg_controllen for
  the output message header. If a control message contains an unaligned
  payload, such as the 1-byte TTL field in the example attached to PR 236737,
  this can produce control message payload boundaries that extend beyond
  the boundary reported by msg_controllen.

  PR:	236737
  Reported by:	Yuval Pavel Zholkover <paulzhol@gmail.com>
  Reviewed by:	markj
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D19768

Changes:
  head/sys/compat/freebsd32/freebsd32_misc.c
Comment 5 Jason A. Harmening freebsd_committer freebsd_triage 2019-03-30 23:49:58 UTC
(In reply to Yuval Pavel Zholkover from comment #3)

Yes, I plan to MFC to both 11 and 12.
I don't think we typically bump osreldate for anything that isn't a feature change or KPI/KBI breakage, but osreldate will at the very least be bumped on the next minor release.
Comment 6 commit-hook freebsd_committer freebsd_triage 2019-04-07 19:03:18 UTC
A commit references this bug:

Author: jah
Date: Sun Apr  7 19:02:33 UTC 2019
New revision: 346019
URL: https://svnweb.freebsd.org/changeset/base/346019

Log:
  MFC r345741:

  freebsd32: fix padding of computed control message length for recvmsg()

  Each control message region must be aligned on a 4-byte boundary on 32-bit
  architectures. The 32-bit compat shim for recvmsg() gets the actual layout
  right, but doesn't pad the payload length when computing msg_controllen for
  the output message header. If a control message contains an unaligned
  payload, such as the 1-byte TTL field in the example attached to PR 236737,
  this can produce control message payload boundaries that extend beyond
  the boundary reported by msg_controllen.

  PR:	236737

Changes:
_U  stable/12/
  stable/12/sys/compat/freebsd32/freebsd32_misc.c
Comment 7 commit-hook freebsd_committer freebsd_triage 2019-04-07 19:08:25 UTC
A commit references this bug:

Author: jah
Date: Sun Apr  7 19:08:07 UTC 2019
New revision: 346020
URL: https://svnweb.freebsd.org/changeset/base/346020

Log:
  MFC r345741:

  freebsd32: fix padding of computed control message length for recvmsg()

  Each control message region must be aligned on a 4-byte boundary on 32-bit
  architectures. The 32-bit compat shim for recvmsg() gets the actual layout
  right, but doesn't pad the payload length when computing msg_controllen for
  the output message header. If a control message contains an unaligned
  payload, such as the 1-byte TTL field in the example attached to PR 236737,
  this can produce control message payload boundaries that extend beyond
  the boundary reported by msg_controllen.

  PR:	236737

Changes:
_U  stable/11/
  stable/11/sys/compat/freebsd32/freebsd32_misc.c