Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
iochannel: Strictly specify PF_UNIX ancillary data boundaries
Users reported audio breakage for 32-bit pulse clients connected
to a 64-bit server over memfds. Investigating the issue further,
the problem is twofold:

1. iochannel's file-descriptor passing code is liberal in what it
   issues: produced ancillary data object's "data" section exceeds
   length field. How such an extra space is handled is a grey area
   in the POSIX.1g spec, the IETF RFC #2292 "Advanced Sockets API
   for IPv6" memo, and the cmsg(3) manpage.

2. A 64-bit kernel handling of such extra space differs by whether
   the app is 64-bit or 32-bit. For 64-bit apps, the kernel
   smartly ducks the issue. For 32-bit apps, an -EINVAL is
   directly returned; that's due to a kernel CMSG header traversal
   bug in the networking stack "32-bit sockets emulation layer".

   Compare Linux Kernel's socket.h cmsg_nxthdr() code and the
   32-bit emulation layer version of it at net/compat.c
   cmsg_compat_nxthdr() for further info. Notice how the former
   graciously ignores incomplete CMSGs while the latter _directly_
   complains about them -- as of kernel version 4.9-rc5.

   (A kernel patch is to be submitted)

Details:

iochannel typically uses sendmsg() for passing FDs & credentials.
>From RFC 2292, sendmsg() control data is just a heterogeneous
array of embedded ancillary objects that can differ in length.
Linguistically, a "control message" is an ancillary data object.

For example, below is a sendmsg() "msg_control" containing two
ancillary objects:

|<---------------------- msg_controllen---------------------->|
|                                                             |
|<--- ancillary data object -->|<----- ancillary data object->|
|<------- CMSG_SPACE() ------->|<------- CMSG_SPACE() ------->|
|                              |                              |
|<-------- cmsg_len ------->|  |<-------- cmsg_len ------->|  |
|<------- CMSG_LEN() ------>|  |<------- CMSG_LEN() ------>|  |
|                           |  |                           |  |
+-----+-----+-----+--+------+--+-----+-----+-----+--+------+--+
|cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|
|len  |level|type |XX|data[]|XX|len  |level|type |XX|data[]|XX|
+-----+-----+-----+--+------+--+-----+-----+-----+--+----+-+--+
 ^^^^^^^ Ancil Object #1        ^^^^^^^ Ancil Object #2
         (control message)              (control message)
^
|
+--- sendmsg() "msg_control" points here

Problem is, while passing FDs, iochannel's code try to avoid
variable-length arrays by creating a single cmsg object that can
fit as much FDs as possible:

  union {
    struct cmsghdr hdr;
    uint8_t data[CMSG_SPACE(sizeof(int) * MAX_ANCIL_DATA_FDS)];
  } cmsg;                                 ^^^^^^^^^^^^^^^^^^

Most of the time though the number of FDs to be passed is less
than the maximum above, thus "cmsg_len" is set to the _actual_ FD
array size:

  cmsg.hdr.cmsg_len = CMSG_LEN(sizeof(int) * nfd);
                                             ^^^
This inconsistency tricks the kernel into thinking that we have 2
ancillay data objects instead of one! First cmsg is valid as
intended, but the second is instantly _corrupt_ since it has a
cmsg_len size of 0 -- thus failing kernel's CMSG_OK() tests.

For 32-bit apps on a 32-bit kernel, and 64-bit apps over a 64-bit
one, the kernel's own CMSG header traversal macros just ignore the
second "incomplete" cmsg. For 32-bit apps over a 64-bit kernel
though, the kernel 32-bit socket emulation macros does not forgive
such incompleteness and directly complains of invalid args (due to
a subtle bug).

Avoid this ugly problem, which can also bite us in a pure 64-bit
environment if MAX_ANCIL_DATA_FDS got extended to 5 FDs, by
setting "cmsg_data[]" array size to "cmsg_len".

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97769

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
  • Loading branch information
a-darwish authored and tanuk committed Nov 17, 2016
1 parent e262379 commit 451d1d6
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/pulsecore/iochannel.c
Expand Up @@ -346,14 +346,16 @@ ssize_t pa_iochannel_write_with_creds(pa_iochannel*io, const void*data, size_t l
return r;
}

/* For more details on FD passing, check the cmsg(3) manpage
* and IETF RFC #2292: "Advanced Sockets API for IPv6" */
ssize_t pa_iochannel_write_with_fds(pa_iochannel*io, const void*data, size_t l, int nfd, const int *fds) {
ssize_t r;
int *msgdata;
struct msghdr mh;
struct iovec iov;
union {
struct cmsghdr hdr;
uint8_t data[CMSG_SPACE(sizeof(int) * MAX_ANCIL_DATA_FDS)];
uint8_t data[CMSG_SPACE(sizeof(int) * nfd)];
} cmsg;

pa_assert(io);
Expand Down

0 comments on commit 451d1d6

Please sign in to comment.